Ticket #2199 (closed defect: fixed)

Opened 3 months ago

Last modified 2 months ago

[PATCH] Segfault Reading/Writing large images

Reported by: iigeshii Assigned to: 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

gdal_svn_trunk_fix2199.patch (3.8 kB) - added by rouault on 02/06/08 16:49:54.

Change History

02/05/08 21:56:54 changed by iigeshii

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

02/06/08 16:49:30 changed by rouault

  • summary changed from Segfault Reading/Writing large images to [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.

02/06/08 16:49:54 changed by rouault

  • attachment gdal_svn_trunk_fix2199.patch added.

02/11/08 15:59:37 changed by iigeshii

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

03/04/08 15:52:01 changed by warmerdam

  • owner changed from warmerdam to rouault.
  • cc set to warmerdam.
  • milestone set to 1.6.0.

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.

03/04/08 16:05:20 changed by rouault

  • keywords set to large array 64 bit.
  • status changed from new to closed.
  • resolution set to fixed.
  • milestone changed from 1.6.0 to 1.5.1.

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