Created attachment 1473
fix "use after free" in async.c
async file reader (windows version at least) has a nasty bug: it uses `AsyncRead` structure after `free()`. this is what makes dmd.exe crash under my wine.
i found that by inserting `printf`s everywhere. then i saw this:
..\dmd\src\dmd.exe -c -o- -Isrc -Iimport -Hfimport\core\sync\barrier.di src\core\sync\barrier.d
AsyncRead::create: aw = 00242754; max=1
*** addFile(file = 00262510)
filesdim = 0, filesmax = 1
addFile done: (f = 00242760; file = 00262510; result=0; event=0000003C)
*** ::start aw->filesdim = 00242754 1
*** AsyncRead::read 000 i=0 (f=00242760; file = 00262510; event = 0000003C)
*** aw->filesdim = 00242754 1
++ aw->filesdim = 00242754 1 i=0
000: i=0; f=00242760; file=2499856; event=0000003C
001: i=0; f=00242760; file=2499856; event=0000003C
AsyncRead::read 001 i=0 (f=00242760; file = 00262510; event = 0000003C; res=0)
AsyncRead::read 002 i=0 (f=00242760; file = 00262510; event = 0000003C; res=0)
AsyncRead::dispose; aw = 00242754
002: i=0; f=00242760; file=2499856; event=0000003C
startthread done: aw->filesdim = 00242754 1
the line "002: i=0; f=00242760; file=2499856; event=0000003C" is from `startthread`, it printed right after `SetEvent(f->event);` call. as you can see, `AsyncRead::dispose` was already called, yet `startthread` is still using `aw`.
seems that this is highly depends on timing, 'cause SOMETIMES (very rarely) dmd works ok in my wine. but most of the time it crashes. yet it works fine on real windows.
nevertheless, just commenting out `free(aw);` in `AsyncRead::dispose` fixes it all. as dmd.exe leaks memory as crazy anyway, there is no harm in leaking a little more, and it was the easiest fix.
posix version of this seems to be protected from the bug, as posix `AsyncRead::dispose` correctly waits while all operations on all files are complete. more than that, posix `startthread` correctly caches `aw->filesdim`, so it will not use freed memory in loop condition check.
so i attached a patch that waits for completion of all file operations before freeing `aw`. this seems to fix the issue, and this is what posix version do.
Comment #1 by ketmar — 2015-02-01T01:35:21Z
p.s. note that "printf log" was taken from the version with `free()` commented, that's why it looks OK. the original version was simply crashing there.
as i have no github account, i'll answer here:
WalterBright added a note Feb 1, 2015
Why this change?
'cause `AsyncRead::dispose` can `free()` aw before loop in `startthread` completes, and then `startthread` will try to reference already freed memory in loop condition check. this is very unlikely, but still possible situation. so we have to cache the value.
Comment #4 by petar.p.kirov — 2015-02-12T02:32:07Z
(In reply to Ketmar Dark from comment #3)
Now that `AsyncRead::dispose()` waits for all operations on all files to complete, it should not be possible to `free()` `aw`, before `startthread()` completes.
Even if this somehow happens accessing `aw->filesdim` or using a cached version of it is irrelevant, because inside the loop we access the `aw->files` member, which would also lead to the same problem.
Comment #5 by ketmar — 2015-02-12T09:11:32Z
it is possible.
1. executing WaitForSingleObject(f->event, INFINITE); in `dispose()`.
2. last `f->result = f->file->read();` fires event
3. time slice goes to `dispose()`, it finishes and frees `aw`.
4. time slice returns to `startthread` (we are still in loop, right after `f->file->read()`.
5. `startthread` executes `i < aw->filesdim` in `for`… oops.
Comment #6 by ketmar — 2015-02-12T09:14:10Z
yet it is perfectly safe to access `aw->files` there, 'cause we will *never* access it after it's last item processed.
Comment #7 by github-bugzilla — 2015-05-15T13:29:27Z