Opened 12 years ago

Closed 12 years ago

#2075 closed enhancement (fixed)

[PATCH] Add VSISafeMul2, etc. functions to detect overflows

Reported by: Even Rouault Owned by: Even Rouault
Priority: normal Milestone: 1.6.0
Component: default Version: svn-trunk
Severity: normal Keywords: overflow memory allocation
Cc: warmerdam, Mateusz Łoskot

Description

In GDAL, we frequently do things like VSIMalloc(nXSize * nYSize * sizeof(float)). If nXSize and nYSize are big enough and well choosen, the result can be a moderate value, and VSIMalloc can return a non-NULL pointer. However, the memory buffer will probably be overrun in later operations.

The first patch gdal_svn_trunk_vsi_safe_mul.patch adds new API to do safe multiplications with overflow checking. The second patch uses these new API in gdal/gcore. The third patch uses these new API in gdal/frmts. Note that it only uses these new API at places where VSIMalloc/VSICalloc are already used. Drivers using CPLMalloc/CPLCalloc would crash on big allocations and should be modified to use VSIMalloc/VSICalloc, but that's a much bigger effort.

Attachments (3)

gdal_svn_trunk_use_vsi_safe_mul_in_gcore.patch (4.5 KB) - added by Even Rouault 12 years ago.
Use of new API in gdal/gcore
gdal_svn_trunk_vsi_safe_mul.patch (4.1 KB) - added by Even Rouault 12 years ago.
gdal_svn_trunk_use_vsi_safe_mul_in_frmts.patch (17.8 KB) - added by Even Rouault 12 years ago.
Use of new API in gdal/frmts

Download all attachments as: .zip

Change History (11)

Changed 12 years ago by Even Rouault

Use of new API in gdal/gcore

comment:1 Changed 12 years ago by nowakpl

root@r0:~# cat malloc_test.cpp #include <stdio.h> #include <stdlib.h>

int main() {

printf("%p\n", malloc(0)); return 0;

} root@r0:~# ./malloc_test 0x804a008

So if you use VSIMalloc(VSISafeMul2(x, y)), VSISafeMul2 will return 0, VSIMalloc will allocate 0 bytes and the code that used it will think everything is ok.

VSISafeSizetCastToInt has hardcoded int size + returns correct int value on error.

comment:2 Changed 12 years ago by Even Rouault

gdal_svn_trunk_vsi_safe_mul.patch updated so that VSIMalloc/Calloc/Realloc return NULL when size is 0.

Changed 12 years ago by Even Rouault

Changed 12 years ago by Even Rouault

Use of new API in gdal/frmts

comment:3 Changed 12 years ago by Even Rouault

I'm wondering if the change in VSIMalloc/Calloc/Realloc is not a bit too disruptive... I had to change hfa_band.cpp line 891 in order not to break hfa_write.py.

comment:4 Changed 12 years ago by warmerdam

Milestone: 1.6.0

Even,

I'd be more comfortable holding this change to 1.6 at this point. Feel free to apply the patch once we have branched off a 1.5 branch.

comment:5 Changed 12 years ago by Mateusz Łoskot

Cc: Mateusz Łoskot added

comment:6 Changed 12 years ago by Even Rouault

A revised proposal of the patches is described in RFC 19.

comment:7 Changed 12 years ago by Even Rouault

Changes commited in trunk in r13498, r13499 and r13500

comment:8 Changed 12 years ago by Even Rouault

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.