From 8fb53c9b2d122e88a1eef8318b0524b725bd97bd Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Mon, 11 Mar 2024 00:49:46 +0100 Subject: [PATCH 1/3] Internal shapelib: SBNOpenDiskTree(): make it work with node descriptors with non-increasing nBinStart Fixes https://github.com/OSGeo/shapelib/issues/106 / https://github.com/OSGeo/gdal/issues/9430 --- autotest/ogr/ogr_shape_sbn.py | 19 +++ ogr/ogrsf_frmts/shape/sbnsearch.c | 205 +++++++++++++++++++++++++----- 2 files changed, 193 insertions(+), 31 deletions(-) diff --git a/autotest/ogr/ogr_shape_sbn.py b/autotest/ogr/ogr_shape_sbn.py index 6fe5cd0ca991..f528308b6e6e 100755 --- a/autotest/ogr/ogr_shape_sbn.py +++ b/autotest/ogr/ogr_shape_sbn.py @@ -142,3 +142,22 @@ def test_ogr_shape_sbn_2(): ds = ogr.Open("data/shp/CoHI_GCS12.shp") lyr = ds.GetLayer(0) return search_all_features(lyr) + + +############################################################################### +# Test bugfix for https://github.com/OSGeo/gdal/issues/9430 + + +@pytest.mark.require_curl() +def test_ogr_shape_sbn_out_of_order_bin_start(): + + srv = "https://github.com/OSGeo/gdal-test-datasets/raw/master/shapefile/65sv5l285i_GLWD_level2/README.TXT" + if gdaltest.gdalurlopen(srv, timeout=5) is None: + pytest.skip(reason=f"{srv} is down") + + ds = ogr.Open( + "/vsizip//vsicurl/https://github.com/OSGeo/gdal-test-datasets/raw/master/shapefile/65sv5l285i_GLWD_level2/65sv5l285i_GLWD_level2_sozip.zip" + ) + lyr = ds.GetLayer(0) + lyr.SetSpatialFilterRect(5, 5, 6, 6) + assert lyr.GetFeatureCount() == 13 diff --git a/ogr/ogrsf_frmts/shape/sbnsearch.c b/ogr/ogrsf_frmts/shape/sbnsearch.c index be080210d7a8..34ea56350d2e 100644 --- a/ogr/ogrsf_frmts/shape/sbnsearch.c +++ b/ogr/ogrsf_frmts/shape/sbnsearch.c @@ -97,6 +97,24 @@ typedef struct #endif } SearchStruct; +/* Associates a node id with the index of its first bin */ +typedef struct +{ + int nNodeId; + int nBinStart; +} SBNNodeIdBinStartPair; + +/************************************************************************/ +/* SBNCompareNodeIdBinStartPairs() */ +/************************************************************************/ + +/* helper for qsort, to sort SBNNodeIdBinStartPair by increasing nBinStart */ +static int SBNCompareNodeIdBinStartPairs(const void *a, const void *b) +{ + return STATIC_CAST(const SBNNodeIdBinStartPair *, a)->nBinStart - + STATIC_CAST(const SBNNodeIdBinStartPair *, b)->nBinStart; +} + /************************************************************************/ /* SBNOpenDiskTree() */ /************************************************************************/ @@ -254,6 +272,21 @@ SBNSearchHandle SBNOpenDiskTree(const char *pszSBNFilename, hSBN->pasNodeDescriptor = pasNodeDescriptor; + SBNNodeIdBinStartPair *pasNodeIdBinStartPairs = + STATIC_CAST(SBNNodeIdBinStartPair *, + malloc(nNodeDescCount * sizeof(SBNNodeIdBinStartPair))); + if (pasNodeIdBinStartPairs == SHPLIB_NULLPTR) + { + free(pabyData); + hSBN->sHooks.Error("Out of memory error"); + SBNCloseDiskTree(hSBN); + return SHPLIB_NULLPTR; + } + +#ifdef ENABLE_SBN_SANITY_CHECKS + int nShapeCountAcc = 0; +#endif + int nEntriesInNodeIdBinStartPairs = 0; for (int i = 0; i < nNodeDescCount; i++) { /* -------------------------------------------------------------------- */ @@ -261,45 +294,121 @@ SBNSearchHandle SBNOpenDiskTree(const char *pszSBNFilename, /* described it, and the number of shapes in this first bin and */ /* the following bins (in the relevant case). */ /* -------------------------------------------------------------------- */ - int nBinStart = READ_MSB_INT(pabyData + 8 * i); - int nNodeShapeCount = READ_MSB_INT(pabyData + 8 * i + 4); + const int nBinStart = READ_MSB_INT(pabyData + 8 * i); + const int nNodeShapeCount = READ_MSB_INT(pabyData + 8 * i + 4); pasNodeDescriptor[i].nBinStart = nBinStart > 0 ? nBinStart : 0; pasNodeDescriptor[i].nShapeCount = nNodeShapeCount; +#ifdef DEBUG_SBN + fprintf(stderr, "node[%d], nBinStart=%d, nShapeCount=%d\n", i, + nBinStart, nNodeShapeCount); +#endif + if ((nBinStart > 0 && nNodeShapeCount == 0) || nNodeShapeCount < 0 || nNodeShapeCount > nShapeCount) { hSBN->sHooks.Error("Inconsistent shape count in bin"); + free(pabyData); + free(pasNodeIdBinStartPairs); SBNCloseDiskTree(hSBN); return SHPLIB_NULLPTR; } + +#ifdef ENABLE_SBN_SANITY_CHECKS + if (nShapeCountAcc > nShapeCount - nNodeShapeCount) + { + hSBN->sHooks.Error("Inconsistent shape count in bin"); + free(pabyData); + free(pasNodeIdBinStartPairs); + SBNCloseDiskTree(hSBN); + return SHPLIB_NULLPTR; + } + nShapeCountAcc += nNodeShapeCount; +#endif + + if (nBinStart > 0) + { + pasNodeIdBinStartPairs[nEntriesInNodeIdBinStartPairs].nNodeId = i; + pasNodeIdBinStartPairs[nEntriesInNodeIdBinStartPairs].nBinStart = + nBinStart; + ++nEntriesInNodeIdBinStartPairs; + } } free(pabyData); /* pabyData = SHPLIB_NULLPTR; */ - /* Locate first non-empty node */ - int nCurNode = 0; - while (nCurNode < nMaxNodes && pasNodeDescriptor[nCurNode].nBinStart <= 0) - nCurNode++; - - if (nCurNode >= nMaxNodes) + if (nEntriesInNodeIdBinStartPairs == 0) { + free(pasNodeIdBinStartPairs); hSBN->sHooks.Error("All nodes are empty"); SBNCloseDiskTree(hSBN); return SHPLIB_NULLPTR; } - pasNodeDescriptor[nCurNode].nBinOffset = - STATIC_CAST(int, hSBN->sHooks.FTell(hSBN->fpSBN)); +#ifdef ENABLE_SBN_SANITY_CHECKS + if (nShapeCountAcc != nShapeCount) + { + /* Not totally sure if the above condition is always true */ + /* Not enabled by default, as non-needed for the good working */ + /* of our code. */ + free(pasNodeIdBinStartPairs); + char szMessage[128]; + snprintf(szMessage, sizeof(szMessage), + "Inconsistent shape count read in .sbn header (%d) vs total " + "of shapes over nodes (%d)", + nShapeCount, nShapeCountAcc); + hSBN->sHooks.Error(szMessage); + SBNCloseDiskTree(hSBN); + return SHPLIB_NULLPTR; + } +#endif - /* Compute the index of the next non empty node. */ - int nNextNonEmptyNode = nCurNode + 1; - while (nNextNonEmptyNode < nMaxNodes && - pasNodeDescriptor[nNextNonEmptyNode].nBinStart <= 0) - nNextNonEmptyNode++; + /* Sort node descriptors by increasing nBinStart */ + /* In most cases, the node descriptors have already an increasing nBinStart, + * but not for https://github.com/OSGeo/gdal/issues/9430 */ + qsort(pasNodeIdBinStartPairs, nEntriesInNodeIdBinStartPairs, + sizeof(SBNNodeIdBinStartPair), SBNCompareNodeIdBinStartPairs); + + /* Consistency check: the first referenced nBinStart should be 2. */ + if (pasNodeIdBinStartPairs[0].nBinStart != 2) + { + char szMessage[128]; + snprintf(szMessage, sizeof(szMessage), + "First referenced bin (by node %d) should be 2, but %d found", + pasNodeIdBinStartPairs[0].nNodeId, + pasNodeIdBinStartPairs[0].nBinStart); + hSBN->sHooks.Error(szMessage); + SBNCloseDiskTree(hSBN); + free(pasNodeIdBinStartPairs); + return SHPLIB_NULLPTR; + } + + /* And referenced nBinStart should be all distinct. */ + for (int i = 1; i < nEntriesInNodeIdBinStartPairs; ++i) + { + if (pasNodeIdBinStartPairs[i].nBinStart == + pasNodeIdBinStartPairs[i - 1].nBinStart) + { + char szMessage[128]; + snprintf(szMessage, sizeof(szMessage), + "Node %d and %d have the same nBinStart=%d", + pasNodeIdBinStartPairs[i - 1].nNodeId, + pasNodeIdBinStartPairs[i].nNodeId, + pasNodeIdBinStartPairs[i].nBinStart); + hSBN->sHooks.Error(szMessage); + SBNCloseDiskTree(hSBN); + free(pasNodeIdBinStartPairs); + return SHPLIB_NULLPTR; + } + } int nExpectedBinId = 1; + int nIdxInNodeBinPair = 0; + int nCurNode = pasNodeIdBinStartPairs[nIdxInNodeBinPair].nNodeId; + + pasNodeDescriptor[nCurNode].nBinOffset = + STATIC_CAST(int, hSBN->sHooks.FTell(hSBN->fpSBN)); /* -------------------------------------------------------------------- */ /* Traverse bins to compute the offset of the first bin of each */ @@ -316,10 +425,22 @@ SBNSearchHandle SBNOpenDiskTree(const char *pszSBNFilename, int nBinSize = READ_MSB_INT(abyBinHeader + 4); nBinSize *= 2; /* 16-bit words */ +#ifdef DEBUG_SBN + fprintf(stderr, "bin id=%d, bin size (in features) = %d\n", nBinId, + nBinSize / 8); +#endif + if (nBinId != nExpectedBinId) { - hSBN->sHooks.Error("Unexpected bin id"); + char szMessage[128]; + snprintf(szMessage, sizeof(szMessage), + "Unexpected bin id at bin starting at offset %d. Got %d, " + "expected %d", + STATIC_CAST(int, hSBN->sHooks.FTell(hSBN->fpSBN)) - 8, + nBinId, nExpectedBinId); + hSBN->sHooks.Error(szMessage); SBNCloseDiskTree(hSBN); + free(pasNodeIdBinStartPairs); return SHPLIB_NULLPTR; } @@ -327,23 +448,24 @@ SBNSearchHandle SBNOpenDiskTree(const char *pszSBNFilename, /* If there are more, then they are located in continuous bins */ if ((nBinSize % 8) != 0 || nBinSize <= 0 || nBinSize > 100 * 8) { - hSBN->sHooks.Error("Unexpected bin size"); + char szMessage[128]; + snprintf(szMessage, sizeof(szMessage), + "Unexpected bin size at bin starting at offset %d. Got %d", + STATIC_CAST(int, hSBN->sHooks.FTell(hSBN->fpSBN)) - 8, + nBinSize); + hSBN->sHooks.Error(szMessage); SBNCloseDiskTree(hSBN); + free(pasNodeIdBinStartPairs); return SHPLIB_NULLPTR; } - if (nNextNonEmptyNode < nMaxNodes && - nBinId == pasNodeDescriptor[nNextNonEmptyNode].nBinStart) + if (nIdxInNodeBinPair + 1 < nEntriesInNodeIdBinStartPairs && + nBinId == pasNodeIdBinStartPairs[nIdxInNodeBinPair + 1].nBinStart) { - nCurNode = nNextNonEmptyNode; + ++nIdxInNodeBinPair; + nCurNode = pasNodeIdBinStartPairs[nIdxInNodeBinPair].nNodeId; pasNodeDescriptor[nCurNode].nBinOffset = STATIC_CAST(int, hSBN->sHooks.FTell(hSBN->fpSBN)) - 8; - - /* Compute the index of the next non empty node. */ - nNextNonEmptyNode = nCurNode + 1; - while (nNextNonEmptyNode < nMaxNodes && - pasNodeDescriptor[nNextNonEmptyNode].nBinStart <= 0) - nNextNonEmptyNode++; } pasNodeDescriptor[nCurNode].nBinCount++; @@ -352,6 +474,17 @@ SBNSearchHandle SBNOpenDiskTree(const char *pszSBNFilename, hSBN->sHooks.FSeek(hSBN->fpSBN, nBinSize, SEEK_CUR); } + if (nIdxInNodeBinPair + 1 != nEntriesInNodeIdBinStartPairs) + { + hSBN->sHooks.Error("Could not determine nBinOffset / nBinCount for all " + "non-empty nodes."); + SBNCloseDiskTree(hSBN); + free(pasNodeIdBinStartPairs); + return SHPLIB_NULLPTR; + } + + free(pasNodeIdBinStartPairs); + return hSBN; } @@ -537,7 +670,13 @@ static bool SBNSearchDiskInternal(SearchStruct *psSearch, int nDepth, { free(psNode->pabyShapeDesc); psNode->pabyShapeDesc = SHPLIB_NULLPTR; - hSBN->sHooks.Error("Inconsistent shape count for bin"); + char szMessage[128]; + snprintf( + szMessage, sizeof(szMessage), + "Inconsistent shape count for bin idx=%d of node %d. " + "nShapeCountAcc=(%d) + nShapes=(%d) > nShapeCount(=%d)", + i, nNodeId, nShapeCountAcc, nShapes, psNode->nShapeCount); + hSBN->sHooks.Error(szMessage); return false; } @@ -583,7 +722,7 @@ static bool SBNSearchDiskInternal(SearchStruct *psSearch, int nDepth, if (!psNode->bBBoxInit) { /* clang-format off */ -#ifdef sanity_checks +#ifdef ENABLE_SBN_SANITY_CHECKS /* -------------------------------------------------------------------- */ /* Those tests only check that the shape bounding box in the bin */ /* are consistent (self-consistent and consistent with the node */ @@ -639,7 +778,12 @@ static bool SBNSearchDiskInternal(SearchStruct *psSearch, int nDepth, { free(psNode->pabyShapeDesc); psNode->pabyShapeDesc = SHPLIB_NULLPTR; - hSBN->sHooks.Error("Inconsistent shape count for bin"); + char szMessage[96]; + snprintf( + szMessage, sizeof(szMessage), + "Inconsistent shape count for node %d. Got %d, expected %d", + nNodeId, nShapeCountAcc, psNode->nShapeCount); + hSBN->sHooks.Error(szMessage); return false; } @@ -701,8 +845,7 @@ static bool SBNSearchDiskInternal(SearchStruct *psSearch, int nDepth, /* helper for qsort */ static int compare_ints(const void *a, const void *b) { - return *REINTERPRET_CAST(const int *, a) - - *REINTERPRET_CAST(const int *, b); + return *STATIC_CAST(const int *, a) - *STATIC_CAST(const int *, b); } /************************************************************************/ From 167385ac58a2c92b01e18cc57c2fa817dee8d366 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Mon, 11 Mar 2024 00:49:22 +0100 Subject: [PATCH 2/3] sbnsearch.c: avoid potential integer overflow on corrupted bin size --- ogr/ogrsf_frmts/shape/sbnsearch.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/ogr/ogrsf_frmts/shape/sbnsearch.c b/ogr/ogrsf_frmts/shape/sbnsearch.c index 34ea56350d2e..2fabe76963d6 100644 --- a/ogr/ogrsf_frmts/shape/sbnsearch.c +++ b/ogr/ogrsf_frmts/shape/sbnsearch.c @@ -422,12 +422,11 @@ SBNSearchHandle SBNOpenDiskTree(const char *pszSBNFilename, nExpectedBinId++; const int nBinId = READ_MSB_INT(abyBinHeader); - int nBinSize = READ_MSB_INT(abyBinHeader + 4); - nBinSize *= 2; /* 16-bit words */ + const int nBinSize = READ_MSB_INT(abyBinHeader + 4); /* 16-bit words */ #ifdef DEBUG_SBN fprintf(stderr, "bin id=%d, bin size (in features) = %d\n", nBinId, - nBinSize / 8); + nBinSize / 4); #endif if (nBinId != nExpectedBinId) @@ -446,7 +445,7 @@ SBNSearchHandle SBNOpenDiskTree(const char *pszSBNFilename, /* Bins are always limited to 100 features */ /* If there are more, then they are located in continuous bins */ - if ((nBinSize % 8) != 0 || nBinSize <= 0 || nBinSize > 100 * 8) + if ((nBinSize % 4) != 0 || nBinSize <= 0 || nBinSize > 100 * 4) { char szMessage[128]; snprintf(szMessage, sizeof(szMessage), @@ -471,7 +470,7 @@ SBNSearchHandle SBNOpenDiskTree(const char *pszSBNFilename, pasNodeDescriptor[nCurNode].nBinCount++; /* Skip shape description */ - hSBN->sHooks.FSeek(hSBN->fpSBN, nBinSize, SEEK_CUR); + hSBN->sHooks.FSeek(hSBN->fpSBN, nBinSize * sizeof(uint16_t), SEEK_CUR); } if (nIdxInNodeBinPair + 1 != nEntriesInNodeIdBinStartPairs) @@ -652,13 +651,12 @@ static bool SBNSearchDiskInternal(SearchStruct *psSearch, int nDepth, return false; } - int nBinSize = READ_MSB_INT(abyBinHeader + 4); - nBinSize *= 2; /* 16-bit words */ - - int nShapes = nBinSize / 8; + /* 16-bit words */ + const int nBinSize = READ_MSB_INT(abyBinHeader + 4); + const int nShapes = nBinSize / 4; /* Bins are always limited to 100 features */ - if ((nBinSize % 8) != 0 || nShapes <= 0 || nShapes > 100) + if ((nBinSize % 4) != 0 || nShapes <= 0 || nShapes > 100) { hSBN->sHooks.Error("Unexpected bin size"); free(psNode->pabyShapeDesc); @@ -692,9 +690,10 @@ static bool SBNSearchDiskInternal(SearchStruct *psSearch, int nDepth, } #ifdef DEBUG_IO - psSearch->nBytesRead += nBinSize; + psSearch->nBytesRead += nBinSize * sizeof(uint16_t); #endif - if (hSBN->sHooks.FRead(pabyBinShape, nBinSize, 1, hSBN->fpSBN) != 1) + if (hSBN->sHooks.FRead(pabyBinShape, nBinSize * sizeof(uint16_t), 1, + hSBN->fpSBN) != 1) { hSBN->sHooks.Error("I/O error"); free(psNode->pabyShapeDesc); From 0d3e5fda6dcf50211363281e629fd79681711bb7 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Mon, 11 Mar 2024 02:36:49 +0100 Subject: [PATCH 3/3] sbnsearch.c: avoid potential integer overflow on corrupted nNodeDescSize --- ogr/ogrsf_frmts/shape/sbnsearch.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/ogr/ogrsf_frmts/shape/sbnsearch.c b/ogr/ogrsf_frmts/shape/sbnsearch.c index 2fabe76963d6..5fdadeb88ca4 100644 --- a/ogr/ogrsf_frmts/shape/sbnsearch.c +++ b/ogr/ogrsf_frmts/shape/sbnsearch.c @@ -227,26 +227,27 @@ SBNSearchHandle SBNOpenDiskTree(const char *pszSBNFilename, /* There are at most (2^nMaxDepth) - 1, but all are not necessary */ /* described. Non described nodes are empty. */ /* -------------------------------------------------------------------- */ - int nNodeDescSize = READ_MSB_INT(abyHeader + 104); - nNodeDescSize *= 2; /* 16-bit words */ + const int nNodeDescSize = READ_MSB_INT(abyHeader + 104); /* 16-bit words */ /* each bin descriptor is made of 2 ints */ - const int nNodeDescCount = nNodeDescSize / 8; + const int nNodeDescCount = nNodeDescSize / 4; - if ((nNodeDescSize % 8) != 0 || nNodeDescCount < 0 || + if ((nNodeDescSize % 4) != 0 || nNodeDescCount < 0 || nNodeDescCount > nMaxNodes) { char szErrorMsg[64]; snprintf(szErrorMsg, sizeof(szErrorMsg), - "Invalid node descriptor size in .sbn : %d", nNodeDescSize); + "Invalid node descriptor size in .sbn : %d", + nNodeDescSize * STATIC_CAST(int, sizeof(uint16_t))); hSBN->sHooks.Error(szErrorMsg); SBNCloseDiskTree(hSBN); return SHPLIB_NULLPTR; } + const int nNodeDescSizeBytes = nNodeDescCount * 2 * 4; /* coverity[tainted_data] */ unsigned char *pabyData = - STATIC_CAST(unsigned char *, malloc(nNodeDescSize)); + STATIC_CAST(unsigned char *, malloc(nNodeDescSizeBytes)); SBNNodeDescriptor *pasNodeDescriptor = STATIC_CAST( SBNNodeDescriptor *, calloc(nMaxNodes, sizeof(SBNNodeDescriptor))); if (pabyData == SHPLIB_NULLPTR || pasNodeDescriptor == SHPLIB_NULLPTR) @@ -261,7 +262,7 @@ SBNSearchHandle SBNOpenDiskTree(const char *pszSBNFilename, /* -------------------------------------------------------------------- */ /* Read node descriptors. */ /* -------------------------------------------------------------------- */ - if (hSBN->sHooks.FRead(pabyData, nNodeDescSize, 1, hSBN->fpSBN) != 1) + if (hSBN->sHooks.FRead(pabyData, nNodeDescSizeBytes, 1, hSBN->fpSBN) != 1) { free(pabyData); free(pasNodeDescriptor);