[postgis-devel] [postgis-tickets] r15671 - Do not call getPointxx_p on empty or null pointarray

[Daniel Baston]

Yes, I was thinking that with PARANOIA_LEVEL at zero, we shouldn't need
these types of runtime checks at all. (In other words, a function that is
reading a geometry should be checking the number of points before
attempting to access them, so that we don't need to check for emptiness on
every single point access.)  I realize now that this is unrelated to your
commit, so maybe it's a question for the broader group. I will experiment
with making the change and seeing if it regresses OK.

Dan

On Sat, Sep 9, 2017 at 8:35 AM, Sandro Santilli <strk at kbt.io> wrote:

> On Fri, Sep 08, 2017 at 08:28:12PM -0400, Daniel Baston wrote:
> > Should callers be doing this type of checking themselves, instead of
> > incurring the penalty on all point access?
>
> Yes, I think it was good for that check to be only activated
> on PARANOIA_LEVEL > 1
>
> The change of mine (r15671) is really the outermost caller (from the
> liblwgeom point of view) as it's done on a point itself.
>
> Or do you think  lwpoint_getPointXX_p should also not perform
> the test ?
>
> I'd be fine either way, as long as we then patch the higher levels
> (a unit test, in this case, IIRC)
>
> --strk;
>
> >
> > On Fri, Sep 8, 2017 at 5:46 PM, Sandro Santilli <strk at kbt.io> wrote:
> >
> > > Author: strk
> > > Date: 2017-09-08 14:46:17 -0700 (Fri, 08 Sep 2017)
> > > New Revision: 15671
> > >
> > > Modified:
> > >    trunk/liblwgeom/lwpoint.c
> > > Log:
> > > Do not call getPointxx_p on empty or null pointarray
> > >
> > > Add more 0 returns on error
> > >
> > > Modified: trunk/liblwgeom/lwpoint.c
> > > ===================================================================
> > > --- trunk/liblwgeom/lwpoint.c   2017-09-08 21:46:10 UTC (rev 15670)
> > > +++ trunk/liblwgeom/lwpoint.c   2017-09-08 21:46:17 UTC (rev 15671)
> > > @@ -39,24 +39,24 @@
> > >  int
> > >  lwpoint_getPoint2d_p(const LWPOINT *point, POINT2D *out)
> > >  {
> > > -       return getPoint2d_p(point->point, 0, out);
> > > +       return lwpoint_is_empty(point) ? 0 :
> getPoint2d_p(point->point, 0,
> > > out);
> > >  }
> > >
> > >  /* convenience functions to hide the POINTARRAY */
> > >  int
> > >  lwpoint_getPoint3dz_p(const LWPOINT *point, POINT3DZ *out)
> > >  {
> > > -       return getPoint3dz_p(point->point,0,out);
> > > +       return lwpoint_is_empty(point) ? 0 :
> getPoint3dz_p(point->point,0,
> > > out);
> > >  }
> > >  int
> > >  lwpoint_getPoint3dm_p(const LWPOINT *point, POINT3DM *out)
> > >  {
> > > -       return getPoint3dm_p(point->point,0,out);
> > > +       return lwpoint_is_empty(point) ? 0 :
> getPoint3dm_p(point->point,0,
> > > out);
> > >  }
> > >  int
> > >  lwpoint_getPoint4d_p(const LWPOINT *point, POINT4D *out)
> > >  {
> > > -       return getPoint4d_p(point->point,0,out);
> > > +       return lwpoint_is_empty(point) ? 0 :
> getPoint4d_p(point->point,0,
> > > out);
> > >  }
> > >
> > >  double
> > > @@ -64,7 +64,10 @@
> > >  {
> > >         POINT4D pt;
> > >         if ( lwpoint_is_empty(point) )
> > > +       {
> > >                 lwerror("lwpoint_get_x called with empty geometry");
> > > +               return 0;
> > > +       }
> > >         getPoint4d_p(point->point, 0, &pt);
> > >         return pt.x;
> > >  }
> > > @@ -74,7 +77,10 @@
> > >  {
> > >         POINT4D pt;
> > >         if ( lwpoint_is_empty(point) )
> > > +       {
> > >                 lwerror("lwpoint_get_y called with empty geometry");
> > > +               return 0;
> > > +       }
> > >         getPoint4d_p(point->point, 0, &pt);
> > >         return pt.y;
> > >  }
> > > @@ -84,9 +90,15 @@
> > >  {
> > >         POINT4D pt;
> > >         if ( lwpoint_is_empty(point) )
> > > +       {
> > >                 lwerror("lwpoint_get_z called with empty geometry");
> > > +               return 0;
> > > +       }
> > >         if ( ! FLAGS_GET_Z(point->flags) )
> > > +       {
> > >                 lwerror("lwpoint_get_z called without z dimension");
> > > +               return 0;
> > > +       }
> > >         getPoint4d_p(point->point, 0, &pt);
> > >         return pt.z;
> > >  }
> > > @@ -96,9 +108,15 @@
> > >  {
> > >         POINT4D pt;
> > >         if ( lwpoint_is_empty(point) )
> > > +       {
> > >                 lwerror("lwpoint_get_m called with empty geometry");
> > > +               return 0;
> > > +       }
> > >         if ( ! FLAGS_GET_M(point->flags) )
> > > +       {
> > >                 lwerror("lwpoint_get_m called without m dimension");
> > > +               return 0;
> > > +       }
> > >         getPoint4d_p(point->point, 0, &pt);
> > >         return pt.m;
> > >  }
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osgeo.org/pipermail/postgis-devel/attachments/20170910/593556ff/attachment.html>