Opened 5 months ago

Closed 5 months ago

Last modified 5 months ago

#7204 closed defect (fixed)

NTF Generic Attribute Reading: Offset at end of record treated as error instead of simply as end of data

Reported by: molnar Owned by: warmerdam
Priority: normal Milestone: 2.1.5
Component: OGR_SF Version: unspecified
Severity: normal Keywords:
Cc:

Description

This commit guards against overrunning the attribute record buffer, which is good: https://github.com/OSGeo/gdal/commit/9c3a4a49a3d95cf9d122dee4449308e7e57b98e9

However, I think this commit may be a bit overzealous in safety: https://github.com/OSGeo/gdal/commit/2b3f5f329d671e8d0a94a73ad6920f391cec1228

See line 953

iOffset = nEnd;
if( iOffset >= poRecord->GetLength() )
{
   bError = true;
   break;
}

I believe an offset that is equal to the record length should simply break the loop, and should not be an error. Greater offsets may still be an error.

I might suggest:

iOffset = nEnd;
const int nRecordLength = poRecord->GetLength();
if( iOffset >= nRecordLength )
{
   bError = (iOffset > nRecordLength);
   break;
}

or something similar. Attaching a repro dataset

Attachments (1)

ntf-repro.tar.gz (622.2 KB) - added by molnar 5 months ago.
Repro dataset

Download all attachments as: .zip

Change History (5)

Changed 5 months ago by molnar

Attachment: ntf-repro.tar.gz added

Repro dataset

comment:1 Changed 5 months ago by Even Rouault

Resolution: fixed
Status: newclosed

In 41288:

NTF: fix regression introduced by fix for https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=2166 causing some valid records to be skipped (patch by molnar, fixes #7204)

comment:2 Changed 5 months ago by Even Rouault

In 41289:

NTF: fix regression introduced by fix for https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=2166 causing some valid records to be skipped (patch by molnar, fixes #7204)

comment:3 Changed 5 months ago by Even Rouault

In 41290:

NTF: fix regression introduced by fix for https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=2166 causing some valid records to be skipped (patch by molnar, fixes #7204)

comment:4 Changed 5 months ago by Even Rouault

Milestone: 2.1.5
Note: See TracTickets for help on using tickets.