Fix conditional breakpoints being triggered unconditionally during stepping and hijacking the step #21
Conversation
- When issuing a step command, remember its thread ID so that other threads cannot hijack the callback and trigger their breakpoints.
| @@ -1237,7 +1237,7 @@ __declspec(dllexport) void TITCALL DebugLoop() | |||
| //general unhandled exception callback | |||
| if(DBGCode == DBG_EXCEPTION_NOT_HANDLED) | |||
| { | |||
| engineStepActive = false; | |||
| engineStepTID = 0; | |||
There was a problem hiding this comment.
Maybe we should only clear the field if the unhandled exception comes from the same thread that's awaiting the step to complete?
ynwarcs
changed the title
|
While this is probably an improvement, I think the proper solution is to track the (internal) stepping state on a per-thread basis. This is how GleeBug implements this and I did not observe the same bug with TitanEngine. Similar bugs exist with hardware breakpoints, because the next debug event can always come from a different thread. I don't remember from the top of my head, but I think you can trigger a similar bug by just stepping around near a |
|
You're right, I hadn't really considered other stuff that could hijack the step. After having a brief look, I see that GleeBug's implementation seems quite more sophisticated than Titan's, so I assume Titan would have to undergo similar changes to cover all edge cases. Do you expect that it'd take a lot of effort to port the implementation or write an alternate one from scratch here? Is it something you'd be open to accepting, seeing as it would be a serious risk to breaking existing behaviours that other folks may be relying on? As for my personal woes with this bug, I can use my forked version with this fix + some other junk for the time being, as it works pretty well for my needs. But I did notice some other bugs that happen from time to time, that could potentially be fixed with a rework of stepping implementation. Alternatively, I could give GleeBug a go but I'm not familiar with it and I don't see any documentation. Can it be used with x64dbg, e.g. as a drop-in replacement for titan, or is it meant to serve a different purpose? I also just flagged the same bug in WinDBG (microsoftfeedback/WinDbg-Feedback#233), and seeing as it's been present forever, I guess not many folks are affected by it and it doesn't need to be fixed asap. |
|
You can switch to GleeBug in the x64dbg settings. I also concluded that it is a lot of work to backport the fixed between TitanEngine and GleeBug, so my goal eventually is to just turn on GleeBug per default 🤷🏻♂️ The main issue right now is there are no proper tests to make sure GleeBug and TitanEngine work the same, which is kind of annoying. Scenarios like this also need to be tested and it's just a lot of work. Eventually I'll spend some time on it, I have a minimal project here that can be good for testing https://github.com/mrexodia/DebugLoopRace/tree/main |
This should fix #20.
The fix is pretty straightforward but there's still room for improvement: