[PATCH] Heap over-read in compute_geometry_stats()

[landon.j.fuller]

What steps will reproduce the problem?

- We saw SIGSEGVs occurring during all vacuums of a database containing the TIGER2007FE dataset (~56 gigs).

The SIGSEGV occurs when the allocator returns a buffer that ends directly prior to a page-aligned address, and the next page is unmapped. The code reads past the end of the allocated buffer, and triggers the SIGSEGV on the unmapped page

What version of the product are you using? On what operating system?

PostGIS 1.3.3 on FreeBSD 7.0/amd64.

Please provide any additional information below.

I tracked this issue down to compute_geometry_stats(), where a BOX2DFLOAT4 struct is heap allocated:

sampleboxes[notnull_cnt] = palloc(sizeof(BOX2DFLOAT4));

And then later, copied to another palloced buffer:

BOX2DFLOAT4 *box; box = (BOX2DFLOAT4 *)sampleboxes[i]; ... newhistobox = palloc(sizeof(BOX)); memcpy(newhistobox, box, sizeof(BOX));

sizeof(BOX) is used, however both the source and destination are BOX2DFLOAT4 structs. While the newly allocated destination buffer is large enough to hold sizeof(BOX), the source buffer, allocated using sizeof(BOX2DFLOAT4), is not:

sizeof(BOX) == 32 sizeof(BOX2DFLOAT4) == 16

This leads to memcpy reading beyond the end of source box in the memcpy. If the address past the source box was invalid, SIGSEGV resulted -- otherwise, junk data was copied and left unused.

I've attached patches for both trunk and 1.3.3. With the patch in place, vacuum completes successfully.

[landon.j.fuller]

 http://postgis.googlecode.com/issues/attachment?aid=5231877255340216755&name=patch-compute_geometry_stats-trunk.diff

[mcayland]

Hi Landon,

Thank you very much for the comprehensive bug report and supplied patch - I've just applied this to both SVN trunk and the 1.3 branch. The error you were seeing comes from the fact that the original code was written to use the PostgreSQL BOX type rather than the PostGIS BOX2DFLOAT4 type, and so it seems that this is the one place that was accidentally missed during the conversion. A quick check indicates that this is the only remaining use of BOX within the file so there should be no more similar problems lurking.

ATB,

Mark.

[landon.j.fuller]

 http://postgis.googlecode.com/issues/attachment?aid=-3939073518746649323&name=patch-compute_geometry_stats-1_3_3.diff