Do what the title says (I had difficulty understanding this code, so not sure whether this works).
Details
Details
Diff Detail
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
Comment Actions
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.
Comment Actions
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.
Comment Actions
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?
Comment Actions
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?
Comment Actions
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".
Comment Actions
With the suggested piece of code, HoldMessages() is still called when m_HeldDepth == 0 resulting in an out of bounds access...
Comment Actions
@echotangoecho : abandoning this for inactivity so it gets removed from the review queue, feel free to rebase and reopen.