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.

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

Fix usage of TerminateThread() causing critical section corruption. #2314

Merged
merged 1 commit into from
Nov 20, 2019

Conversation

Jehan
Copy link

@Jehan Jehan commented Nov 20, 2019

So here is the deal: we have crashes in GIMP on Windows where a specific security software is used. We got in touch with the publisher (through a user of both GIMP and this software) and they told us the problem does not come from GIMP, but from OpenBlas (which is an indirect dependency of GIMP through SuiteSparse) with the technical explanation which I gave in the commit message.

They also told us a similar problem was discussed here: https://stackoverflow.com/questions/39635817/windows-10-specific-crash-on-call-leavecriticalsection

So for some reason, they don't want to discuss this publicly or whatever, which is why I am doing the intermediary and why I don't even cite their name or the software's name, which is stupid — I know — as contributing to Free Software is to their credit. But whatever apparently they want to keep their contribution quiet.


This patch was submitted to the GIMP project by a publisher wishing to
keep confidentiality (hence anonymously). I just pass along the patch.
Here is the patch explanation which came with:

First they remind us what Microsoft documentation says about
TerminateThread:

TerminateThread is a dangerous function that should only be used in
the most extreme cases. You should call TerminateThread only if you
know exactly what the target thread is doing, and you control all of
the code that the target thread could possibly be running at the time
of the termination.
(https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-terminatethread)

Then they say that 5 milliseconds time-out might not be long enough for
the thread to exit gracefully. They propose to set it to a much higher
value (for instance here 5 seconds).

And finally you should always check the return value of
WaitForSingleObject(). In particular you want to run TerminateThread()
only if WaitForSingleObject() failed, not on success case.

This patch was submitted to the GIMP project by a publisher wishing to
keep confidentiality (hence anonymously). I just pass along the patch.
Here is the patch explanation which came with:

First they remind us what Microsoft documentation says about
TerminateThread:
> TerminateThread is a dangerous function that should only be used in
> the most extreme cases. You should call TerminateThread only if you
> know exactly what the target thread is doing, and you control all of
> the code that the target thread could possibly be running at the time
> of the termination.
(https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-terminatethread)

Then they say that 5 milliseconds time-out might not be long enough for
the thread to exit gracefully. They propose to set it to a much higher
value (for instance here 5 seconds).

And finally you should always check the return value of
WaitForSingleObject(). In particular you want to run TerminateThread()
only if WaitForSingleObject() failed, not on success case.
@martin-frbg
Copy link
Collaborator

Interesting, this must have been fun to debug. The code in question seems to be mostly unchanged from GotoBLAS so may have appeared to do the right thing on XP or 7. Perhaps the earlier commit that changed the INFINITE wait to five milliseconds to solve a hang #307 was unknowingly papering over a side effect of this bug

@martin-frbg martin-frbg added this to the 0.3.8 milestone Nov 20, 2019
@Jehan
Copy link
Author

Jehan commented Nov 20, 2019

Perhaps the earlier commit that changed the INFINITE wait to five milliseconds to solve a hang #307

Yeah the developer who contributed this said ideally we should keep as infinite (as was in comment). But we all know that idealistic code doesn't exist, and infinite wait means potential blocking execution. So this person went for a longer time-out instead. Makes sense to me.

@Jehan
Copy link
Author

Jehan commented Nov 20, 2019

Anyway we hope this gets merged soon. This issue has quite a lot of people complaining about GIMP crashing. ;-(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants