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)

gdal_svn_trunk_fix2199.patch (3.8 KB ) - added by Even Rouault 16 years ago.

Download all attachments as: .zip

Change History (6)

comment:1 by iigeshii, 16 years ago

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:

[516.hornet.source] diff -c gdal-1.4.2/gcore/rasterio.cpp orig/gdal-1.4.2/gcore/rasterio.cpp
*** gdal-1.4.2/gcore/rasterio.cpp       2008-02-05 18:16:19.530012000 -0700
--- orig/gdal-1.4.2/gcore/rasterio.cpp  2007-06-27 11:47:49.000000000 -0700
***************
*** 107,118 ****
              if( eDataType == eBufType )
              {
                  if( eRWFlag == GF_Read )
!                     memcpy( ((GByte *) pData) + ((size_t)iBufYOff * (size_t)nLineSpace),
                              pabySrcBlock + nSrcByteOffset,
                              nLineSpace );
                  else
                      memcpy( pabySrcBlock + nSrcByteOffset,
!                             ((GByte *) pData) + ((size_t)iBufYOff * (size_t)nLineSpace),
                              nLineSpace );
              }
              else
--- 107,118 ----
              if( eDataType == eBufType )
              {
                  if( eRWFlag == GF_Read )
!                     memcpy( ((GByte *) pData) + iBufYOff * nLineSpace,
                              pabySrcBlock + nSrcByteOffset,
                              nLineSpace );
                  else
                      memcpy( pabySrcBlock + nSrcByteOffset,
!                             ((GByte *) pData) + iBufYOff * nLineSpace,
                              nLineSpace );
              }
              else

comment:2 by Even Rouault, 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 Even Rouault, 16 years ago

comment:3 by iigeshii, 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 warmerdam, 16 years ago

Cc: warmerdam added
Milestone: 1.6.0
Owner: changed from warmerdam to Even Rouault

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 Even Rouault, 16 years ago

Keywords: large array 64 bit added
Milestone: 1.6.01.5.1
Resolution: fixed
Status: newclosed

Patch applied in trunk in r13926 and in branches/1.5 in r13927

Note: See TracTickets for help on using tickets.