Out of bound array access on emptyCoords3d about geos HOT 7 CLOSED

With branch prediction I'd guess the GEOS penalty vanishes into the overhead of the C API. We already have some similar idiot-proofing like checking for a null pointer on every C API call.

from geos.

Thanks for the detailed write-up. I can see both sides: (1) that it's really on the caller (QgsGeos::fromGeos) to be checking whether a geometry has any coordinates before reading them, and ; yet (2) GEOSCoordSeq_getXYZ_r returns an error code, so it should do a bounds-check instead of crashing.

I haven't checked to see why this is a new crash, or whether GEOS previously provided a coordinate from an empty geometry, but I can't think of a reason why it should have done so.

from geos.

I wonder how much more expensive checking on every access (fix in geos) is vs checking before starting to access (fix in qgis).

from geos.

GEOSCoordSeq_getXYZ_r returns an error code, so it should do a bounds-check instead of crashing.

Or the check could be done in FixedSizeCoordinateSequence. I don’t know if there are other kinds of sequences, but this one certainly can segfault.

I haven't checked to see why this is a new crash

FWIW, it wasn’t crashing with QGIS 3.4.15 and GEOS 3.8.0 (Debian testing).

from geos.

Or the check could be done in FixedSizeCoordinateSequence. I don’t know if there are other kinds of sequences, but this one certainly can segfault.

Whether bounds checking is performed or not should not depend on the type of coordinate sequence. The more I think about it, the less I see the desirability. Should we also check that one does not access a subgeometry that does not exist? At some point, yes, you can expect compiled code to crash when you try to access elements out of bounds. That's not really unique to GEOS.

from geos.

Whether bounds checking is performed or not should not depend on the type of coordinate sequence.

I meant that FixedSizeCoordinateSequence uses a std::array internally and relies on operator[] which does no bound checking. “Crashing” is the way std::array::operator[] works, while throwing an exception could be part of CoordinateSequence::getAt()’s interface.

Should we also check that one does not access a subgeometry that does not exist?

Is std::vector (this one) an implementation detail or GeometryCollection’s interface?

Both cases boil down to a design decision: GEOS should either validate (more) input data (https://git.osgeo.org/gitea/geos/geos/pulls/103) or, well, it would be nice to at least document the current behavior, though I’m afraid it wouldn’t prevent this crash…

At some point, yes, you can expect compiled code to crash when you try to access elements out of bounds.

Actually, according to https://www.cplusplus.com/reference/vector/vector/operator[]/, std::vector::operator[] causes undefined behavior, so maybe it won’t crash when it should. Throwing an exception would probably be safer. I don’t know C++ well enough.

from geos.

For reference- an explicit point count check was added to QGIS in qgis/QGIS@774f1db . This can be closed now if the geos project deems that an automatic check at the geos level is too costly.

from geos.