From 491ef83fc1970d47cd236ba4895d9af18765d6be Mon Sep 17 00:00:00 2001 From: Colin Dellow Date: Sat, 10 Feb 2024 16:32:14 -0500 Subject: [PATCH] SortedNodeStore::publishGroup: out of bounds read 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::allocator > > 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 ()> const&, std::function ()> 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. --- src/sorted_node_store.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/sorted_node_store.cpp b/src/sorted_node_store.cpp index 82dccb55..0e0c7a8a 100644 --- a/src/sorted_node_store.cpp +++ b/src/sorted_node_store.cpp @@ -423,9 +423,11 @@ void SortedNodeStore::publishGroup(const std::vector& 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;