Opened 16 years ago
Closed 16 years ago
#2199 closed defect (fixed)
[PATCH] Segfault Reading/Writing large images
Reported by: | iigeshii | Owned by: | Even Rouault |
---|---|---|---|
Priority: | normal | Milestone: | 1.5.1 |
Component: | GDAL_Raster | Version: | svn-trunk |
Severity: | normal | Keywords: | large array 64 bit |
Cc: | warmerdam |
Description
I tracked down a bug while attempting to open a 2.7GB image file using a 64-bit linux OS.
The seg fault occurs during a memcpy when the pointer address becomes negative do to an integer overflow during a multiply/add.
The problem is the type (int) that is used in the multiply for iBufYOff * nLineSpace is not sufficient to hold a large enough number to point to the required memory address offset. Casting it to type (size_t) prior to the multiplication ensures the resulting type is large enough for the memory address space of the machine you are working on.
Here is the original and suggested code change.
root/trunk/gdal/gcore/rasterio.cpp
Before: 109 if( eRWFlag == GF_Read ) 110 memcpy( ((GByte *) pData) + iBufYOff * nLineSpace, 111 pabySrcBlock + nSrcByteOffset, 112 nLineSpace ); 113 else 114 memcpy( pabySrcBlock + nSrcByteOffset, 115 ((GByte *) pData) + iBufYOff * nLineSpace, 116 nLineSpace ); After: 109 if( eRWFlag == GF_Read ) 110 memcpy( ((GByte *) pData) + ((size_t)iBufYOff * (size_t)nLineSpace), 111 pabySrcBlock + nSrcByteOffset, 112 nLineSpace ); 113 else 114 memcpy( pabySrcBlock + nSrcByteOffset, 115 ((GByte *) pData) + ((size_t)iBufYOff * ((size_t)nLineSpace), 116 (size_t)nLineSpace );
Attachments (1)
Change History (6)
comment:1 by , 16 years ago
comment:2 by , 16 years ago
Summary: | Segfault Reading/Writing large images → [PATCH] Segfault Reading/Writing large images |
---|
Woo, you're raising an interesting issue ! It would be very interesting if you could attach the result of gdalinfo on your image so we can see its dimensions and its block size.
I think your patch is a start, but by reading the code, I can see that there are many other places in that same function where we can have issues with multiplying integers. I'm attaching a patch that tries to address more potential issues. Please note I've no images (and probably not enough RAM lol) to actually test it in a case where problems might happen. I'm quite afraid that there may be other places in GDAL that should also be fixed.
by , 16 years ago
Attachment: | gdal_svn_trunk_fix2199.patch added |
---|
comment:3 by , 16 years ago
Sorry it took me a while. I wasn't able to do exactly what you asked. But I did construct a new, even larger tiff to play with. Here is the gdalinfo for it.
gdalinfo works both prior and after the patch was applied.
[1058.hornet.source] ls -alh /volume102/rp/images/BigImage/big.tif -rw-rw-r-- 1 milese vivid 3.7G 2008-02-08 09:53 /volume102/rp/images/BigImage/big.tif [1059.hornet.source] gdalinfo /volume102/rp/images/BigImage/big.tif Driver: GTiff/GeoTIFF Size is 25600, 19200 Coordinate System is `' Corner Coordinates: Upper Left ( 0.0, 0.0) Lower Left ( 0.0,19200.0) Upper Right (25600.0, 0.0) Lower Right (25600.0,19200.0) Center (12800.0, 9600.0) Band 1 Block=25600x1 Type=UInt16, ColorInterp=Red Band 2 Block=25600x1 Type=UInt16, ColorInterp=Green Band 3 Block=25600x1 Type=UInt16, ColorInterp=Blue Band 4 Block=25600x1 Type=UInt16, ColorInterp=Alpha
Note that did apply your patch and attempted an image read. Your changes seem to check out ok. Thanks!
-Eric
comment:4 by , 16 years ago
Cc: | added |
---|---|
Milestone: | → 1.6.0 |
Owner: | changed from | to
Eric,
Could you clarify the circumstances when the problem is encountered? Are you reading the whole image into a memory buffer in one RasterIO() gulp?
Even,
Feel free to apply your patch in trunk and even 1.5 branch if there is no risk of damage. But I'm not *really* all that concerned about managing memory arrays larger than 2GB yet. I can live with encouraging developers to do such big reads in chunks for a while.
comment:5 by , 16 years ago
Keywords: | large array 64 bit added |
---|---|
Milestone: | 1.6.0 → 1.5.1 |
Resolution: | → fixed |
Status: | new → closed |
Typo in my suggested change. It helps if I try to compile prior to submitting a suggested change =).
Here it is again in diff format after a tested compile: