search

systemed / tilemaker

Make OpenStreetMap vector tiles without the stack
https://tilemaker.org/
Other
1.4k stars 226 forks source link

SortedNodeStore::publishGroup: out of bounds read #677

Closed cldellow closed 5 months ago

cldellow commented 5 months ago

Fixes the issued identified by @freeExec in #661

This code is a bit gross. It intentionally loops 1 past the size of the vector to ensure that we "publish" all the chunks.

There's a similar loop at lines 511-602.

In both loops, there's sort of three chunks of code, which I'll call "pre", "body" and "post". The pre/post sections need to be guarded, and the guard for the first loop's "post" section was missing.

Looking at it with fresh eyes, I think we could extract the "body" parts of both loops to their own functions, change the loop to use < instead of <= and then just call the body function again after the loop ends if it's needed. For now, I'm inclined to let a sleeping dog lie, but if more work happens in here, that'd be a good cleanup step.

This was also visible in valgrind, as it turns out--the error goes away with this change:

==1968107== Invalid read of size 4
==1968107==    at 0x33ABCD: SortedNodeStore::publishGroup(std::vector<std::pair<unsigned long, LatpLon>, std::allocator<std::pair<unsigned long, LatpLon> > > const&) (in /usr/local/bin/tilemaker)
==1968107==    by 0x33B715: SortedNodeStore::finalize(unsigned long) (in /usr/local/bin/tilemaker)
==1968107==    by 0x2FEE77: PbfProcessor::ReadPbfFile(unsigned int, bool, SignificantTags const&, SignificantTags const&, unsigned int, std::function<std::shared_ptr<std::istream> ()> const&, std::function<std::shared_ptr<OsmLuaProcessing> ()> const&, NodeStore const&, WayStore const&) (in /usr/local/bin/tilemaker)
==1968107==    by 0x140801: main (in /usr/local/bin/tilemaker)

SortedWayStore has a similar publishGroup function, but its internal implementation is different and it doesn't have this issue.

cldellow commented 5 months ago

I see that https://github.com/systemed/tilemaker/commit/c63040069824065a20f236524a0d53fbe9fccb27 failed with a segfault, looks like it was during node reading: https://github.com/systemed/tilemaker/actions/runs/7857409309/job/21441273093

It's possible that was this bug.

systemed commented 5 months ago

Thanks!

For now, I'm inclined to let a sleeping dog lie

🐶💤