Opened 6 years ago

Closed 6 years ago

#4246 closed defect (fixed)

Undefined behaviour in define_plane

Reported by: Algunenano Owned by: Algunenano
Priority: high Milestone: PostGIS 3.0.0
Component: postgis Version: master
Keywords: Cc:

Description

Reproducible with regress test:

SELECT '3dDistancetest6',
	ST_3DDistance(a,b) FROM (
	SELECT 'LINESTRING(1 1 1 , 2 2 2)'::geometry as a, 'POLYGON((0 0 0, 2 2 2, 3 3 3, 0 0 0))'::geometry as b) as foo;

Clang sanitizer:

#0  define_plane (pa=0x55f1780347c8, pl=<optimized out>) at measures3d.c:1146
1146            if((pa->npoints-1)==3) /*Triangle is special case*/
(gdb) bt
#0  define_plane (pa=0x55f1780347c8, pl=<optimized out>) at measures3d.c:1146
#1  0x00007f8574ef911b in lw_dist3d_line_poly (line=<optimized out>, poly=<optimized out>, dl=0x7ffda6b15110) at measures3d.c:672
#2  0x00007f8574ef8d2d in lw_dist3d_distribute_bruteforce (lwg1=0x1, lwg2=<optimized out>, dl=<optimized out>) at measures3d.c:549
#3  0x00007f8574ef8454 in lw_dist3d_recursive (lwg1=0x55f178034700, lwg2=0x55f178034760, dl=0x7ffda6b15110) at measures3d.c:466
#4  0x00007f8574ef86e2 in lwgeom_mindistance3d_tolerance (lw1=0x55f178034700, lw2=0x55f178034760, tolerance=0) at measures3d.c:376
#5  lwgeom_mindistance3d (lw1=0x55f178034700, lw2=0x55f178034760) at measures3d.c:355
#6  0x00007f8574e97283 in LWGEOM_mindistance3d (fcinfo=0x55f1780329e0) at lwgeom_functions_basic.c:928
#7  0x000055f176357250 in ExecInterpExpr (state=<optimized out>, econtext=<optimized out>, isnull=0x7ffda6b152df) at execExprInterp.c:678
#8  0x000055f1764235ae in ExecEvalExprSwitchContext (state=<optimized out>, econtext=0x11, isNull=0xc40ba7bf2bc10000) at ../../../../src/include/executor/executor.h:303
#9  evaluate_expr (expr=<optimized out>, result_type=701, result_typmod=-1, result_collation=0) at clauses.c:4900

The debugger is pointing to the line 1146 but when stepping trough the code it's crashing around:

1183                    sumx+=(v.x/vl);

So it's probably a division by zero.

Change History (11)

comment:1 by Algunenano, 6 years ago

Owner: changed from pramsey to Algunenano

Disabling optimizations gives a clearer view; vl is 0:

#0  0x00007fa1e1151b2e in define_plane (pa=0x55c1ce446560, pl=0x7ffe1a2fffd8) at measures3d.c:1183
1183                    sumx+=(v.x/vl);
(gdb) bt f
#0  0x00007fa1e1151b2e in define_plane (pa=0x55c1ce446560, pl=0x7ffe1a2fffd8) at measures3d.c:1183
        i = 3
        j = 1
        numberofvectors = 3
        pointsinslice = 1
        p = {x = 3, y = 3, z = 3}
        p1 = {x = 0, y = 0, z = 0}
        p2 = {x = 2, y = 2, z = 2}
        sumx = 0
        sumy = 0
        sumz = 0
        vl = 0
        v1 = {x = -1.6666666666666667, y = -1.6666666666666667, z = -1.6666666666666667}
        v2 = {x = 0.33333333333333326, y = 0.33333333333333326, z = 0.33333333333333326}
        v = {x = 0, y = 0, z = 0}

comment:2 by nicklas, 6 years ago

Thank you! good catch

I am guilty. It has lived there for quite a long time.

Do you fix it or do you want me to?

comment:3 by Algunenano, 6 years ago

I'm on it, thanks.

comment:4 by Algunenano, 6 years ago

I've found several issues when investigating this peculiar case but the main one is that the polygon used (POLYGON((0 0 0, 2 2 2, 3 3 3, 0 0 0))) isn't valid as it's just defining a 3d line.

@nicklas What do you think is the proper way to handle this? I'm thinking about lwerror("%s: Polygon does not define a plane", __func__); when the polygon passed does not define a plane. It's either that or be clever or default to line to line distance, but then we'd ignoring possible holes so I don't really like it.

I'm also adding a bunch of tests around the lwgeom_mindistance3d_tolerance since it appears to have several issues with division by zero when the geometries interect.

comment:5 by nicklas, 6 years ago

I can take a deeper look later today. But from what I can remember is that I had problems handling cases when there is no plane. If the vertex points is creating a wave for instance.

But I don't remember how I thought I solved it.

If I just left it? or if I tried to catch something?

But I think for sure that it isn't bullet proof :-(

comment:6 by Algunenano, 6 years ago

Here is the PR with the fixes if you want to have a look https://github.com/postgis/postgis/pull/338

comment:7 by nicklas, 6 years ago

Hey, I'm impressed by myself :-)

I actually documentated the thoughts about defining the plane when I wrote those functions.

I started to recall those thoughts, but here it is in the wiki.

https://trac.osgeo.org/postgis/wiki/3DDistancecalc

comment:8 by Raul Marin, 6 years ago

In 17050:

Fix undefined behaviour in ST_3DDistance

References #4246

comment:9 by Raul Marin, 6 years ago

In 17051:

Fix undefined behaviour in ST_3DDistance

References #4246

comment:10 by Raul Marin, 6 years ago

In 17052:

Fix undefined behaviour in ST_3DDistance

References #4246

comment:11 by Raul Marin, 6 years ago

Resolution: fixed
Status: newclosed

In 17053:

Fix undefined behaviour in ST_3DDistance

Closes #4246
Closes https://github.com/postgis/postgis/pull/338

Note: See TracTickets for help on using tickets.