Opened 7 years ago
Closed 7 years ago
#3970 closed defect (fixed)
Undefined behaviour in lwcollection.c and lwpoly.c
Reported by: | Algunenano | Owned by: | strk |
---|---|---|---|
Priority: | medium | Milestone: | PostGIS 2.5.0 |
Component: | liblwgeom | Version: | master |
Keywords: | Cc: |
Description
Detected with clang -fsanizite=integer
lwcollection.c:249:12: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'uint32_t' (aka 'unsigned int') lwpoly.c:322:12: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'uint32_t' (aka 'unsigned int')
GH PR: https://github.com/postgis/postgis/pull/183 For the same price, I've removed some other warnings related to unsigned - signed comparison.
Change History (3)
comment:1 by , 7 years ago
comment:2 by , 7 years ago
In gserialized_from_lwcollection
it is written as uint32_t
/* Write in the number of subgeoms. */ memcpy(loc, &coll->ngeoms, sizeof(uint32_t)); loc += sizeof(uint32_t);
Then it's read as an uint32_t
and implicitly casted to int
in lwcollection_from_gserialized_buffer
:
ngeoms = gserialized_get_uint32_t(data_ptr); collection->ngeoms = ngeoms; /* Zero => empty geometry */
I'm all about changing it to unsigned, but I'd rather use the uint32_t
and be sure it's 4 bytes.
Since that change (either way) requires a lot of modifications I'd rather do it in another ticket. It was being discussed in the mailing list (http://lists.osgeo.org/pipermail/postgis-devel/2017-December/026773.html) but I don't think anyone is working on it.
Alternatively, should the fields in relevant structs be changed from signed to unsigned in liblwgeom.h?
Is it the case that the GSERIALIZED format uses unsigned ints for these quantities, and then they get put into signed fields after deserialization?