#3324 closed defect (fixed)
Memory error in postgis provider
Reported by: | strk | Owned by: | pramsey |
---|---|---|---|
Priority: | normal | Milestone: | 5.6.3 release |
Component: | MapServer C Library | Version: | svn-trunk (development) |
Severity: | critical | Keywords: | |
Cc: | greenwood, dmorissette, theduckylittle |
Description (last modified by )
Trunk svn 9801.
==20521== Invalid write of size 1 ==20521== at 0x4E67BC4: _IO_default_xsputn (in /lib/libc-2.9.so) ==20521== by 0x4E3E73C: vfprintf (in /lib/libc-2.9.so) ==20521== by 0x4E5C1EB: vsprintf (in /lib/libc-2.9.so) ==20521== by 0x4E44EFA: sprintf (in /lib/libc-2.9.so) ==20521== by 0x814CFC0: msPostGISBuildSQLBox (in /net/lafont/home/santisa/src/mapserver/trunk/map server/mapserv) ==20521== Address 0x6bcb8c3 is 0 bytes after a block of size 243 alloc'd ==20521== at 0x4027DDE: malloc (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so) ==20521== by 0x814CF62: msPostGISBuildSQLBox (in /net/lafont/home/santisa/src/mapserver/trunk/map server/mapserv)
Attachments (2)
Change History (11)
comment:1 by , 14 years ago
by , 14 years ago
Attachment: | msPostGISBuildSQLBox.invalidwrite.patch added |
---|
Limit number of digits to 15, no matter how many significant. Make sure not to write out of allocated buffer by using snprintf rather than sprintf
comment:2 by , 14 years ago
The attached path makes the code more robust by using snprintf, warns on truncation, uses %f format to get predictable number of digits in output.
comment:3 by , 14 years ago
Owner: | changed from | to
---|
comment:4 by , 14 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Patch applied to 5.4, 5.6 and trunk
comment:5 by , 14 years ago
Description: | modified (diff) |
---|---|
Milestone: | → 5.6.2 release |
Resolution: | fixed |
Status: | closed → reopened |
Reopening.
I have received a report that 5.6.2 (which includes the fix above) breaks with the following error with a mode=map request with mapext=2701967.5295031+1184936.875+2705055.4704969+1186876.375
msDrawMap(): Image handling error. Failed to draw layer named 'ownershipqpoly'. msPostGISLayerWhichShapes(): Query error. Failed to build query SQL. msPostGISBuildSQL(): General error message. Failed to build SQL 'where'. msPostGISBuildSQLWhere(): General error message. Unable to build box SQL. msPostGISBuildSQLBox: General error message. Bounding box digits truncated.
The "%.15f" format does NOT guarantee a total of 15 digits, it requests 15 digits AFTER the decimal point... so in most cases you are guaranteed to get more than 15 digits e.g.
printf("%.15f\n%.15f\n%.15f\n%.15f\n", 2701967.5295031, 1184936.875, 2705055.4704969, 1186876.375); 2701967.529503100086004 1184936.875000000000000 2705055.470496899913996 1186876.375000000000000
I think the right fix would be to use the %.15g format and account a few extra chars per coordinate for the potential use of exponential notation.
comment:6 by , 14 years ago
Cc: | added |
---|
With the %.15g format, the worst case is the exponent notation which is:
[-]m.dddde[+/-]xxx
Based on that the total number of chars should be 15 + 1 for the optional sign, + 1 for the dot, and + up to 5 for the exponent ... so that's up to 22 chars per number for doubles. e.g.
printf("%.15g\n", -1.234567890123456789e300); -1.23456789012346e+300
by , 14 years ago
Attachment: | 3224-2.patch added |
---|
Patch that reverts back to %.15g and tests for 22 chars per value
comment:7 by , 14 years ago
Cc: | added |
---|
Richard, can you please test the new patch and confirm that it fixes the issue with your test case?
comment:8 by , 14 years ago
Cc: | added |
---|---|
Resolution: | → fixed |
Status: | reopened → closed |
Richard has confirmed that the patch to use %.15g and tests for 22 chars per value fixes the issue for him so I have applied it in branch-5-6 r9967 and branch-5-4 r9868.
I see that theduckylittle had already increased the size from 15 to 26 in r9866 (a larger commit for SVG symbol stuff), but had kept the format to %.15f. Unfortunately I don't think that was enough to guard us against possible overflows e.g.
printf("%.15f\n",1e30); 1000000000000000019884624838656.000000000000000
So I have reverted his change and applied the same patch to use use %.15g and tests for 22 chars per value in SVN trunk r9969.
Hopefully that will cover all cases. If anyone can think of a case that the new patch does not cover then please let me know ASAP before I release 5.6.3.
comment:9 by , 14 years ago
Milestone: | 5.6.2 release → 5.6.3 release |
---|
Problem is that the size of the allocated buffer is computed by *assuming* 15 characters for doubles printed with %.15g, but that format requests 15 *significant* digits, not 15 digits in general. Using %.15f would fix that.