GPSBabel: simplify code to avoid false-positive nullptr deref warning…

… (CID 1297184, 1214416), and increase test coverage
OSGeo · Nov 26, 2023 · 5a51594 · 5a51594
1 parent a5ea205
commit 5a51594
Show file tree

Hide file tree

Showing 3 changed files with 51 additions and 10 deletions.
diff --git a/.github/workflows/ubuntu_20.04/Dockerfile.ci b/.github/workflows/ubuntu_20.04/Dockerfile.ci
@@ -20,6 +20,7 @@ RUN apt-get update -y \
     curl \
     git \
     gnupg \
+    gpsbabel \
     lcov \
     libaec-dev \
     libarmadillo-dev \

diff --git a/autotest/ogr/ogr_gpsbabel.py b/autotest/ogr/ogr_gpsbabel.py
@@ -123,3 +123,36 @@ def test_ogr_gpsbabel_3():
     assert not (
         res.find("$GPRMC") == -1 or res.find("$GPGGA") == -1 or res.find("$GPGSA") == -1
     ), "did not get expected result"
+
+
+###############################################################################
+# Test features=
+
+
+@pytest.mark.parametrize("features", ["waypoints", "tracks", "routes"])
+def test_ogr_gpsbabel_features_in_connection_string(features):
+
+    ds = ogr.Open(f"GPSBABEL:gpx:features={features}:data/gpx/test.gpx")
+    assert ds
+    assert ds.GetLayerCount() == (1 if features == "waypoints" else 2)
+    assert ds.GetLayer(0).GetName() == features
+
+
+###############################################################################
+
+
+@gdaltest.enable_exceptions()
+@pytest.mark.parametrize(
+    "connection_string",
+    [
+        "GPSBABEL:",
+        "GPSBABEL:wrong_driver",
+        "GPSBABEL:gpx:wrong_file",
+        "GPSBABEL:gpx:features=wrong_feature:data/gpx/test.gpx",
+        "GPSBABEL:gpx:features=waypoints",
+    ],
+)
+def test_ogr_gpsbabel_bad_connection_string(connection_string):
+
+    with pytest.raises(Exception):
+        ogr.Open(connection_string)
diff --git a/ogr/ogrsf_frmts/gpsbabel/ogrgpsbabeldatasource.cpp b/ogr/ogrsf_frmts/gpsbabel/ogrgpsbabeldatasource.cpp
@@ -145,7 +145,8 @@ int OGRGPSBabelDataSource::Open(const char *pszDatasourceName,
                                 char **papszOpenOptionsIn)
 
 {
-    if (!STARTS_WITH_CI(pszDatasourceName, "GPSBABEL:"))
+    constexpr const char *GPSBABEL_PREFIX = "GPSBABEL:";
+    if (!STARTS_WITH_CI(pszDatasourceName, GPSBABEL_PREFIX))
     {
         CPLAssert(pszGPSBabelDriverNameIn);
         pszGPSBabelDriverName = CPLStrdup(pszGPSBabelDriverNameIn);
@@ -183,25 +184,31 @@ int OGRGPSBabelDataSource::Open(const char *pszDatasourceName,
 
     if (pszGPSBabelDriverName == nullptr)
     {
-        const char *pszSep = strchr(pszDatasourceName + 9, ':');
+        const char *pszDatasourceNameAfterPrefix =
+            pszDatasourceName + strlen(GPSBABEL_PREFIX);
+        const char *pszSep = strchr(pszDatasourceNameAfterPrefix, ':');
         if (pszSep == nullptr)
         {
             CPLError(CE_Failure, CPLE_AppDefined,
                      "Wrong syntax. Expected GPSBabel:driver_name:file_name");
             return FALSE;
         }
 
-        pszGPSBabelDriverName = CPLStrdup(pszDatasourceName + 9);
-        *(strchr(pszGPSBabelDriverName, ':')) = '\0';
+        pszGPSBabelDriverName = CPLStrdup(pszDatasourceNameAfterPrefix);
+        pszGPSBabelDriverName[pszSep - pszDatasourceNameAfterPrefix] = '\0';
 
         /* A bit of validation to avoid command line injection */
         if (!IsValidDriverName(pszGPSBabelDriverName))
             return FALSE;
 
         /* Parse optional features= option */
-        if (STARTS_WITH_CI(pszSep + 1, "features="))
+        const char *pszAfterSep = pszSep + 1;
+        constexpr const char *FEATURES_EQUAL = "features=";
+        if (STARTS_WITH_CI(pszAfterSep, FEATURES_EQUAL))
         {
-            const char *pszNextSep = strchr(pszSep + 1, ':');
+            const char *pszAfterFeaturesEqual =
+                pszAfterSep + strlen(FEATURES_EQUAL);
+            const char *pszNextSep = strchr(pszAfterFeaturesEqual, ':');
             if (pszNextSep == nullptr)
             {
                 CPLError(CE_Failure, CPLE_AppDefined,
@@ -211,8 +218,8 @@ int OGRGPSBabelDataSource::Open(const char *pszDatasourceName,
                 return FALSE;
             }
 
-            char *pszFeatures = CPLStrdup(pszSep + 1 + 9);
-            *strchr(pszFeatures, ':') = 0;
+            char *pszFeatures = CPLStrdup(pszAfterFeaturesEqual);
+            pszFeatures[pszNextSep - pszAfterFeaturesEqual] = 0;
             char **papszTokens = CSLTokenizeString(pszFeatures);
             char **papszIter = papszTokens;
             bool bErr = false;
@@ -242,11 +249,11 @@ int OGRGPSBabelDataSource::Open(const char *pszDatasourceName,
             if (bErr)
                 return FALSE;
 
-            pszSep = pszNextSep;
+            pszAfterSep = pszNextSep + 1;
         }
 
         if (pszFilename == nullptr)
-            pszFilename = CPLStrdup(pszSep + 1);
+            pszFilename = CPLStrdup(pszAfterSep);
     }
 
     const char *pszOptionUseTempFile =
-Original file line number
+Diff line change
@@ Expand Up / @@ -20,6 +20,7 @@ RUN apt-get update -y \ @@
         curl \
         git \
         gnupg \
+        gpsbabel \
         lcov \
         libaec-dev \
         libarmadillo-dev \
@@ Expand Down @@