3 ms·

Immediately guessed the culprit after seeing the stack trace, because (network) I/O was occurring. Windows overlapped (=async) I/O to a stack allocated struct

Immediately guessed the culprit after seeing the stack trace, because (network) I/O was occurring. Windows overlapped (=async) I/O to a stack allocated struct is pretty certain to blow up just like that, sooner or later. If you guard for it, worse, since canceling is asynchronous as well, at least in theory you could end up in a scenario you just can't reliably ever let the execution to continue again. Luckily this should be extremely rare... I hope. Much better idea: allocate async data structures on the heap. In the worst case you leak some memory. Just never deallocate until the I/O has actually completed. Variation of the same can happen on the hardware level as well. Don't DMA into stack, unless you can really be sure it'll complete fast enough or you can afford to wait for it.

spockz 5y ago
Would allocating on the heap have been better than the work around that they used now? I suppose there is something to say for having the same mechanism on multiple platforms.
- Jare 5y ago
  I suspect it would have corrupted the heap instead of the stack. If so, this would be even harder to diagnose, because the corruption would show up elsewhere unrelated to the actual problem. The underlying problem is that you 1) queue an asynchronous callback and reserve some memory for it to use 2) wait (or so you think) for the function that calls the callback to be done and return 3) something interrupts the wait, but the callback is still queued for execution sometime in the future. 4) since the wait is over, you assume the callback was called, and the memory it was going to use can be safely freed 5) if the memory was on the stack, it gets freed purely by returning. if it was in the heap, you'd deallocate it. 6) the callback that was still queued fires up and writes to memory it no longer should be able to use. To really fix this situation, you'd need to free the memory in the callback. This is not possible in this case unless you can rewrite the library select() function significantly, perhaps even rewriting other parts (not familiar enough with those APIs to be really sure). Even in similar situations where it is possible (e.g. if you have garbage collection), it will often turn a memory corruption into a logical corruption, because you have a callback fired related to something that is not relevant anymore, and any side effects of that callback will happen without the proper context. (in this case the only side effect is probably the memory access, so in this particular instance the problem would be "fixed") [Edit: if the heap allocation was exception-aware (e.g. RAII that gets called during exception unwinding, then you would get heap corruption. If it was manual deallocation and thus skipped during exception unwind, then indeed you would just get a memory leak exactly as you said]
  - vardump 5y ago
    Somehow the lifetime must be at least as long as the async operation takes to complete. All other scenarios could result memory corruption. If there's no other option, I'd rather leak that memory.
quietbritishjim 5y ago
Hmm, you immediately knew that async IO was the culprit... but it wasn't, according to the article. The function that called the async IO (which was a Windows API function, not in Unity) was correctly waiting until the IO completed before releasing the struct (by returning). The actual problem was that the rug was being whipped from under it by an exception being injected into a place where it ought to have been impossible. Your suggestion of using heap allocation instead wouldn't have fixed anything; it would've turned the crash into a memory leak, which would've just masked the the problem. The solution they went for in the end addressed this real issue, and the async IO (happening under the hood of that Windows API function) remains unchanged.
- vardump 5y ago
  Well, overlapped (=async) IO was what ultimately overwrote stack memory. The hazard pattern I recognized from previous experience. Of course exception in the queued APC was the reason why select's WFSO was prematurely interrupted and stack rewound, so the bug was of course there. It's pretty dangerous to execute code in the context of a waiting thread. While the solution to fix it by using heap allocation is a bit ugly in some sense, if your choices are to abort execution completely or to infrequently leak some bytes of memory, I know I'd choose to leak and perhaps log what just happened. There are rare cases when the I/O never completes and CancelIo doesn't complete either. Probably hardware faults or kernel driver bugs. Stuff that should absolutely never happen, but can still be defensively programmed around.
deleted 5y ago
[deleted]

spockz 5y ago

Would allocating on the heap have been better than the work around that they used now? I suppose there is something to say for having the same mechanism on multiple platforms.

Jare 5y ago
I suspect it would have corrupted the heap instead of the stack. If so, this would be even harder to diagnose, because the corruption would show up elsewhere unrelated to the actual problem. The underlying problem is that you 1) queue an asynchronous callback and reserve some memory for it to use 2) wait (or so you think) for the function that calls the callback to be done and return 3) something interrupts the wait, but the callback is still queued for execution sometime in the future. 4) since the wait is over, you assume the callback was called, and the memory it was going to use can be safely freed 5) if the memory was on the stack, it gets freed purely by returning. if it was in the heap, you'd deallocate it. 6) the callback that was still queued fires up and writes to memory it no longer should be able to use. To really fix this situation, you'd need to free the memory in the callback. This is not possible in this case unless you can rewrite the library select() function significantly, perhaps even rewriting other parts (not familiar enough with those APIs to be really sure). Even in similar situations where it is possible (e.g. if you have garbage collection), it will often turn a memory corruption into a logical corruption, because you have a callback fired related to something that is not relevant anymore, and any side effects of that callback will happen without the proper context. (in this case the only side effect is probably the memory access, so in this particular instance the problem would be "fixed") [Edit: if the heap allocation was exception-aware (e.g. RAII that gets called during exception unwinding, then you would get heap corruption. If it was manual deallocation and thus skipped during exception unwind, then indeed you would just get a memory leak exactly as you said]
- vardump 5y ago
  Somehow the lifetime must be at least as long as the async operation takes to complete. All other scenarios could result memory corruption. If there's no other option, I'd rather leak that memory.

quietbritishjim 5y ago

Hmm, you immediately knew that async IO was the culprit... but it wasn't, according to the article. The function that called the async IO (which was a Windows API function, not in Unity) was correctly waiting until the IO completed before releasing the struct (by returning). The actual problem was that the rug was being whipped from under it by an exception being injected into a place where it ought to have been impossible. Your suggestion of using heap allocation instead wouldn't have fixed anything; it would've turned the crash into a memory leak, which would've just masked the the problem. The solution they went for in the end addressed this real issue, and the async IO (happening under the hood of that Windows API function) remains unchanged.

vardump 5y ago
Well, overlapped (=async) IO was what ultimately overwrote stack memory. The hazard pattern I recognized from previous experience. Of course exception in the queued APC was the reason why select's WFSO was prematurely interrupted and stack rewound, so the bug was of course there. It's pretty dangerous to execute code in the context of a waiting thread. While the solution to fix it by using heap allocation is a bit ugly in some sense, if your choices are to abort execution completely or to infrequently leak some bytes of memory, I know I'd choose to leak and perhaps log what just happened. There are rare cases when the I/O never completes and CancelIo doesn't complete either. Probably hardware faults or kernel driver bugs. Stuff that should absolutely never happen, but can still be defensively programmed around.

deleted 5y ago

[deleted]