Skip to content

Commit

Permalink
SortedNodeStore::publishGroup: out of bounds read
Browse files Browse the repository at this point in the history
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, 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.
  • Loading branch information
cldellow committed Feb 10, 2024
1 parent c630400 commit 491ef83
Showing 1 changed file with 5 additions and 3 deletions.
8 changes: 5 additions & 3 deletions src/sorted_node_store.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -423,9 +423,11 @@ void SortedNodeStore::publishGroup(const std::vector<element_t>& nodes) {
lastChunk = currentChunk;
}

tmpLatpLons[currentNodeIndex] = nodes[i].second.latp;
tmpLatpLons[currentNodeIndex + 256] = nodes[i].second.lon;
currentNodeIndex++;
if (i != nodes.size()) {
tmpLatpLons[currentNodeIndex] = nodes[i].second.latp;
tmpLatpLons[currentNodeIndex + 256] = nodes[i].second.lon;
currentNodeIndex++;
}
}

uint64_t chunks = currentChunkIndex;
Expand Down

0 comments on commit 491ef83

Please sign in to comment.