This has sat here for a long time pending careful review, because the logic is
not easy to follow. Fortunately I think I understand it now.
The described race is pretty much accurate. When a thread called
RtlExitUserThread() in 1.7.38, it first decremented nb_threads. If it was the
last thread, it would call exit() with the thread exit status; if not, it would
mask off signals [the order here is important] and then call pthread_exit() with
the status.
When a thread called RtlExitUserProcess(), this happened:
* The caller does a terminate_process() request to the server, which sends
SIGQUIT to every thread *but* the caller.
* The SIGQUIT handler calls terminate_thread() with a zero status.
terminate_thread() masks off signals, then decrements nb_threads. If the
aborting thread is the last thread, it would call _exit(), otherwise, it'd
again just pthread_exit().
* Finally, the original thread would call exit(), with the intended status
code.
[All of the intermediate function calls and helpers are skipped for brevity and
clarity].
The problem happens if both of these happen at the same time in different
threads. In this case the RtlExitUserThread() thread could decrement nb_threads,
then get interrupted by SIGQUIT and decrement nb_threads again. The end result
is that, instead of the RtlExitUserProcess() thread exiting with the intended
status, instead one of the SIGQUIT threads will be the "last" thread, and exit
with the status that SIGQUIT uses, which is zero as described.
A more serious race than this can be constructed if a thread is terminated by
another thread while already exiting. In this case nb_threads would be executed
twice, but the consequence would be that the *penultimate* thread to exit,
later, would end up killing the process, since it thinks it's the ultimate
thread.
2334f4e64582a518e4d5a7627472a0d817b147ef changed this. Now a thread calling
RtlExitUserThread() does not decrement nb_threads, but instead asks the server
if it's the last thread, and if so exit the whole process [at the time via
exit(); later via RtlExitUserProcess().] If not the last thread, the threads
mask off signals and then call pthread_exit() as before.
This avoided the race, but added a different one, essentially the opposite
problem: if two threads exit cleanly at the same time, neither one of them will
think they're the last thread, then both will exit without calling exit().
Apparently (from IRC logs) this would leave the thread in a weird state where
it'd still be running somehow, although it's not really clear how.
In any case, this problem was fixed by fac1aabbef3753afc53a4ea4f933b3d0516fd302
upstream. Now if two threads call NtTerminateThread() on themselves at the same
time, they really will exit cleanly and one will terminate the process.
Critically, this is now safe from the original race, because decrementing
nb_threads is done after masking off signals.
Erich can't remember the purpose of these, and suspects that it was incidental
to the other patches for TransmitFile (which used to be in the same Wine-Staging
patch set); those were for WineHQ bug 5048.
Moreover, these aren't correct, and a correct implementation would take a lot of
work, and wouldn't really be able to use anything from these patches as a
reference.
Hence, they're not providing any value to anyone, so remove them.
This goes through a buffer in advapi32, so it's not obviously visible to the application; however, it means that a call to getrandom() will return EFAULT and won't actually fill the buffer.
This was implemented upstream in e.g. 52e09e823e2a2b65f1f993145c0a65ab1d84713d and 926dd780697852b375ec0a193c762e7723a3f5e6, albeit in ntdll instead of mscoree.
This patch concerns the case where we successfully create a read-only directory
with FILE_DELETE_ON_CLOSE. Currently we return STATUS_CANNOT_DELETE (having
cleared O_CREAT). This patch would change that to return success.
However, tests show that this is incorrect. Actually FILE_DELETE_ON_CLOSE should
always fail on a read-only file, including when that file is created by the same
call.
These were committed without any explanation, but they do seem to fix Cygwin
unlink(), and it's been observed that many patches in Wine-Staging, whether
labeled or not, were specifically for Cygwin.
The relevant functionality was fixed a different way upstream, namely:
91e442b060
There is no record at all of what this was for.
However, it's not hard to make a good guess. The effect of the patch is to skip
walking the TEB chain if the faulting %esp is *above* the first element in the
TEB chain.
The most obvious thing this protects against is the case where the application
switched stacks and the new stack happened to be at a higher address. Without
this patch, we would walk through the whole TEB chain, since all of its entries
would be below the target frame we are unwinding to. But they wouldn't actually
be "inner to" it, and so we'd incorrectly hit Wine try/catch blocks.
The most notable such try/catch block is the unhandled exception filter itself,
and it would necessarily have been triggered by any such exception if no other
blocks were.
One can further speculate that this patch, like many others in Wine-Staging, was
written for Cygwin, which is known to switch stacks.
Besides Wine commits c22aa54e9977785eafcd7cc3811116e5f4dd2da8, and other more
targeted workarounds to specific functions, the workaround introduced by this
patch was obviated by a similar, but more complete and holistic, workaround
upstream, namely 8fe95d29d32533e8fa28383c0211555eb71ea6c1.
Thus this patch has been, in almost the simplest sense, upstreamed. Remove it.
This seems to be another one of those cases where Sebastian added a patch for an
application [1], then never submitted the patch upstream, and then someone else
submitted a different patch upstream [2], but Sebastian didn't bother checking
if his version was actually still necessary, and instead assumed it was (or just
decided it was better) and rebased the patch [3], and of course still never
submitted it upstream.
In this case the patch is for Terragen 4. That application does not, according
to my testing, actually depend on success from SetThreadIdealProcessorEx().
The patch does not add a correct implementation, is trivial to recreate, and is
easy to re-debug (the function still prints a FIXME), so it's not adding any
value. Remove it.
[1] a12dca03ce
[2] 980754bff7
[3] 0c46d1e8a2
Despite Michael Müller's claim that all patches in wine-staging actually fix
something [1], I've come across several patch sets over the years that seem to
be related to some contemporaneous work but don't actually fix any application
themselves (e.g. wine-staging commits 5d8901ac21, ba9a7a6a74, probably most of
e353590528; I think there are plenty of other examples as well.)
This patch appears to fall into this category. The upstream commit it was
written in response to was bc68b30d20.
The application in question is buggy. It uses OpenFile(), but compares the
return value to 0 instead of -1. The open in question is the first in the
program's run. The problem occurs if the DOS handles are unassigned, in which
case the valid handle 0 will be returned, and the program will interpret it as
failure, hit some broken code path, and crash.
bc68b30d20 fixes this by ensuring that the DOS standard handles are always
valid, and therefore OpenFile() will always return at least 5. This seems to
match what happens on Windows. I can reproduce this fix; I didn't go to the
trouble of building its parent, but reverting that patch in current Wine does
make the program crash the exact same way (comparing to the +relay log helpfully
provided in the bug report).
Sebastian probably saw this commit, thought that "well, there's multiple ways
for a handle to be invalid", wrote this patch catching the additional ones, and
for some reason never submitted it upstream.
Thing is, these handles come from the server, and they're guaranteed to be
either valid or zero. As evidence cf. the duplicate_handle() calls in the
new_process request handler, which were present even at the time. Hence this
patch isn't doing anything, so remove it.
[1] https://www.winehq.org/mailman3/hyperkitty/list/wine-devel@winehq.org/message/YGKVQN2N537MXAVSMLHX5IV4XCEWKBVY/
The tests are left alone since they aren't particularly important and will need to be rewritten anyway.
They should probably be somewhere other than input.c, since they don't actually relate to input.
After upstream commit 62a979347a4dc1bd68f79b86397de85c016ec588
any TRACE() present on the way of virtual_init() will cause
a crash with WINEDEBUG present regardless of actual debug channels.
Avoid TRACE in virtual_init().
This has no real effect. It was probably written because it looks more correct, but it never actually matters: we only get a short read at EOF, and in that case we will break out at the next iteration anyway.
