From beb3674845b65065f943b9ba29848388d2619cfb Mon Sep 17 00:00:00 2001
From: Even Rouault <even.rouault@spatialys.com>
Date: Fri, 24 Nov 2023 01:43:06 +0100
Subject: [PATCH] NITF: fix undefined behavior when using NITFGetField()
 several times 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."
---
 frmts/nitf/nitfimage.c | 46 +++++++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 12 deletions(-)

diff --git a/frmts/nitf/nitfimage.c b/frmts/nitf/nitfimage.c
index ff31ce5cd5dc..592d6fd0f44a 100644
--- a/frmts/nitf/nitfimage.c
+++ b/frmts/nitf/nitfimage.c
@@ -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')
@@ -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;