Skip to content

Commit

Permalink
NITF: fix undefined behavior when using NITFGetField() several times …
Browse files Browse the repository at this point in the history
…in the same statement

This is not a bug of MSVC. Just that our code was using undefined
behavior.
https://en.cppreference.com/w/c/language/eval_order mentions:
"the expression f1() + f2() + f3() is parsed as (f1() + f2()) + f3() due
to left-to-right associativity of operator+, but the function call to f3
may be evaluated first, last, or between f1() or f2() at run time."
  • Loading branch information
rouault committed Nov 24, 2023
1 parent 0cc27ac commit beb3674
Showing 1 changed file with 34 additions and 12 deletions.
46 changes: 34 additions & 12 deletions frmts/nitf/nitfimage.c
Original file line number Diff line number Diff line change
Expand Up @@ -291,16 +291,28 @@ NITFImage *NITFImageAccess(NITFFile *psFile, int iSegment)
}
else if (psImage->chICORDS == 'G' || psImage->chICORDS == 'C')
{
pdfXY[1] =
CPLAtof(NITFGetField(szTemp, pszCoordPair, 0, 2)) +
CPLAtof(NITFGetField(szTemp, pszCoordPair, 2, 2)) / 60.0 +
// It is critical to do the fetching of the D, M, S components
// in 3 separate statements, otherwise if NITFGetField() is
// defined in this compilation unit, the MSVC optimizer will
// generate bad code, due to szTemp being overwritten before
// being evaluated by CPLAtof() !
pdfXY[1] = CPLAtof(NITFGetField(szTemp, pszCoordPair, 0, 2));
pdfXY[1] +=
CPLAtof(NITFGetField(szTemp, pszCoordPair, 2, 2)) / 60.0;
pdfXY[1] +=
CPLAtof(NITFGetField(szTemp, pszCoordPair, 4, 2)) / 3600.0;
if (pszCoordPair[6] == 's' || pszCoordPair[6] == 'S')
pdfXY[1] *= -1;

pdfXY[0] =
CPLAtof(NITFGetField(szTemp, pszCoordPair, 7, 3)) +
CPLAtof(NITFGetField(szTemp, pszCoordPair, 10, 2)) / 60.0 +
// It is critical to do the fetching of the D, M, S components
// in 3 separate statements, otherwise if NITFGetField() is
// defined in this compilation unit, the MSVC optimizer will
// generate bad code, due to szTemp being overwritten before
// being evaluated by CPLAtof() !
pdfXY[0] = CPLAtof(NITFGetField(szTemp, pszCoordPair, 7, 3));
pdfXY[0] +=
CPLAtof(NITFGetField(szTemp, pszCoordPair, 10, 2)) / 60.0;
pdfXY[0] +=
CPLAtof(NITFGetField(szTemp, pszCoordPair, 12, 2)) / 3600.0;

if (pszCoordPair[14] == 'w' || pszCoordPair[14] == 'W')
Expand Down Expand Up @@ -3087,16 +3099,26 @@ void NITFGetGCP(const char *pachCoord, double *pdfXYs, int iCoord)
/* (00 to 99) of longitude, with Y = E for east or W for west. */
/* ------------------------------------------------------------ */

pdfXYs[1] = CPLAtof(NITFGetField(szTemp, pachCoord, 1, 2)) +
CPLAtof(NITFGetField(szTemp, pachCoord, 3, 2)) / 60.0 +
CPLAtof(NITFGetField(szTemp, pachCoord, 5, 5)) / 3600.0;
// It is critical to do the fetching of the D, M, S components
// in 3 separate statements, otherwise if NITFGetField() is
// defined in this compilation unit, the MSVC optimizer will
// generate bad code, due to szTemp being overwritten before
// being evaluated by CPLAtof() !
pdfXYs[1] = CPLAtof(NITFGetField(szTemp, pachCoord, 1, 2));
pdfXYs[1] += CPLAtof(NITFGetField(szTemp, pachCoord, 3, 2)) / 60.0;
pdfXYs[1] += CPLAtof(NITFGetField(szTemp, pachCoord, 5, 5)) / 3600.0;

if (pachCoord[0] == 's' || pachCoord[0] == 'S')
pdfXYs[1] *= -1;

pdfXYs[0] = CPLAtof(NITFGetField(szTemp, pachCoord, 11, 3)) +
CPLAtof(NITFGetField(szTemp, pachCoord, 14, 2)) / 60.0 +
CPLAtof(NITFGetField(szTemp, pachCoord, 16, 5)) / 3600.0;
// It is critical to do the fetching of the D, M, S components
// in 3 separate statements, otherwise if NITFGetField() is
// defined in this compilation unit, the MSVC optimizer will
// generate bad code, due to szTemp being overwritten before
// being evaluated by CPLAtof() !
pdfXYs[0] = CPLAtof(NITFGetField(szTemp, pachCoord, 11, 3));
pdfXYs[0] += CPLAtof(NITFGetField(szTemp, pachCoord, 14, 2)) / 60.0;
pdfXYs[0] += CPLAtof(NITFGetField(szTemp, pachCoord, 16, 5)) / 3600.0;

if (pachCoord[10] == 'w' || pachCoord[10] == 'W')
pdfXYs[0] *= -1;
Expand Down

0 comments on commit beb3674

Please sign in to comment.