From d1c4bc77e2288f265d3155ec6e408ecb4adf946e Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Mon, 30 Dec 2024 16:24:13 +0100 Subject: [PATCH 1/2] GTiff: detect I/O error when getting tile offset/count in multi-threaded reading Fixes #11552 --- autotest/gcore/tiff_read.py | 86 ++++++++++++++++++++++++++++ frmts/gtiff/gtiffdataset.h | 5 +- frmts/gtiff/gtiffdataset_read.cpp | 26 ++++++--- frmts/gtiff/gtiffdataset_write.cpp | 6 +- frmts/gtiff/gtiffrasterband_read.cpp | 16 ++++-- frmts/gtiff/gtiffrgbaband.cpp | 5 +- 6 files changed, 123 insertions(+), 21 deletions(-) diff --git a/autotest/gcore/tiff_read.py b/autotest/gcore/tiff_read.py index 07ce7e15163f..a6f9352aaa59 100755 --- a/autotest/gcore/tiff_read.py +++ b/autotest/gcore/tiff_read.py @@ -5134,6 +5134,92 @@ def method(request): gdal.VSICurlClearCache() +############################################################################### +# Test that we honor GDAL_DISABLE_READDIR_ON_OPEN when working on a dataset opened with OVERVIEW_LEVEL open option + + +@pytest.mark.require_curl() +@pytest.mark.skipif( + not check_libtiff_internal_or_at_least(4, 0, 11), + reason="libtiff >= 4.0.11 required", +) +def test_tiff_read_multi_threaded_vsicurl_error_in_IsBlocksAvailable( + tmp_path, +): + + webserver_process = None + webserver_port = 0 + + (webserver_process, webserver_port) = webserver.launch( + handler=webserver.DispatcherHttpHandler + ) + if webserver_port == 0: + pytest.skip() + + gdal.VSICurlClearCache() + + try: + tmp_filename = str(tmp_path / "test.tif") + ds = gdal.GetDriverByName("GTiff").Create( + tmp_filename, 2001, 10000, 1, options=["SPARSE_OK=YES", "BLOCKYSIZE=1"] + ) + ds.GetRasterBand(1).SetNoDataValue(255) + ds.GetRasterBand(1).Fill(255) + ds.Close() + + filesize = gdal.VSIStatL(tmp_filename).size + handler = webserver.SequentialHandler() + handler.add("HEAD", "/test.tif", 200, {"Content-Length": "%d" % filesize}) + + def method(request): + # sys.stderr.write('%s\n' % str(request.headers)) + + if request.headers["Range"].startswith("bytes="): + rng = request.headers["Range"][len("bytes=") :] + assert len(rng.split("-")) == 2 + start = int(rng.split("-")[0]) + end = int(rng.split("-")[1]) + + request.protocol_version = "HTTP/1.1" + request.send_response(206) + request.send_header("Content-type", "application/octet-stream") + request.send_header( + "Content-Range", "bytes %d-%d/%d" % (start, end, filesize) + ) + request.send_header("Content-Length", end - start + 1) + request.send_header("Connection", "close") + request.end_headers() + with open(tmp_filename, "rb") as f: + f.seek(start, 0) + request.wfile.write(f.read(end - start + 1)) + + for i in range(2): + handler.add("GET", "/test.tif", custom_method=method) + handler.add("GET", "/test.tif", 404) + + with gdaltest.config_options( + { + "GDAL_NUM_THREADS": "2", + "CPL_VSIL_CURL_ALLOWED_EXTENSIONS": ".tif", + "GDAL_DISABLE_READDIR_ON_OPEN": "EMPTY_DIR", + } + ): + with webserver.install_http_handler(handler): + ds = gdal.OpenEx( + "/vsicurl/http://127.0.0.1:%d/test.tif" % webserver_port, + ) + with pytest.raises( + Exception, + match="_TIFFPartialReadStripArray:Cannot read offset/size for strile", + ): + ds.GetRasterBand(1).ReadRaster() + + finally: + webserver.server_stop(webserver_process, webserver_port) + + gdal.VSICurlClearCache() + + ############################################################################### # Test that a user receives a warning when it queries # GetMetadataItem("PIXELTYPE", "IMAGE_STRUCTURE") diff --git a/frmts/gtiff/gtiffdataset.h b/frmts/gtiff/gtiffdataset.h index 3bca95d0ad33..8d8a543279ec 100644 --- a/frmts/gtiff/gtiffdataset.h +++ b/frmts/gtiff/gtiffdataset.h @@ -326,9 +326,8 @@ class GTiffDataset final : public GDALPamDataset int GetJPEGOverviewCount(); - bool IsBlockAvailable(int nBlockId, vsi_l_offset *pnOffset = nullptr, - vsi_l_offset *pnSize = nullptr, - bool *pbErrOccurred = nullptr); + bool IsBlockAvailable(int nBlockId, vsi_l_offset *pnOffset, + vsi_l_offset *pnSize, bool *pbErrOccurred); void ApplyPamInfo(); void PushMetadataToPam(); diff --git a/frmts/gtiff/gtiffdataset_read.cpp b/frmts/gtiff/gtiffdataset_read.cpp index 81c587b0f167..6bf8134c6374 100644 --- a/frmts/gtiff/gtiffdataset_read.cpp +++ b/frmts/gtiff/gtiffdataset_read.cpp @@ -142,7 +142,7 @@ CPLStringList GTiffDataset::GetCompressionFormats(int nXOff, int nYOff, vsi_l_offset nOffset = 0; vsi_l_offset nSize = 0; - if (IsBlockAvailable(nBlockId, &nOffset, &nSize) && + if (IsBlockAvailable(nBlockId, &nOffset, &nSize, nullptr) && nSize < static_cast(std::numeric_limits::max())) { @@ -222,7 +222,7 @@ CPLErr GTiffDataset::ReadCompressedData(const char *pszFormat, int nXOff, vsi_l_offset nOffset = 0; vsi_l_offset nSize = 0; - if (IsBlockAvailable(nBlockId, &nOffset, &nSize) && + if (IsBlockAvailable(nBlockId, &nOffset, &nSize, nullptr) && nSize < static_cast( std::numeric_limits::max())) { @@ -1305,6 +1305,7 @@ CPLErr GTiffDataset::MultiThreadedRead(int nXOff, int nYOff, int nXSize, nBlockId += asJobs[iJob].iSrcBandIdxSeparate * m_nBlocksPerBand; + bool bErrorInIsBlockAvailable = false; if (!sContext.bHasPRead) { // Taking the mutex here is only needed when bHasPRead == @@ -1314,13 +1315,22 @@ CPLErr GTiffDataset::MultiThreadedRead(int nXOff, int nYOff, int nXSize, std::lock_guard oLock( sContext.oMutex); - IsBlockAvailable(nBlockId, &asJobs[iJob].nOffset, - &asJobs[iJob].nSize); + CPL_IGNORE_RET_VAL(IsBlockAvailable( + nBlockId, &asJobs[iJob].nOffset, &asJobs[iJob].nSize, + &bErrorInIsBlockAvailable)); } else { - IsBlockAvailable(nBlockId, &asJobs[iJob].nOffset, - &asJobs[iJob].nSize); + CPL_IGNORE_RET_VAL(IsBlockAvailable( + nBlockId, &asJobs[iJob].nOffset, &asJobs[iJob].nSize, + &bErrorInIsBlockAvailable)); + } + if (bErrorInIsBlockAvailable) + { + std::lock_guard oLock( + sContext.oMutex); + sContext.bSuccess = false; + return CE_Failure; } // Sanity check on block size @@ -1344,7 +1354,7 @@ CPLErr GTiffDataset::MultiThreadedRead(int nXOff, int nYOff, int nXSize, std::lock_guard oLock( sContext.oMutex); sContext.bSuccess = false; - break; + return CE_Failure; } } @@ -6383,7 +6393,7 @@ const char *GTiffDataset::GetMetadataItem(const char *pszName, { vsi_l_offset nOffset = 0; vsi_l_offset nSize = 0; - IsBlockAvailable(0, &nOffset, &nSize); + IsBlockAvailable(0, &nOffset, &nSize, nullptr); if (nSize > 0) { const std::string osSubfile( diff --git a/frmts/gtiff/gtiffdataset_write.cpp b/frmts/gtiff/gtiffdataset_write.cpp index 095b9d19a9e3..97e505cc89f4 100644 --- a/frmts/gtiff/gtiffdataset_write.cpp +++ b/frmts/gtiff/gtiffdataset_write.cpp @@ -397,7 +397,7 @@ CPLErr GTiffDataset::FillEmptyTiles() } vsi_l_offset nOffset = 0; - if (!IsBlockAvailable(iBlock, &nOffset, &nRawSize)) + if (!IsBlockAvailable(iBlock, &nOffset, &nRawSize, nullptr)) break; // When using compression, get back the compressed block @@ -614,7 +614,7 @@ bool GTiffDataset::WriteEncodedTile(uint32_t tile, GByte *pabyData, /* -------------------------------------------------------------------- */ if (!m_bWriteEmptyTiles && IsFirstPixelEqualToNoData(pabyData)) { - if (!IsBlockAvailable(tile)) + if (!IsBlockAvailable(tile, nullptr, nullptr, nullptr)) { const int nComponents = m_nPlanarConfig == PLANARCONFIG_CONTIG ? nBands : 1; @@ -800,7 +800,7 @@ bool GTiffDataset::WriteEncodedStrip(uint32_t strip, GByte *pabyData, /* -------------------------------------------------------------------- */ if (!m_bWriteEmptyTiles && IsFirstPixelEqualToNoData(pabyData)) { - if (!IsBlockAvailable(strip)) + if (!IsBlockAvailable(strip, nullptr, nullptr, nullptr)) { const int nComponents = m_nPlanarConfig == PLANARCONFIG_CONTIG ? nBands : 1; diff --git a/frmts/gtiff/gtiffrasterband_read.cpp b/frmts/gtiff/gtiffrasterband_read.cpp index 63b381671375..59806e6e7e2a 100644 --- a/frmts/gtiff/gtiffrasterband_read.cpp +++ b/frmts/gtiff/gtiffrasterband_read.cpp @@ -983,8 +983,8 @@ void *GTiffRasterBand::CacheMultiRange(int nXOff, int nYOff, int nXSize, } else { - CPL_IGNORE_RET_VAL( - m_poGDS->IsBlockAvailable(nBlockId, &nOffset, &nSize)); + CPL_IGNORE_RET_VAL(m_poGDS->IsBlockAvailable( + nBlockId, &nOffset, &nSize, nullptr)); } if (nSize) { @@ -1129,8 +1129,12 @@ int GTiffRasterBand::IGetDataCoverageStatus(int nXOff, int nYOff, int nXSize, vsi_l_offset nOffset = 0; vsi_l_offset nLength = 0; bool bHasData = false; - if (!m_poGDS->IsBlockAvailable(nBlockId, &nOffset, &nLength)) + bool bError = false; + if (!m_poGDS->IsBlockAvailable(nBlockId, &nOffset, &nLength, + &bError)) { + if (bError) + return GDAL_DATA_COVERAGE_STATUS_UNIMPLEMENTED; nStatus |= GDAL_DATA_COVERAGE_STATUS_EMPTY; } else @@ -1584,7 +1588,8 @@ const char *GTiffRasterBand::GetMetadataItem(const char *pszName, } vsi_l_offset nOffset = 0; - if (!m_poGDS->IsBlockAvailable(nBlockId, &nOffset)) + if (!m_poGDS->IsBlockAvailable(nBlockId, &nOffset, nullptr, + nullptr)) { return nullptr; } @@ -1605,7 +1610,8 @@ const char *GTiffRasterBand::GetMetadataItem(const char *pszName, } vsi_l_offset nByteCount = 0; - if (!m_poGDS->IsBlockAvailable(nBlockId, nullptr, &nByteCount)) + if (!m_poGDS->IsBlockAvailable(nBlockId, nullptr, &nByteCount, + nullptr)) { return nullptr; } diff --git a/frmts/gtiff/gtiffrgbaband.cpp b/frmts/gtiff/gtiffrgbaband.cpp index d527cfd50de6..fdaed879a29f 100644 --- a/frmts/gtiff/gtiffrgbaband.cpp +++ b/frmts/gtiff/gtiffrgbaband.cpp @@ -66,13 +66,14 @@ CPLErr GTiffRGBABand::IReadBlock(int nBlockXOff, int nBlockYOff, void *pImage) for (int iBand = 0; iBand < m_poGDS->m_nSamplesPerPixel; iBand++) { int nBlockIdBand = nBlockId + iBand * m_poGDS->m_nBlocksPerBand; - if (!m_poGDS->IsBlockAvailable(nBlockIdBand)) + if (!m_poGDS->IsBlockAvailable(nBlockIdBand, nullptr, nullptr, + nullptr)) return CE_Failure; } } else { - if (!m_poGDS->IsBlockAvailable(nBlockId)) + if (!m_poGDS->IsBlockAvailable(nBlockId, nullptr, nullptr, nullptr)) return CE_Failure; } From 95cbce0a6f3d1ae2068a56ca2aafc5d4e44b475d Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Mon, 30 Dec 2024 16:28:55 +0100 Subject: [PATCH 2/2] GTiff: CacheMultiRange(): properly react to errors in VSIFReadMultiRangeL() Fixes #11552 --- frmts/gtiff/gtiffrasterband_read.cpp | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/frmts/gtiff/gtiffrasterband_read.cpp b/frmts/gtiff/gtiffrasterband_read.cpp index 59806e6e7e2a..c29be2bc98f4 100644 --- a/frmts/gtiff/gtiffrasterband_read.cpp +++ b/frmts/gtiff/gtiffrasterband_read.cpp @@ -1071,9 +1071,17 @@ void *GTiffRasterBand::CacheMultiRange(int nXOff, int nYOff, int nXSize, VSILFILE *fp = VSI_TIFFGetVSILFile(th); - if (VSIFReadMultiRangeL(static_cast(anSizes.size()), + // An error in VSIFReadMultiRangeL() will not be criticial, + // as this method is an optimization, and if it fails, + // tile-by-tile data acquisition will be done, so we can + // temporary turn failures into warnings. + CPLTurnFailureIntoWarning(true); + const bool ok = + VSIFReadMultiRangeL(static_cast(anSizes.size()), &apData[0], &anOffsets[0], &anSizes[0], - fp) == 0) + fp) == 0; + CPLTurnFailureIntoWarning(false); + if (ok) { if (!oMapStrileToOffsetByteCount.empty() && !FillCacheStrileToOffsetByteCount(anOffsets, anSizes, @@ -1093,6 +1101,11 @@ void *GTiffRasterBand::CacheMultiRange(int nXOff, int nYOff, int nXSize, th, static_cast(anSizes.size()), &apData[0], &anOffsets[0], &anSizes[0]); } + else + { + CPLFree(pBufferedData); + pBufferedData = nullptr; + } } } }