SortedNodeStore::publishGroup: out of bounds read #677
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes the issued identified by @freeExec in systemed#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, and then just call them again after the loop ends. 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.
|
I see that c630400 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. |
|
Thanks!
🐶💤 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
SortedWayStore has a similar publishGroup function, but its internal implementation is different and it doesn't have this issue.