Do what the title says (I had difficulty understanding this code, so not sure whether this works).
Details
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- fix-out-of-bounds-profiler2
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 737 Build 1169: Vulcan Build Jenkins Build 1168: arc lint + arc unit
Event Timeline
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/502/ for more details.
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/503/ for more details.
I don't believe this is correct, since most likely when you only have a spike buffer then the profiling will never be recorded (unless I'm misreading this?)
Can you explain what the issue with the code is?
The problem is that when the HoldLevel() == 0, in HoldMessages() the index into the m_HoldBuffers[8] (which is HoldLevel() - 1) is out of bounds. But like I said, I don't know how this code is supposed to work so I expected the HoldLevel() < 8 to be a bounds check, is this wrong?
Ah, ok, I see. You're right that there's a problem. The < 8 thing is indeed a bounds check.
Your fix is still incorrect however, I believe it should be
if (g_Profiler2.HoldLevel() == 0 || (g_Profiler2.HoldLevel() <= 8 && g_Profiler2.HoldType() != CProfiler2::ThreadStorage::BUFFER_AGGREGATE)) g_Profiler2.HoldMessages(CProfiler2::ThreadStorage::BUFFER_AGGREGATE); else m_PushedHold = false;
The < 8 thing makes it so that if you go beyond 8 levels of "conditional buffers", it just gets added to the innermost buffer iirc instead of scrapping the data entirely, so for 0 hold level we need it to always call "HoldMessages".
With the suggested piece of code, HoldMessages() is still called when m_HeldDepth == 0 resulting in an out of bounds access...
@echotangoecho : abandoning this for inactivity so it gets removed from the review queue, feel free to rebase and reopen.