#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: trunk
Keywords: Cc:


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 Changed 19 months ago by dbaston

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?

comment:2 Changed 19 months ago by Algunenano

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.

comment:3 Changed 18 months ago by komzpa

Resolution: fixed
Status: newclosed

In 16270:

Use fixed width integers.

Patch by Raúl Marín Rodríguez.

Closes #3970
Closes https://github.com/postgis/postgis/pull/183

Note: See TracTickets for help on using tickets.