Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

One Crash and my analysize #1863

Closed
lynnux opened this issue Jan 5, 2018 · 640 comments
Closed

One Crash and my analysize #1863

lynnux opened this issue Jan 5, 2018 · 640 comments

Comments

@lynnux
Copy link
Contributor

lynnux commented Jan 5, 2018

When CTRL+F2 to restart, x32dbg crashed. And I attached the crashed x32dbg, maybe found the problem:
Firstly check the crashed thread, the stack is:
1
the source code is:
default

Secondly, check all thread stack which may do stuff with static CRITICAL_SECTION criticalSection;, I found in the main thread:
default
and the source code is:
default
So, that's maybe the problem.
@lynnux
Copy link
Contributor Author

lynnux commented Jan 5, 2018

the more stack info in main thread:
default
and the crash place is in windows API:
default

@Nukem9
Copy link
Member

Nukem9 commented Jan 5, 2018

mrexodia: Tons of fun stuff here lol

capture1
capture2

I don't have valid k32/ntdll symbols because of the recent windows update.

@lynnux
Copy link
Contributor Author

lynnux commented Jan 5, 2018

Oh, maybe I'm wrong, I never used TryEnterCriticalSection, which implemented the WEAK CriticalSection. But I've noticed there are two functions: SafeDbghelpInitialize SafeDbghelpDeinitialize, they are be called when debug session starting and stopping, but the static CRITICAL_SECTION criticalSection; are global, is it really safe?

@lynnux
Copy link
Contributor Author

lynnux commented Jan 5, 2018

After dig into this one or two hours, I really don't know where are wrong, I tried this code:
default
but still got crash, but I think it truelly relate to CRITICAL_SECTION, and there are still got SafeSymFromAddr(in main thread) and SafeSymLoadModuleExW in those crashs.

@mrexodia
Copy link
Member

mrexodia commented Jan 5, 2018

I think #1820 will solve this issue...

@Mattiwatti
Copy link
Member

The RTL_CRITICAL_SECTION being a global is not an issue in this case; it's not deleted until the process exits. So that part is OK.

What's strange to me however is the STRONG_ACQUIRE/WEAK_ACQUIRE divide. It seems to me that if you need a critical section for thread safety (definitely the case with dbghelp) that you always want STRONG_ACQUIRE. WEAK_ACQUIRE to me is like saying 'it would be nice if I could have this mutex, but if not, that's fine too'. In that case you might as well not attempt synchronization in the first place since there are no guarantees this way.

I spent some time debugging this and the crash conditions were always as follows:

  • The main thread is holding a weak lock for SafeSymFromAddr and is in its destructor at ntdll!RtlLeaveCriticalSection -> ntdll!RtlpUnWaitCriticalSection
  • The threadDebugLoop thread is simultaneously trying to obtain a strong lock for SafeSymLoadModuleExW and is in its ctor at ntdll!RtlEnterCriticalSection -> ntdll!RtlpWaitCriticalSection.

So this is exactly the same as in the OP. Do note SafeSymFromAddr is by far the most frequently called 'weak acquire' function, so it's possible that if you try this long enough you'll see other functions too.

The above in itself shouldn't be an issue AFAICT. The threadDebugLoop thread should simply block until the main thread is done releasing the critical section. And this seems to be supported by the fact that the crash doesn't happen on x64 (at least on my machine). So the kinda annoying conclusion is that I have no idea WTF is really going on. Nevertheless the following change fixes the issue completely for me:

BOOL
SafeSymFromAddr(
    __in HANDLE hProcess,
    __in DWORD64 Address,
    __out_opt PDWORD64 Displacement,
    __inout PSYMBOL_INFO Symbol
)
{
    -WEAK_ACQUIRE();
    +STRONG_ACQUIRE();
    return SymFromAddr(hProcess, Address, Displacement, Symbol);
}

I can similarly replace all WEAK_ACQUIREs with guaranteed ones with no ill effect. The commit that added these macros mentions fixing a freeze on attach, but I can't reproduce that. A deadlock in a critical section is really a pathological case IMO since they can be recursively acquired. You typically need to do something pretty bad like terminate a thread while it's holding a critical section to manage to get a deadlock.

@Mattiwatti Mattiwatti mentioned this issue Jan 7, 2018
Closed
@mrexodia
Copy link
Member

mrexodia commented Jan 7, 2018 

#define WEAK_ACQUIRE() Lock __lock(true); if(!__lock.success) return 0;

it would be nice if I could have this mutex, but if not, that's fine too.

That is indeed the idea, if the critical section cannot be acquired the function will fail (eg the symbol for that address will not be displayed). This mostly happens during module loading, but if reverting this indeed fixes this issue I think it's good to do before #1820 is merged (since the whole of dbghelp will be removed with that).

@Mattiwatti
Copy link
Member

Oh yeah, I forgot about the return 0 part. With WEAK_ACQUIRE the function simply returns silently, which the calling code is built to handle. So there's nothing really wrong with it per se, but it does introduce nondeterminism.

My best guess is SymFromAddr having some internal side effect and it being (not) called can somehow affect dbghelp behaviour later on. Only on 32 bit of course. Good old dbghelp.

@x64dbg x64dbg deleted a comment from surajd13 Jan 7, 2018
@x64dbg x64dbg deleted a comment from aacos24653 Jan 7, 2018
@x64dbg x64dbg deleted a comment from CripleMike Jan 12, 2018
@x64dbg x64dbg deleted a comment from openzilla Jan 12, 2018
@x64dbg x64dbg deleted a comment from clyall Jan 12, 2018
@mrexodia
Copy link
Member

@lynnux could you test with this fix?

@pksmith
Copy link

pksmith commented Jan 12, 2018 via email

@Phkrauss
Copy link

Phkrauss commented Jan 12, 2018 via email

@Maakat
Copy link

Maakat commented Jan 12, 2018 via email

@0st3n
Copy link

0st3n commented Jan 12, 2018 via email

@kuta1069
Copy link

kuta1069 commented Jan 12, 2018 via email

@luistong
Copy link

luistong commented Jan 12, 2018 via email

@tcollins590
Copy link

tcollins590 commented Jan 12, 2018 via email

@sevillanoj
Copy link

sevillanoj commented Jan 12, 2018 via email

@StudentBlake
Copy link

StudentBlake commented Jan 12, 2018 via email

@rdias23
Copy link

rdias23 commented Jan 12, 2018

Unsubscribe

@Terbla
Copy link

Terbla commented Jan 12, 2018 via email

@0fin
Copy link

0fin commented Jan 12, 2018 via email

@lukedg
Copy link

lukedg commented Jan 12, 2018 via email

@ghost
Copy link

ghost commented Jan 12, 2018

Unsubscribe!!

@Tomino1
Copy link

Tomino1 commented Jan 14, 2018 via email

@ghost
Copy link

ghost commented Jan 14, 2018 via email

@ShadowMuffin512
Copy link

ShadowMuffin512 commented Jan 14, 2018 via email

@Lokiamus
Copy link

Unsubscribe

@wesleybpereira
Copy link

wesleybpereira commented Jan 14, 2018 via email

@chiangm
Copy link

chiangm commented Jan 14, 2018 via email

@yigitsk
Copy link

yigitsk commented Jan 14, 2018 via email

@vedranius
Copy link

vedranius commented Jan 14, 2018 via email

@SA-Code
Copy link

SA-Code commented Jan 14, 2018 via email

@MastaKillaGamer
Copy link

MastaKillaGamer commented Jan 14, 2018 via email

@apatrick662
Copy link

Unsubscribe

@apine001
Copy link

apine001 commented Jan 14, 2018 via email

@InitiallyNothing
Copy link

InitiallyNothing commented Jan 15, 2018 via email

@nbgucer
Copy link

nbgucer commented Jan 15, 2018

Unsubscribe

@vta92
Copy link

vta92 commented Jan 15, 2018 via email

@dawja5215
Copy link

dawja5215 commented Jan 15, 2018 via email

@vta92
Copy link

vta92 commented Jan 15, 2018 via email

@Questo
Copy link

Questo commented Jan 15, 2018 via email

@oleg-g1t
Copy link

oleg-g1t commented Jan 15, 2018 via email

@Emhyrus
Copy link

Emhyrus commented Jan 15, 2018 via email

@mrjazzz
Copy link

mrjazzz commented Jan 15, 2018 via email

@fcenteno
Copy link

fcenteno commented Jan 15, 2018 via email

@michaelmarino
Copy link

michaelmarino commented Jan 15, 2018 via email

@kir486680
Copy link

kir486680 commented Jan 15, 2018 via email

@Vist3
Copy link

Vist3 commented Jan 15, 2018 via email

@ghost
Copy link

ghost commented Jan 15, 2018 via email

@eodell001
Copy link

eodell001 commented Jan 15, 2018 via email

@ghost ghost locked and limited conversation to collaborators Jan 15, 2018
@mrexodia
Copy link
Member

Everybody who is still subscribed to this and doesn't know why: change your password. You will not see anything in your audit log, but your credentials were used in a Github API call to subscribe to this repo.

mrexodia added a commit that referenced this issue Mar 4, 2018
@mrexodia
DBG: fix rare handle corruption in TitanEngine
1a99e09 
closes #1863
closes #1759
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests