Possible bug in NITF TRE parser engine

[bradh]

I'm working on adding support for the ENGRDA TRE for NITF.

The XML is relatively straight forward:

+    <!-- STDI-0002 Appendix N -->
+    <tre name="ENGRDA">
+        <field name="RESRC" length="20" type="string"/>
+        <field name="RECNT" length="3" type="integer" minval="1"/>
+        <loop counter="RECNT" md_prefix="RECORD_%d_" name="RECORDS">
+            <field name="ENGLN" length="2" type="integer" minval="1"/>
+            <field name="ENGLBL" length_var="ENGLN" type="string"/>
+            <field name="ENGMTXC" length="4" type="integer" minval="1"/>
+            <field name="ENGMTXR" length="4" type="integer" minval="1"/>
+            <field name="ENGTYP" length="1" type="string"/>
+            <field name="ENGDTS" length="1" type="integer"/>
+            <field name="ENGDTU" length="2" type="string"/>
+            <field name="ENGDATC" length="8" type="integer" minval="1" maxval="99999932"/>
+            <field name="ENGDATA" length_var="ENGDATC"/>
+        </loop>
+    </tre>

Unfortunately it doesn't work right in GDAL (it does work OK in my Java parser).

I see:

  <tre name="ENGRDA" location="image">
    <field name="RESRC" value="LAIR2EOSensor" />
    <field name="RECNT" value="023" />
    <repeated name="RECORDS" number="23">
      <group index="0">
        <field name="ENGLN" value="07" />
        <field name="ENGLBL" value="version" />
        <field name="ENGMTXC" value="0001" />
        <field name="ENGMTXR" value="0001" />
        <field name="ENGTYP" value="A" />
        <field name="ENGDTS" value="1" />
        <field name="ENGDTU" value="NA" />
        <field name="ENGDATC" value="00000001" />
        <field name="ENGDATA" value="2" />
      </group>
      <group index="1">
        <field name="ENGLN" value="12" />
        <field name="ENGLBL" value="majorVe" />
        <field name="ENGMTXC" value="rsio" />
        <field name="ENGMTXR" value="n000" />
        <field name="ENGTYP" value="1" />
        <field name="ENGDTS" value="0" />
        <field name="ENGDTU" value="00" />
        <field name="ENGDATC" value="1A1NA000" />
        <field name="ENGDATA" value="0" />
      </group>

Note that the ENGLBL in the second block is truncated and some of it ends up being parsed as ENGMTXC and ENGMTXR.

So I would expect to see:

  <tre name="ENGRDA" location="image">
    <field name="RESRC" value="LAIR2EOSensor" />
    <field name="RECNT" value="023" />
    <repeated name="RECORDS" number="23">
      <group index="0">
        <field name="ENGLN" value="07" />
        <field name="ENGLBL" value="version" />
        <field name="ENGMTXC" value="0001" />
        <field name="ENGMTXR" value="0001" />
        <field name="ENGTYP" value="A" />
        <field name="ENGDTS" value="1" />
        <field name="ENGDTU" value="NA" />
        <field name="ENGDATC" value="00000001" />
        <field name="ENGDATA" value="2" />
      </group>
      <group index="1">
        <field name="ENGLN" value="12" />
        <field name="ENGLBL" value="majorVersion" />
        <field name="ENGMTXC" value="0001" />
        <field name="ENGMTXR" value="0001" />
        <field name="ENGTYP" value="A" />
        <field name="ENGDTS" value="1" />
        <field name="ENGDTU" value="NA" />
        <field name="ENGDATC" value="00000001" />
        <field name="ENGDATA" value="2" />
      </group>

(in both cases I've removed the other 21 groups - this is already long enough)

I think the problem is that the parser is using the ENGLN value from the first group when parsing the second group (and every other group after that).

I have a suggestion that fixes this:

diff --git a/gdal/frmts/nitf/nitffile.c b/gdal/frmts/nitf/nitffile.c
index 9bda6d7..68cae38 100644
--- a/gdal/frmts/nitf/nitffile.c
+++ b/gdal/frmts/nitf/nitffile.c
@@ -2145,7 +2145,6 @@ static char** NITFGenericMetadataReadTREInternal(char **papszMD,
                             if (pszEqual != NULL)
                             {
                                 nLength = atoi(pszEqual + 1);
-                                break;
                             }
                         }
                         papszMDIter ++;

However I'm not confident that I understand the code well enough. So happy to propose a pull request (or attach a patch), but not sure its right, or what the other consequences are.

I do have redistributable files that demonstrates this, the smallest one is attached.

[bradh]

Patch with XML bits

[bradh]

Sample image from ​https://www.sdms.afrl.af.mil/index.php?collection=wpafb2009

[bradh]

Looks like trac will allow me to attach that size file, so sample included.

[Even Rouault]

Thanks Brad. I've slightly modified it to be a bit more straightforward.

trunk r32464 "NITF: add definition of ENGRDA TRE and fix parser to properly deal with variable length items not in first nesting level (derived from patch by bradh, #6285)"

branches/2.0 r32465, branches/1.11 r32466 "NITF: fix TRE parser to properly deal with variable length items not in first nesting level, such as ENGRDA (not added in nitf_spec.xml in this changeset) (derived from patch by bradh, #6285)"

[bradh]

Thanks, very much appreciated, especially for the exceptionally quick response!