New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Liblwgeom] Use fixed width integers #183

Closed
wants to merge 17 commits into
base: svn-trunk
from

Conversation

Projects
None yet
5 participants
@Algunenano
Member

Algunenano commented Jan 4, 2018

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')

For the same price, I've removed some other warnings related to unsigned - signed comparison.

Trac issue: https://trac.osgeo.org/postgis/ticket/3970

@codecov

This comment has been minimized.

codecov bot commented Jan 4, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (svn-trunk@fdf42e9). Click here to learn what that means.
The diff coverage is 86.44%.

Impacted file tree graph

@@             Coverage Diff              @@
##             svn-trunk     #183   +/-   ##
============================================
  Coverage             ?   79.24%           
============================================
  Files                ?      201           
  Lines                ?    62905           
  Branches             ?        0           
============================================
  Hits                 ?    49850           
  Misses               ?    13055           
  Partials             ?        0
Impacted Files Coverage Δ
liblwgeom/lwmpoly.c 70.58% <ø> (ø)
liblwgeom/g_box.c 81.96% <ø> (ø)
liblwgeom/cunit/cu_unionfind.c 100% <ø> (ø)
liblwgeom/cunit/cu_geodetic.c 99.75% <ø> (ø)
liblwgeom/lwhomogenize.c 89.7% <ø> (ø)
liblwgeom/lwout_encoded_polyline.c 95.34% <ø> (ø)
liblwgeom/cunit/cu_geos.c 98.14% <ø> (ø)
postgis/lwgeom_dumppoints.c 97.91% <ø> (ø)
postgis/gserialized_gist_nd.c 66.35% <ø> (ø)
raster/rt_core/rt_pixel.c 88.95% <ø> (ø)
... and 72 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fdf42e9...572c565. Read the comment docs.

@Komzpa Komzpa requested review from pramsey and strk Jan 4, 2018

@Komzpa

Hey @Algunenano would you like to tweak it so that there is no going below zero at all instead of changing type to one going below zero? :)

@@ -236,7 +236,7 @@ LWCOLLECTION* lwcollection_add_lwgeom(LWCOLLECTION *col, const LWGEOM *geom)
LWCOLLECTION *
lwcollection_segmentize2d(const LWCOLLECTION *col, double dist)
{
uint32_t i;
int i;

This comment has been minimized.

@Komzpa

Komzpa Jan 9, 2018

Member

I think that more robust way to fix the warning is to rewrite while statement to skip subtraction if it's 0 already, something like this:

while (i) lwgeom_free(newgeoms[i--]);

This comment has been minimized.

@Algunenano

Algunenano Jan 9, 2018

Member

That maintains another different warning since col->ngeoms is signed and uint32_t unsigned:

lwcollection.c:245:13: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  for (i=0; i<col->ngeoms; i++)

Setting it to int solves both issues.

This comment has been minimized.

@Komzpa

Komzpa Jan 9, 2018

Member

It looks like it does not solve but instead hides them :)

Can you instead try changing all collections' ngeoms and maxgeoms it to uint32_t (or even size_t) in
https://github.com/postgis/postgis/blame/9715e11745bb54a981c7402d46b4191e9d680a81/liblwgeom/liblwgeom.h.in#L584 ? It may open a can of worms, but there they are.

This comment has been minimized.

@Algunenano

Algunenano Jan 9, 2018

Member

It was being discussed in here (http://lists.osgeo.org/pipermail/postgis-devel/2017-December/026773.html) and I'm all for it, but I don't want to go ahead and edit hundreds of lines to get it rejected later.

@strk

This comment has been minimized.

Member

strk commented Jan 9, 2018

@Komzpa

This comment has been minimized.

Member

Komzpa commented Jan 9, 2018

@strk it's LWGEOM, not GSERIALIZED, and int is already differently sized - why would same-size matter?

@strk

This comment has been minimized.

Member

strk commented Jan 9, 2018

@pramsey

This comment has been minimized.

Member

pramsey commented Jan 9, 2018

Because of constructions like this, which I assume are all over the place https://github.com/postgis/postgis/blob/svn-trunk/liblwgeom/g_serialized.c#L849

@Komzpa

This comment has been minimized.

Member

Komzpa commented Jan 9, 2018

Okay, so there needs to be sizeof(lwgeom.ngeoms) == sizeof(gserialized.ngeoms) somewhere too.
With presence of such memcpy's around the code I agree that uint32_t is a type to convert ngeoms/maxgeoms to.

@Algunenano will this be enough for you to believe it's not going to be simply rejected? :)

@Algunenano

This comment has been minimized.

Member

Algunenano commented Jan 9, 2018

I understand changing the API in liblwgeom.h isn't a problem, right? For example, with the change
extern LWPOINT* lwline_get_lwpoint(const LWLINE *line, int where);
becomes
extern LWPOINT* lwline_get_lwpoint(const LWLINE *line, uint32_t where);

@pramsey

This comment has been minimized.

Member

pramsey commented Jan 9, 2018

Mostly. There may be some insertion functions that use -1 to mean "at the beginning".

@Algunenano

This comment has been minimized.

Member

Algunenano commented Jan 9, 2018

Yeah, I see that the body of some functions will need to be modified and I trust the compiler to complain about it, but I was asking about the type of the function parameters which should be modified also (unless you want a lot of casting in the bodies).

@strk

This comment has been minimized.

Member

strk commented Jan 9, 2018

@Komzpa

This comment has been minimized.

Member

Komzpa commented Jan 9, 2018

@Algunenano feel free to break API, it should be ok to do so in 2.5.0.

@Algunenano Algunenano changed the title from Undefined behaviour in lwcollection.c and lwpoly.c to WIP: [Liblwgeom] Use fixed width integers Jan 9, 2018

@dbaston

This comment has been minimized.

Member

dbaston commented Jan 9, 2018

Going out on a limb here, what do people think about typedeffing npoints_t and ngeoms_t to be uint32_t. Would that improve clarity, or just be weird?

@Algunenano

This comment has been minimized.

Member

Algunenano commented Jan 9, 2018

Going out on a limb here, what do people think about typedeffing npoints_t and ngeoms_t to be uint32_t. Would that improve clarity, or just be weird?

From my experience you end up with a lot of types and, since AFAIK in C you cannot block int type conversions it's not really that useful.

@Algunenano

This comment has been minimized.

Member

Algunenano commented Jan 10, 2018

This is now ready for review. It contains some extra fixes I found during the development:

  • eba5211: Fix for knn_recheck with Postgresql and parallel insert into (which made some ids unpredictable).
  • 1863435 and 248f0b5: Related to sprintf might not have enough space to write.
    If you would rather have those commits in separate PRs please let me know.
  • 8d96eeb: Missing initialization.

The rest are related to the changes (or provoked by them) or just clearing sign-compare warnings. Please let me know if you need me to change anything or you'd rather get different PRs instead of different commits.

Tested in a Linux x86_64 machine with clang (5.0.1) and gcc (7.2.1). The sanitizers did not show anything except what I commented in https://trac.osgeo.org/postgis/ticket/3971#comment:12 but it appears to be solved in #186. I'd expect at least some warnings in 32bit systems.

@Algunenano Algunenano changed the title from WIP: [Liblwgeom] Use fixed width integers to [Liblwgeom] Use fixed width integers Jan 10, 2018

@Komzpa

I like this patch. I think it's not worth splitting out missing initialization.
For sign-compare warnings, I think it's worth another look - some of them look to me as candidates not to be silenced via forced casting, but for other side of operator to be converted to unsigned too. Otherwise with warnings silenced we'll lose them forever :)

I've commented on the cases I've noticed.

@@ -3515,7 +3514,8 @@ int ptarray_contains_point_sphere(const POINTARRAY *pa, const POINT2D *pt_outsid
POINT3D S1, S2; /* Stab line end points */
POINT3D E1, E2; /* Edge end points (3-space) */
POINT2D p; /* Edge end points (lon/lat) */
int count = 0, i, inter;
uint32_t i;
int count = 0, inter;

This comment has been minimized.

@Komzpa

Komzpa Jan 10, 2018

Member

can count or inter be negative?

This comment has been minimized.

@Algunenano

Algunenano Jan 11, 2018

Member

Not really. I'll change edge_intersects to return uint32_t instead (it's returning flags) t change both count and inter to unsigned too.

@@ -795,7 +795,7 @@ LWGEOM_GEOS_makeValidCollection(const GEOSGeometry* gin)
int nvgeoms;
GEOSGeometry **vgeoms;
GEOSGeom gout;
unsigned int i;
int i;

This comment has been minimized.

@Komzpa

Komzpa Jan 10, 2018

Member

why signed?
I see similar while(i--) loop as before, is that the only reason?

This comment has been minimized.

@Algunenano

Algunenano Jan 11, 2018

Member

in this case it's comparing with nvgeoms which is and int coming from GEOSGetNumGeometries. There are a couple more appearances of this in lwgeom_geos.c:

liblwgeom/lwgeom_geos.c
440:                                    while (i) GEOSGeom_destroy(geoms[--i]);
482:                            while (j) GEOSGeom_destroy(geoms[--j]);

Would you rather remove these and use a for loop as done in other places?

This comment has been minimized.

@Komzpa

Komzpa Jan 11, 2018

Member

It would be great. In those constructs I see possibility of off-by-one if i/j is interpreted as length or last element.

This comment has been minimized.

@Algunenano

Algunenano Jan 11, 2018

Member

On it. I have to agree with you when you see code like this:

				if ( ! geoms[i-1] )
				{
					--i;
					while (i) GEOSGeom_destroy(geoms[--i]);
@@ -795,7 +795,7 @@ LWGEOM_GEOS_makeValidCollection(const GEOSGeometry* gin)
int nvgeoms;

This comment has been minimized.

@Komzpa

Komzpa Jan 10, 2018

Member

can this one become signed if GEOSGetNumGeometries gets converted to return INT32_MAX instead of -1? do we need the check for -1 there at all?

This comment has been minimized.

@Algunenano

Algunenano Jan 11, 2018

Member

GEOSGetNumGeometries returns an int, using -1 to signal errors. If it were to return something like uint32_t then returning UINT32_MAX (which should be -1 in all platforms I've checked, but I wouldn't fight for it) would be nice. For compatibility reasons I would not return INT32_MAX if the type is int. Not that I expect support for 16bit platforms, but who knows.

This comment has been minimized.

@Komzpa

Komzpa Jan 11, 2018

Member

can you add assert that returned value is less than UINT32_MAX then?

This comment has been minimized.

@Algunenano

Algunenano Jan 11, 2018

Member

This is pure geos code, so I don't think that is needed. It could be added in ptarray_from_GEOSCoordSeq which I think it's the border between the 2 libraries.
Currently if by any chance the GEOS geometry had more points than UINT32_MAX it'd have some issues, but that was there before any of these changes. We could start looking for all those funny interactions, but then this PR would take months and a lot of extra tests, hopefully with the help of the fuzzer.

This comment has been minimized.

@Komzpa

Komzpa Jan 11, 2018

Member

What bothers me is that we've had a warning of some checking system about the funny interaction, and it gets silenced without rethinking the interaction. It's ok to silence not all the warnings in this PR anyway, so I think it's better if borderline cases with casting in the middle of comparing get skipped for now and fixed in later coming months :)

This comment has been minimized.

@Algunenano

Algunenano Jan 11, 2018

Member

The warning here had nothing to do with one of those funny interactions; since all code is related to GEOS there isn't any interaction with "liblwgeom". It's just that it was using an unsigned int to iterate over a signed count, but since it checks it doesn't return -1 and iterates over nvgeoms using uint was fine but using int finer (because it shuts up the compiler).
I can change the check if ( nvgeoms == -1 ) { to if ( nvgeoms < 0 ) { if that gives you peace of mind.

This comment has been minimized.

@Komzpa
@@ -3003,7 +3003,7 @@ _lwt_FindNextRingEdge(const POINTARRAY *ring, int from,
LWDEBUGF(1, "First point of edge %" LWTFMT_ELEMID
" matches ring vertex %d", isoe->edge_id, from);
/* first point matches, let's check next non-equal one */
for ( j=1; j<epa->npoints; ++j )
for ( j=1; j<(int32_t)epa->npoints; ++j )

This comment has been minimized.

@Komzpa

Komzpa Jan 10, 2018

Member

what was original type for epa->npoints? why are you casting it to signed int32_t?

This comment has been minimized.

@Algunenano

Algunenano Jan 11, 2018

Member

Switching j to int64_t to avoid issues with npoints > INT32_MAX. As stated in the other comment the function needs to be reworked to guarantee no undefined behaviour when using uints.

This comment has been minimized.

@Komzpa

Komzpa Jan 11, 2018

Member

Can j be just uint32_t with a couple of asserts that disallow going below zero?

@@ -2972,7 +2972,7 @@ _lwt_FindNextRingEdge(const POINTARRAY *ring, int from,
POINTARRAY *epa = edge->points;
POINT2D p2, pt;
int match = 0;
int j;
int32_t j;

This comment has been minimized.

@Komzpa

Komzpa Jan 10, 2018

Member

why is it signed?

This comment has been minimized.

@Algunenano

Algunenano Jan 11, 2018

Member

It does some arithmetic that could lead to UB with uint:
for ( j=epa->npoints-2; j>=0; --j )
Since I didn't see no guarantee in the code that epa->npoints is at least 2 I went the easy way. The function would need to be reworked to properly use uint32_t instead.

This comment has been minimized.

@Komzpa

Komzpa Jan 11, 2018

Member

Can you assert(epa->npoints > 2) then so it fails assertion and not mysteriously?

This comment has been minimized.

@Algunenano

Algunenano Jan 11, 2018

Member

I'll add an if statement since that edge can be safely ignored.

@@ -1732,7 +1732,7 @@ compute_gserialized_stats_mode(VacAttrStats *stats, AnalyzeAttrFetchFunc fetchfu
*/
do
{
ND_BOX nd_cell;
ND_BOX nd_cell = { {0.0, 0.0, 0.0, 0.0}, {0.0, 0.0, 0.0, 0.0} };

This comment has been minimized.

@Komzpa

Komzpa Jan 10, 2018

Member

this is a great catch!

This comment has been minimized.

@Algunenano

Algunenano Jan 11, 2018

Member

Props to the compiler warning

@@ -310,7 +310,7 @@ static void parse_column_keys(mvt_agg_context *ctx)
char *key;
POSTGIS_DEBUG(2, "parse_column_keys called");
for (i = 0; i < natts; i++) {
for (i = 0; i < (uint32_t) natts; i++) {

This comment has been minimized.

@Komzpa

Komzpa Jan 10, 2018

Member

can natts be converted to uint32_t at assignment at start of function?

This comment has been minimized.

@Algunenano

Algunenano Jan 11, 2018

Member

Will do.

@@ -1564,7 +1564,7 @@ convert_raster(int idx, RTLOADERCFG *config, RASTERINFO *info, STRINGBUFFER *til
GDALDatasetH hdsSrc;
GDALRasterBandH hbandSrc;
int nband = 0;
int i = 0;
uint32_t i = 0;
int ntiles[2] = {1, 1};
int _tile_size[2] = {0, 0};

This comment has been minimized.

@Komzpa

Komzpa Jan 10, 2018

Member

can _tile_size be also converted to unsigned here instead of below?

This comment has been minimized.

@Algunenano

Algunenano Jan 11, 2018

Member

It does some subtractions like:

_tile_size[1] = info->dim[1] - (ytile * info->tile_size[1]);

Without going too deep into it I'm not able to determine if it'd go under zero.

This comment has been minimized.

@Komzpa

Komzpa Jan 11, 2018

Member

can you do that adding an assert(info->dim[1] >= (ytile * info->tile_size[1]))?

@@ -2522,7 +2522,7 @@ main(int argc, char **argv) {
elements = NULL;
n = 0;
for (j = 0; j < config->overview_count; j++) {
for (j = 0; j < (uint32_t)config->overview_count; j++) {

This comment has been minimized.

@Komzpa

Komzpa Jan 10, 2018

Member

can overview_count be unsigned on declaration?

This comment has been minimized.

@Algunenano

Algunenano Jan 11, 2018

Member

Switching overview_count and rt_file_count both to uint32_t

@@ -368,7 +368,7 @@ rt_raster_set_srid(rt_raster raster, int32_t srid) {
_rt_raster_geotransform_warn_offline_band(raster);
}
int
uint16_t

This comment has been minimized.

@Komzpa

Komzpa Jan 10, 2018

Member

why 16?

This comment has been minimized.

@Algunenano

Algunenano Jan 11, 2018

Member

numBands is a uint16_t and rt_raster_get_width and rt_raster_get_height were already returning a 16 for the similar reasons.

@dbaston

Overall looks good, but I would need to find something other than GitHub to do a thorough review in. The overflow issues are probably outside the scope of this PR but maybe should be noted with TODOs and/or tickets.

{
int i = 0;
uint32_t i = 0;
int v = 0; /* vertices */

This comment has been minimized.

@dbaston

dbaston Jan 10, 2018

Member

This can overflow, right? Going to unsigned would help, but the collection can have 2^32 geoms, and each geom can have 2^32 vertices.

This comment has been minimized.

@Algunenano

Algunenano Jan 11, 2018

Member

Yes. I've changed v to uint32_t and added a TODO comment in lwgeom_count_vertices since both the internal functions lwpoly_count_vertices and lwcollection_count_vertices. Should be fixed by using uint64_t, I think.

@@ -1276,7 +1276,7 @@ int lwgeom_count_vertices(const LWGEOM *geom)
* 2 for polygons, 3 for volume, and the max dimension
* of a collection.
*/
int lwgeom_dimension(const LWGEOM *geom)
uint32_t lwgeom_dimension(const LWGEOM *geom)

This comment has been minimized.

@dbaston

dbaston Jan 10, 2018

Member

Did this need to be changed?

This comment has been minimized.

@Algunenano

Algunenano Jan 11, 2018

Member

Nice catch. It shouldn't be changed unless the behaviour is changed (change returning -1 to return UINT32_MAX for example). I'll leave it as it was for now.

@@ -1109,7 +1109,7 @@ lwtype_is_collection(uint8_t type)
/**
* Given an lwtype number, what homogeneous collection can hold it?
*/
int
uint32_t

This comment has been minimized.

@dbaston

dbaston Jan 10, 2018

Member

Should be uint8_t I think

This comment has been minimized.

@Algunenano

Algunenano Jan 11, 2018

Member

I've set it as uint32_t because lwgeom_get_type was already returning uint32_t. Let me know if you'd rather change both.

@@ -1173,8 +1173,8 @@ static size_t gserialized_from_gbox(const GBOX *gbox, uint8_t *buf)
GSERIALIZED* gserialized_from_lwgeom(LWGEOM *geom, size_t *size)
{
size_t expected_size = 0;
size_t return_size = 0;
uint32_t expected_size = 0;

This comment has been minimized.

@dbaston

dbaston Jan 10, 2018

Member

Sizes should remain as size_t, I think

This comment has been minimized.

@Algunenano

Algunenano Jan 11, 2018

Member

You are right.

@strk

This comment has been minimized.

Member

strk commented Jan 10, 2018

@Algunenano

This comment has been minimized.

Member

Algunenano commented Jan 10, 2018

Also you can get the patch by appending .patch to any commit or the PR itself, for example: https://github.com/postgis/postgis/pull/183.patch

@dbaston

This comment has been minimized.

Member

dbaston commented Jan 10, 2018

Sorry if that was unclear (I know how to use git), I mean that I would want to find an effective UI for reviewing several hundred disconnected changes.

Algunenano added some commits Jan 10, 2018

rtpg_mapalgebra: Make sure there is enough space for sprintf in the s…
…tatic arrays

Example warning:
rtpg_mapalgebra.c:4873:36: error: ‘__builtin___sprintf_chk’ may write a terminating nul past the end of the destination [-Werror=format-overflow=]
rtpg_mapalgebra: Avoid using uninitialized values
rtpg_mapalgebra.c:6197:27: error: ‘pgrast[1]’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
@Algunenano

This comment has been minimized.

Member

Algunenano commented Jan 11, 2018

Rebased and added a couple of extra commits to address comments and pending issues (last 3 [3c954cf,
c237392, 6841b35])

@@ -4479,10 +4479,10 @@ _lwt_HealEdges( LWT_TOPOLOGY* topo, LWT_ELEMID eid1, LWT_ELEMID eid2,
if ( node_edges[i].edge_id == eid2 ) continue;
commonnode = -1;
/* append to string, for error message */
if ( bufleft ) {
if ( bufleft > 0 ) {

This comment has been minimized.

@Komzpa
@Komzpa

Komzpa approved these changes Jan 11, 2018

@Komzpa

This comment has been minimized.

Member

Komzpa commented Jan 11, 2018

I like this version and think it's ok to commit.

@dbaston does it look ok to you?

@Algunenano

This comment has been minimized.

Member

Algunenano commented Jan 11, 2018

Test are failing due to Database postgis_reg already exists.. Anything we can do to clean it?

@Komzpa

This comment has been minimized.

Member

Komzpa commented Jan 11, 2018

@Algunenano this is after recent commit 586a437

Actual failure: https://travis-ci.org/postgis/postgis/jobs/327545716#L6110

@strk is aware of it (pinged on irc.freenode.net/#postgis) and committed a fix. Closing and reopening to kick rebuild.

@Komzpa Komzpa closed this Jan 11, 2018

@Komzpa Komzpa reopened this Jan 11, 2018

@Komzpa

This comment has been minimized.

Member

Komzpa commented Jan 11, 2018

@Algunenano regression:

-----------------------------------------------------------------------------
--- regress/st_getfaceedges_expected	2018-01-11 14:17:45.625556298 +0000
+++ /tmp/pgis_reg/test_19_out	2018-01-11 14:21:25.269048655 +0000
@@ -13,20 +13,9 @@
 E7
 F1
 F2
-F1|1|-4
-F1|2|5
-F1|3|7
-F1|4|-6
-F1|5|1
-F1|6|2
-F1|7|3
-F2|1|-1
-F2|2|-3
-F2|3|-2
-F0|1|4
-F0|2|6
-F0|3|-7
-F0|4|-5
+ERROR:  No edge (among 7) found to be defining geometry of face 1
+ERROR:  No edge (among 3) found to be defining geometry of face 2
+ERROR:  No edge (among 4) found to be defining geometry of face 0
 Topology 'tt' dropped
 t
 #3265.1|E1
@@ -35,7 +24,5 @@
 #3265.4|0|(1,-1)
 #3265.4|0|(2,2)
 #3265.5|1|(1,-3)
-#3265.6|2|(1,1)
-#3265.6|2|(2,-2)
-#3265.6|2|(3,3)
+ERROR:  No edge (among 3) found to be defining geometry of face 2
 Topology 'tt' dropped
-----------------------------------------------------------------------------
@Algunenano

This comment has been minimized.

Member

Algunenano commented Jan 11, 2018

Working on it. Not sure if my local environment was dirty or an issue with the rebase.

Apparently I wasn't running topology tests. Fixing it has surface some issues with PG11 ordering

@Algunenano

This comment has been minimized.

Member

Algunenano commented Jan 11, 2018

Added a commit to fix the regression and another to force ordering in some test that were failing in pg11.

@Komzpa

This comment has been minimized.

Member

Komzpa commented Jan 12, 2018

@dbaston please wave when you think it's ok to commit. :)

@dbaston

This comment has been minimized.

Member

dbaston commented Jan 12, 2018

@strk strk closed this in f9e9411 Jan 12, 2018

@Komzpa

This comment has been minimized.

Member

Komzpa commented Jan 12, 2018

@Algunenano thanks for your patience :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment