[SFCGAL] Potential access to freed memory

[lucasvr]

lwgeom_from_gserialized(input) returns a pointer to an allocated LWGEOM structure:

lwgeom_from_gserialized(input) {

... lwgeom = lwgeom_from_gserialized_buffer(input->data, ...); ... return lwgeom;

}

Depending on the input data type (e.g., point, line, circular string, polygon), a different member of the returned LWGEOM structure will hold a reference to the original input, such as lwgeom->point, lwgeom->points, and lwgeom->rings.

This means that it is not safe to invoke PG_FREE_IF_COPY(input) right after getting a reference to the LWGEOM structure. One can only free it after the object is serialized back or once the returned LWGEOM structure is not needed anymore.

There are several spots on postgis/lwgeo_sfcgal.c where early calls to PG_FREE_IF_COPY() may be corrupting memory accessed by SFCGAL, as in sfcgal_make_solid() and sfcgal_geometry_extrude(). It would be good if somebody could review that code just to double check if this is indeed the case.

[strk]

sfcgal_make_solid surely looks unsafe as the PG_FREE_IF_COPY'ed memory is accessed by subsequently called serializer, I bet other places are also unsafe. It makes sense, IMHO, to simply move the PG_FREE_IF_COPY to after the serialization. Up for a patch or git merge request ?

[lucasvr]

Sure thing. I am attaching a patch that moves calls to PG_FREE_IF_COPY near the end of each function, just to be on the safe side. Note that I've included extra calls to PG_FREE_IF_COPY on conditional branches that call elog(ERROR). My understanding is that a call to such a function raises an exception and aborts the execution of the code underneath, so that extra call should the introduction of memory leaks. Please let me know if that's not how elog(ERROR) behaves.

[strk]

All memory in PosgreSQL is allocated in "pools" and the DETOAST memory pool will be freed up upon elog(ERROR, ...) so that's not a problem.

I guess POSTGIS2SFCGALGeometry will fully copy the data to SFCGAL representation, so there's no problem freening the input after that, it's just lwgeom_from_gserialized that holds references to serialized memory.

BTW, the patch does not apply cleanly to current trunk nor to current tip of 2.4 branch

[lucasvr]

Hi! Please find attached an updated patch against svn-trunk from yesterday. I've removed the extra calls to PG_FREE_IF_COPY prior to the calls to elog(ERROR), per your advice. Thanks!

[strk]

The POSTGIS2SFCGALGeometry might be making a deep copy, in which case you don't need to postpone the PG_FREE_IF_COPY if its inputs, can you check ?

The final lines instead (postponing freeing inputs to lwgeom_from_gserialized) are really needed.

[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.

[lucasvr]

Final patch. Fixes invalid memory access on sfcgal_make_solid() and harmonizes the use of TAB/spaces on sfcgal_is_solid().

[strk]

We're almost there :) in sfcgal_make_solid, the elog line will exit the function, so adding a PG_FREE_IF_COPY there (before elog ERROR) might be advisable.

Just in case this call is coming from some function handling the exception and continuing keeping the memory context alive for longer than expected...

Finally please give me your full name and email address for crediting you in the commit log :)

[robe]

This looks close to being done, can it be finished off by coming Thursday?

[robe]

Sorry lost your chance, this has to wait for 2.4.5

[pramsey]

In 16563:

Delay freeing input until processing complete. From lucasvr.
References #3980

[pramsey]

In 16564:

Delay freeing input until processing is complete. From lucasvr.
Closes #3980