[postgis-tickets] [PostGIS] #3980: [SFCGAL] Potential access to freed memory

[PostGIS]

#3980: [SFCGAL] Potential access to freed memory
----------------------+---------------------------
  Reporter:  lucasvr  |      Owner:  colivier
      Type:  defect   |     Status:  new
  Priority:  medium   |  Milestone:  PostGIS 2.4.4
 Component:  sfcgal   |    Version:  2.4.x
Resolution:           |   Keywords:
----------------------+---------------------------

Comment (by lucasvr):

 I have just checked and confirmed that your intuition is right. For
 reference, here is the code path for a conversion of a POINT geometry
 (please read this as if it were a simplified stack trace):

 {{{
 #1  ptarray_construct_reference_data(pt_list):
     POINTARRAY *pa = lwalloc(..);
     pa->serialized_pointlist = pt_list;
     return pa;

 #2  lwpoint_from_gserialized_buffer(data_ptr):
     LWPOINT *point = lwalloc(..);
     point->point = ptarray_construct_reference_data(data_ptr);
     return point;

 #3  lwgeom_from_gserialized(input_geom):
     return lwgeom_from_gserialized_buffer(data_ptr[=input_geom]);

 #4  POSTGIS2SFCGALGeometry(input_geom):
     LWGEOM *lwgeom = lwgeom_from_gserialized(input_geom);
     sfcgal_geometry_t *g = LWGEOM2SFCGAL(lwgeom);
     lwgeom_free(lwgeom);
     return g;
 }}}

 On frame 4, the returned LWGEOM holds a reference to input_geom through
 point->serialized_pointlist. However, as you suspected,
 POSTGIS2SFCGALGeometry will indeed create a deep copy of
 serialized_pointlist, so it's safe to keep the calls to PG_FREE_IF_COPY in
 their original place in cases where the geometry is retrieved through that
 function.

 On the other hand, freeing the input on sfcgal_make_solid() is indeed
 dangerous. Assuming the input is a POINT:
 - {{{LWGEOM *lwgeom = lwgeom_from_gserialized(input_geom)}}} will return
 an object with a reference to input_geom through
 point->serialized_pointlist
 - {{{PG_FREE_IF_COPY(input_geom)}}} will be called
 - {{{GSERIALIZED *output = geometry_serialize(lwgeom)}}} will eventually
 access freed memory (see stack trace below).


 {{{
 #1  getPoint_internal(pa):
     ptr = pa->serialized_pointlist + size * n;
     // access invalid memory through *(ptr+offset)

 #2  gserialized_from_lwpoint(point):
     memcpy(buf, getPoint_internal(point->point), ..);

 #3  gserialized_from_lwgeom_any(lwgeom):
     return gserialized_from_lwpoint(lwgeom);

 #4  gserialized_from_lwgeom(lwgeom):
     gserialized_from_lwgeom_any(lwgeom);

 #5  geometry_serialize(lwgeom):
     return gserialized_from_lwgeom(lwgeom);
 }}}


 I will update my patch accordingly.

--
Ticket URL: <https://trac.osgeo.org/postgis/ticket/3980#comment:7>
PostGIS <http://trac.osgeo.org/postgis/>
The PostGIS Trac is used for bug, enhancement & task tracking, a user and developer wiki, and a view into the subversion code repository of PostGIS project.