Skip to content

Commit

Permalink
Merge pull request #11559 from rouault/fix_11558
Browse files Browse the repository at this point in the history 
GTiff: more robust handling of I/O errors in network optimized code paths
  • Loading branch information
@rouault
rouault authored Jan 4, 2025
2 parents a5f476e + 95cbce0 commit 37aa84a
Show file tree
Hide file tree
Showing 6 changed files with 138 additions and 23 deletions.
86 changes: 86 additions & 0 deletions autotest/gcore/tiff_read.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
5 changes: 2 additions & 3 deletions frmts/gtiff/gtiffdataset.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
26 changes: 18 additions & 8 deletions frmts/gtiff/gtiffdataset_read.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<vsi_l_offset>(std::numeric_limits<tmsize_t>::max()))
{
Expand Down Expand Up @@ -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<vsi_l_offset>(
std::numeric_limits<tmsize_t>::max()))
{
Expand Down Expand Up @@ -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 ==
Expand All @@ -1314,13 +1315,22 @@ CPLErr GTiffDataset::MultiThreadedRead(int nXOff, int nYOff, int nXSize,
std::lock_guard<std::recursive_mutex> 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<std::recursive_mutex> oLock(
sContext.oMutex);
sContext.bSuccess = false;
return CE_Failure;
}

// Sanity check on block size
Expand All @@ -1344,7 +1354,7 @@ CPLErr GTiffDataset::MultiThreadedRead(int nXOff, int nYOff, int nXSize,
std::lock_guard<std::recursive_mutex> oLock(
sContext.oMutex);
sContext.bSuccess = false;
break;
return CE_Failure;
}
}

Expand Down Expand Up @@ -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(
Expand Down
6 changes: 3 additions & 3 deletions frmts/gtiff/gtiffdataset_write.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
33 changes: 26 additions & 7 deletions frmts/gtiff/gtiffrasterband_read.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -1071,9 +1071,17 @@ void *GTiffRasterBand::CacheMultiRange(int nXOff, int nYOff, int nXSize,

VSILFILE *fp = VSI_TIFFGetVSILFile(th);

if (VSIFReadMultiRangeL(static_cast<int>(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<int>(anSizes.size()),
&apData[0], &anOffsets[0], &anSizes[0],
fp) == 0)
fp) == 0;
CPLTurnFailureIntoWarning(false);
if (ok)
{
if (!oMapStrileToOffsetByteCount.empty() &&
!FillCacheStrileToOffsetByteCount(anOffsets, anSizes,
Expand All @@ -1093,6 +1101,11 @@ void *GTiffRasterBand::CacheMultiRange(int nXOff, int nYOff, int nXSize,
th, static_cast<int>(anSizes.size()), &apData[0],
&anOffsets[0], &anSizes[0]);
}
else
{
CPLFree(pBufferedData);
pBufferedData = nullptr;
}
}
}
}
Expand Down Expand Up @@ -1129,8 +1142,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
Expand Down Expand Up @@ -1584,7 +1601,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;
}
Expand All @@ -1605,7 +1623,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;
}
Expand Down
5 changes: 3 additions & 2 deletions frmts/gtiff/gtiffrgbaband.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down

0 comments on commit 37aa84a

Please sign in to comment.