16 ms·
Debugging memory corruption: who the hell writes "2" into my stack? (2016)
- pantalaimon 2y ago> Jemand hatte das Gemächt meines Wächters angefasst - und es war definitiv kein Freund. That auto-translation is something else
- saagarjha 2y agoScary. I assume standard memory corruption detection tools would also have trouble finding this, as the write is coming from outside the application itself…
- loeg 2y agoYeah. Not tripping a page fault on modifying readonly (userspace) pages both makes it hard for userspace tools but also paints a pretty specific picture of where the write is coming from. I'm actually not sure if Linux would handle this in the same way or what. Plausibly it sees the same leaf page tables as user space, trips a fault, and doesn't scribble the pages anyway. Maybe Windows translates the user-provided virtual address to a physical address (or other kernel mapping that happens to have write permission) upon registration.
- rramadass 2y agoA Story for the ages. That is some hardcore debugging involving everything viz. user land, system call, kernel, disassembly etc.
- glandium 2y agoI wonder if Time Travel Debugging would have helped narrow it down.
- chris_wot 2y agoHow? Throwing an exception would prevent this wouldn’t it?
- ben-schaaf 2y agoWhen the assertion on the stack sentinel was reached they could have watched the value and then reverse continued, which in theory would reveal the APC causing the issue - or at least the instruction writing the value. Not sure how well reverse debugging works on Windows though, I'm only familiar with rr.
- loeg 2y agoI don't think any reverse debugging system can step the kernel backwards to this degree, unless they're doing something really clever (slow) with virtual machines and snapshots.
- dzaima 2y agoWhile not allowing stepping in the kernel, a large part of rr is indeed intercepting all things the kernel may do and re-implementing its actions, writing down all changes to memory & etc it does (of course for Linux, not Windows). With which the kernel doing an asynchronous write would necessarily end up as a part of the recording stating what the kernel writes at the given point in time, which a debugger could deterministically reason about. (of course this relies on the recording system reimplementing the things accurately enough, but that's at least possible)
- Veserv 2y agoYou are correct. A time travel debugging solution that supports recording the relevant system call side effects would handle this. In fact, this system call is likely just rewriting the program counter register and maybe a few others, so it would likely be very easy to support if you could hook the relevant kernel operations which may or may not be possible in Windows. The replay system would also be unlikely to pose a problem. Replay systems usually just encode and replay the side effects, so there is no need to "reimplement" the operations. So, if you did some wacky system call, but all it did is write 0x2 to a memory location, M, you effectively just record: "at time T we issued a system call that wrote 0x2 to M". Then, when you get to simulated time T in the replay, you do not reissue the wacky system call, you just write 0x2 to M and call it a day.
- loeg 2y agoThis system call returned and then asynchronously wrote to memory some time later. How does the replay system even know the write happened, without scanning all memory? It can't generally. With knowledge of the specific call it could put just that address on a to-be-scanned list to wait for completion, but it still needs to periodically poll the memory. It is far more complicated to record than a synchronous syscall.
- ryao 2y agoIt would have only helped if it were applied to the OS kernel.
- lionkor 2y agoVery wild bug. I feel like this is some kind of a worst-case "exceptions bad" lesson, but I've only been doing systems level programming for a couple of years so I'm probably talking out my ass.
- usrnm 2y agoI don't think that the lesson here is "exceptions are bad", the same kind of bug can be easily made without using exceptions.
- usrnm 2y agoAnother thing to note is that exactly the same bug can be made in Rust or go, both of which officially don't have exceptions. They both, of course, do have exceptions and just call them a different name.
- LegionMammal978 2y agoAs it happens, Rust 1.81 recently added a feature where it aborts by default when you attempt to unwind from a function with the "C" ABI [0], which mitigates this issue for most FFI use cases. Of course, it's not uncommon to find unsafe Rust code that misbehaves badly when something panics, which is yet another of its hazards that I wish were better documented. In this case, I'd put down the lesson as "exceptions in C++ are very dangerous if you're coming out of C code", since they can cause undocumented and unexpected behavior that ultimately led to the use-after-return issue here. [0] https://blog.rust-lang.org/2024/09/05/Rust-1.81.0.html#abort-on-uncaught-panics-in-extern-c-functions https://blog.rust-lang.org/2024/09/05/Rust-1.81.0.html#abort...
- ryao 2y agoWhat do you mean by "misbehaves badly when something panics"?
- LegionMammal978 2y ago
- mhogomchungu 2y agoRaymond Cheng faced a similar situation here: https://devblogs.microsoft.com/oldnewthing/20240927-00/?p=110320 https://devblogs.microsoft.com/oldnewthing/20240927-00/?p=11... The problem boils down to usage of stack memory after the memory is given to somebody else.
- musjleman 2y ago> The problem boils down to usage of stack memory after the memory is given to somebody else. While this isn't incorrect in this case the problem seems to be caused by stack unwinding without the consent of lower frames rather than a willful bug where the callee forgets about the ownership.
- layer8 2y agoYes, it’s the consequence of throwing exceptions through exception-unaware code, which is a problem when said code needs to perform some cleanup logic before returning, like releasing resources.
- bialpio 2y agoWDYM? The root cause is "you passed ownership to stack-based memory to the kernel and didn't ensure it's valid when it called you back", why would "consent of lower frames" matter here? Exceptions (where lower frames matter) hid the control flow here, but that's one way to reach this situation (early return is another way, as shown by Raymond Chen's post).
- musjleman 2y ago> WDYM? The root cause is "you passed ownership to stack-based memory to the kernel and didn't ensure it's valid when it called you back", why would "consent of lower frames" matter here? There is no "called back" in this case. The APC was executed by the sleep and corrupted the stack by unwinding across the C winsock code without any cleanup. It never returned. The user-mode enters an "alertable" wait which allows an asynchronous procedure (APC) to interrupt it and execute code. Instead of returning the APC causes an exception, unwinds the stack across the APC delivery and ends up executing some random code instead of returning to the winapi code that called wait(alertable: true) in a loop. So the code that was supposed to be synchronous because of while(!completed) wait(); suddenly is broken out of the loop without actually being completed. > Exceptions (where lower frames matter) hid the control flow here, but that's one way to reach this situation (early return is another way, as shown by Raymond Chen's post). This isn't just hiding the control flow here. It's control flow that shouldn't have existed in the first place. It walks across the boundary of the windows APC dispatcher. Unity folks needed to go out of their way to make this "work" in the first place because using c++ exceptions and standard library threads this wouldn't work.
- hun3 2y ago(2016)
- saagarjha 2y agoAdded
- mgaunard 2y agoSet a hardware breakpoint and you'll know immediately. That's what he eventually did, but he should have done so sooner. Then obviously, cancelling an operation is always tricky business with lifetime due to asynchronicity. My approach is to always design my APIs with synchronous cancel semantics, which is sometimes tricky to implement. Many common libraries don't do it right.
- saagarjha 2y agoHardware breakpoints don't work if the kernel is doing the writes, because the kernel won't let you enable them globally so they trigger outside of your program.
- machine_coffee 2y agoAlso surprised an async completion was writing to the stack. You should normally pass a heap buffer to these functions and keep it alive e.g for the lifetime of the object being watched.
- muststopmyths 2y agoIt's not an async completion. The call is synchronous. Windows allows some synchronous calls to be interrupted by another thread to run an APC if the called thread is in an "alertable wait" state. The interrupted thread then returns to the blocking call, so the pointers in the call are expected to be valid. Edit 2: I should clarify that the thread returns to the blocking call, which then exits with WAIT_IO_COMPLETION status. So you have to retry it again. but the stack context is expected to be safe. APC is an "Asynchronous procedure call", which is asynchronous to the calling thread in that it may or may not get run. Edit: May or may not run a future time. (https://learn.microsoft.com/en-us/windows/win32/sync/asynchronous-procedure-calls https://learn.microsoft.com/en-us/windows/win32/sync/asynchr...) There are very limited things you are supposed to do in an APC, but these are poorly documented and need one to think carefully about what is happening when a thread is executing in a stack frame and you interrupt it with this horrorshow. Win32 API is a plethora of footguns. For the uninitiated it can be like playing Minesweeper with code. Or like that scene in Galaxy Quest where the hammers are coming at you at random times as you try to cross a hallway. A lot of it was designed by people who, I think, would call one stupid for holding it wrong. I suppose it's a relic of the late 80s and 90s when you crawled on broken glass because there was no other way to get to the other side. You learn a lot of the underlying systems this way, but these days people need to get shit done and move on with their lives. Us olds are left behind staring at nostalgically at our mangled feet while we yell at people to get off our lawns.
- AshleysBrain 2y agoWould memory safe languages avoid these kinds of problems? It seems like a good example of a nightmare bug from memory corruption - 5 days to fix and the author alludes to it keeping them up at night is a pretty strong motivation to avoid memory unsafety IMO.
- saagarjha 2y agoNo*. This is one of the bugs that traditional memory safety would not fix, because the issue crosses privilege boundaries in a way that the language can't protect against. *This could, in theory, be caught by fancy hardware strategies like capabilities. But those are somewhat more esoteric.
- deleted 2y ago[deleted]
- alexvitkov 2y ago> The project was quite big (although far from the largest ones); it took 40 minutes to build on my machine. A bit tangential, but I've been crying about the insane Unity project build times for years now, and about how they've taken zero steps to fix them and are instead trying their hardest to sell you cloud builds. Glad to see them having to suffer through what they're inflicting on us for once! Regardless, very good writeup, and yet another reason to never ever under any conditions use exceptions.
- noitpmeder 2y agoWould a ccache or similar help alleviate the pain?
- deleted 2y ago[deleted]
- yard2010 2y agoThis poor human being doesn't deserve to pay the price for the shitty middle management actions though :(
- voidUpdate 2y agohey, at least its not unreal, where when you build it sits there compiling tens of thousands of shaders
- danaris 2y agoWSPSelect: 'Twas I who wrote "2" to your stack! And I would've gotten away with it, too, if it weren't for that meddling kernel debugger!
- greenbit 2y ago.. and their hardware breakpoint!
- bjornsing 2y agoBack in the day I used to consult for Sony Ericsson. We had like 5000 engineers writing C code that ran as a single executable in a single address space(!). Memory corruption was rampant. So rampant in fact that when we finally got an MMU it took months before we could turn it on in release builds, because there were so many memory corruption bugs even in the released product. The software just wouldn’t work unless it could overwrite memory here and there.
- farmdve 2y agoI remember back in the day of the Sony Satio U1, the last Symbian v5 phone, the software was horrendous(screen tearing, random OS freezes) and later, the phone abandoned. I think it was afterwards that Sony and Ericsson split?
- ryao 2y agoThe Linux kernel has thousands of engineers writing C code that runs as a single executable in a single address space. Memory corruption is not rampant, since they are careful to write good code, although it can still happen. I am going to guess that Sony Ericsson had a ton of use after free bugs and array out of bounds bugs, as that is the main way that developers can step on each others' code when it shares a process space. Speaking of which, I once did consulting for a company that insisted that they had hit a bug in ZFS because there were backtraces showing ZFS. Using KASAN, I found that the bug was in fact in another third party kernel module that they used that appeared to be the result of a hobbyist project on github. It had a string function write 1 past the end of a memory allocation, which often would corrupt memory allocations used in ZFS' AVL trees. ZFS' AVL tree code did not like this, so ZFS appeared in backtraces. That said, the bug described by the unity blog is really an undefined behavior bug rather than a memory corruption bug. Doing an exception in a C++ callback function called by a C function is undefined since C does not support stack unwinding. If the C function does not need any special cleanup when stack unwinding is done, it will probably work fine, but in this particular case, the function needed very special cleanup and invoking undefined behavior bit them.
- bjornsing 2y ago
- hrtk 2y agoI don’t know windows programming and this was a very interesting (nightmare-ish) post. I had a few questions I asked ChatGPT to understand better: https://chatgpt.com/share/677411f9-b8a0-8013-8724-8cdff8dc4d3c https://chatgpt.com/share/677411f9-b8a0-8013-8724-8cdff8dc4d... Very interesting insights about low level programming in general
- rramadass 2y agoThat's a pretty good use case for ChatGPT. Do you do this often? And if so, are your results specific to debugging consistently good?
- hrtk 2y agoYes, I complement GPT answers with Google search for original documentations just to be sure. Results do contain hallucinations at times but can be easily verified.
- dahart 2y agoIt’s been more than a decade since I worked in games, but for my entire games career, any and all use of C++ exceptions was strictly disallowed due to other horror stories. I was still bit in a very similar way by someone’s C++ copy constructor trickery - a crash that only happened in a release build after playing the game for a while, with a stack corruption. Like the author, for me this was one of the hardest bugs I ever had to track down, and I ended up writing a tiny release mode debugger that logged call stacks in order to do it. Once I was able to catch the corruption (after several days of debugging during a crunch weekend), someone on my team noticed the stomp values looked like floating point numbers, and pretty quickly we figured out it was coming from the matrix class trying to be too clever with it’s reference counting IIRC. There’d been a team of around a dozen people trying to track this down during overtime, so it suddenly hit me once we fixed it that someone’s cute idea that took maybe 10 seconds to write cost several tens of thousands of dollars to fix.
- ziml77 2y agoThis could still happen without exceptions though, right? The flow is more explicit without exceptions, but returning an error code up the stack would have the same effect of causing the memory that select() is referencing to possibly be used for a different purpose when it writes its result.
- mark_undoio 2y agoMy reading is that the problem was specifically that they were injecting an exception to recover control from the C library back to their code. It seems like the select() was within its rights to have passed a stack allocated buffer to be written asynchronously by the kernel since it, presumably, knew it couldn't encounter any exceptions. But injecting one has broken that assumption. If the select() implementation had returned normally with an error or was expecting then I'd assume this wouldn't have happened.
- ryao 2y agoAccording to the documentation, the wait is terminated by APC calls, so there is no need to trigger an exception in the APC call to return control to C++: https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-select https://learn.microsoft.com/en-us/windows/win32/api/winsock2... https://learn.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-waitforsingleobjectex https://learn.microsoft.com/en-us/windows/win32/api/synchapi... If they had implemented this to use a C++ exception to return control to C++, they should have encountered this stack corruption issue immediately upon implementing this, rather than have a customer some point in the future hit it before they did. My read of this is that they had the callback function do something and someone eventually got it to throw an exception. This is undefined behavior because there is no correct way to unwind a C stack frame. However, that is not obvious, especially if you test it since if the C function does nothing special such that it needs no special clean up, everything should be fine. However, WaitForSingleObjectEx() does something extremely special. Skipping that by unwinding the stack bit them. I filed bugs against both GCC and LLVM requesting warnings to protect people from doing this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118263 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118263 https://github.com/llvm/llvm-project/issues/121427 https://github.com/llvm/llvm-project/issues/121427
- DustinBrett 2y agoIt was just a dream, there's no such thing as 2.
- rectang 2y agoAfter trying and failing over several days to track down a squirrely segfault in a C project about 15 years ago, I taught myself Valgrind in order to debug the issue. Valgrind flagged an "invalid write", which I eventually hunted down as a fencepost error in a dependency which overwrote their allocated stack array by one byte. I recall that it wrote "1" rather than "2", though, haha. > Lesson learnt, folks: do not throw exceptions out of asynchronous procedures if you’re inside a system call! The author's debugging skills are impressive and significantly better than mine, but I find this an unsatisfying takeaway. I yearn for a systemic approach to either prevent such issues altogether or to make them less difficult to troubleshoot. The general solution is to move away from C/C++ to memory safe languages whenever possible, but such choices are of course not always realistic. With my project, I started running most of the test suite under Valgrind periodically. That took took half an hour to finish rather than a few seconds, but it caught many similar memory corruption issues over the next few years.
- pjmlp 2y agoSimilar experience, spending one week debugging memory corruption issues in production back in 2000, with the customer service pinging our team every couple of hours, due to it being on an high profile customer, has been my lesson.
- jart 2y agoNo the solution isn't to rewrite it in Rust. The solution is to have the option of compiling your C/C++ program with memory safety whenever things go loopy. ASAN, MSAN, and UBSAN are one great way to do that. Another up and coming solution that promises even more memory safety is Fil-C which is being made by Epic Games. https://github.com/pizlonator/llvm-project-deluge/blob/deluge/Manifesto.md https://github.com/pizlonator/llvm-project-deluge/blob/delug...
- kelnos 2y agoThe solution is usually not to do a rewrite, but I think for greenfield projects we should stop using C or C++ unless there is a compelling reason to do so. Memory-safe systems languages are available today; IMO it's professionally irresponsible to not use them, without a good reason. MSAN, ASAN, and UBSAN are great tools that have saved me a lot of time and headaches, but they don't catch everything that the compiler of a memory safe language can, at least not today.
- diekhans 2y agoNicely written (and executed). Worse that my worst memory corruption.
- tomsmeding 2y agoEveryone here is going on about exceptions bad, but let's talk about QueueUserAPC(). Yeah, let's throw an asynchronous interrupt to some other thread that might be doing, you know, anything! In the Unix world we have this too, and it's called signals, but every documentation about signals is sure to say "in a signal handler, almost nothing is safe!". You aren't supposed to call printf() in a signal handler. Throwing exceptions is unthinkable. I skimmed the linked QueueUserAPC() documentation page and it says none of this. Exceptions aren't the handgrenade here (though sure, they're nasty) — QueueUserAPC() is.
- Veserv 2y agoThat does not seem to be correct. The documentation indicates APC [1] can only occur from a waiting/blocking state. So, the program is in a consistent state and can only be on a few known instructions, unlike signals. As such, most functions should be safe to call. This is more like select() sometimes calling a user-supplied function in addition to checking for I/O. [1] https://learn.microsoft.com/en-us/windows/win32/sync/asynchronous-procedure-calls https://learn.microsoft.com/en-us/windows/win32/sync/asynchr...
- tomsmeding 2y agoI see, I read the docs slightly too quickly. Still, though, I would have expected a conspicuous warning about exceptions in those calls, because MS is in on C++ (so they can't hide behind "but we expected only C") and apparently(?) the APC machinery doesn't catch and block exceptions in user code.
- cryptonector 2y agoThey call it an "alertable state", and that's: > A thread enters an alertable state when it calls the SleepEx, SignalObjectAndWait, MsgWaitForMultipleObjectsEx, WaitForMultipleObjectsEx, or WaitForSingleObjectEx function. So this is a lot less like Unix signals. It only really works if the thread you're doing the async procedure call to is one that's likely to use those. So APCs are safe enough -- a lot safer than Unix signal handlers.
- 2y ago
- cyberax 2y agoA small request: please stop using automatic translation for blog posts or documentation. Especially when I still have English set as the second priority language.
- jacinda 2y agoRelated (and hilarious): https://scholar.harvard.edu/files/mickens/files/thenightwatch.pdf https://scholar.harvard.edu/files/mickens/files/thenightwatc... > What is despair? I have known it—hear my song. Despair is when you’re debugging a kernel driver and you look at a memory dump and you see that a pointer has a value of 7. THERE IS NO HARDWARE ARCHITECTURE THAT IS ALIGNED ON 7. Furthermore, 7 IS TOO SMALL AND ONLY EVIL CODE WOULD TRY TO ACCESS SMALL NUMBER MEMORY. Misaligned, small-number memory accesses have stolen decades from my life. All James Mickens' USENIX articles are fun (for a very specific subset of computer scientist - the kind that would comment on this thread). https://mickens.seas.harvard.edu/wisdom-james-mickens https://mickens.seas.harvard.edu/wisdom-james-mickens
- hinkley 2y agoI don’t know if it’s still a thing but there used to be debugging tools that would put a page of memory marked as either read only or unreadable in front of every malloc call so that any pointer arithmetic with a math error would trigger a page fault which could be debugged. It worked in apps that didn’t use too much of total memory or too many fine grained allocations. I mean obviously turning every 8 byte pointer into a whole memory page could consume all of memory very quickly. But in front of arrays or large data structures that could work.
- saagarjha 2y agoIn this case the write bypassed page protections
- dwattttt 2y agoIt shouldn't bypass page protections, that would be a kernel bug. And quite a bit harder to achieve too, since the kernel would still be using the same virtual address mapping as user space there.
- saagarjha 2y agoThey made the pages read-only for themselves; the kernel has the ability to write through that.
- hinkley 2y agoThe second piece of code I wrote for pay was a FFI around a c library, which had callbacks to send incremental data back to the caller. I didn’t understand why the documented examples would re-acquire the object handles every iteration through the loop so I dropped them. And everything seemed to work until I got to the larger problems and then I was getting mutually exclusive states in data that was marked as immutable, in some of the objects. I pulled my hair on this for days. What ended up happening is that if the GC ran inside the callback then the objects the native code could see could move, and so the next block of code was smashing the heap by writing to the wrong spots. All the small inputs finished before a GC was called and looked fine but larger ones went into undefined behavior. So dumb.
- jart 2y agoThis kind of error is a right of passage with WIN32 programming. For example, to do nontrivial i/o on Windows you have to create an OVERLAPPED object and give it to ReadFile() and WriteFile() which will return a pending status code, and write back to your OVERLAPPED object once the i/o has completed. Usually it makes the most sense to put that object on the stack. So if you return from your function without making sure WIN32 is done with that object, you're going to end up with bugs like this one. You have to call GetOverlappedResult() to do that. That means no throwing or returning until you do. Even if you call CancelIoEx() beforehand, you still need to call the result function. When you mix all that up with your WaitForMultipleObjects() call, it ends up being a whole lot of if statements you could easily get wrong if the ritual isn't burned into your brain. UNIX system calls never do this. The kernel won't keep references to pointers you pass them and write to them later. It just isn't in the DNA. The only exceptions I can think of would be clone(), which is abstracted by the POSIX threads runtime, and Windows-inspired non-standard event i/o system calls like epoll.
- dataflow 2y ago> UNIX system calls never do this. The kernel won't keep references to pointers you pass them and write to them later. It just isn't in the DNA. The only exceptions I can think of would be clone(), which is abstracted by the POSIX threads runtime, and Windows-inspired non-standard system calls like epoll. I mean, this is because the UNIX model was based on readiness rather than completion. Which is slower. Hence the newer I/O models.
- jart 2y agoSystem calls take 10x longer on Windows than they do on UNIX. That's what i/o completion ports are working around. They solve a problem UNIX doesn't have.
- dataflow 2y agoNo, IOCP is not a workaround for syscall overhead. In fact this performance hit has nothing to do with syscall overhead. The overhead is O(1). The penalty of the readiness design is O(n). Because if the system can't copy into your n-byte buffer in the background, then you gotta block for O(n) time to memcpy it yourself.
- cryptonector 2y ago> The fix was pretty straightforward: instead of using QueueUserAPC(), we now create a loopback socket to which we send a byte any time we need to interrupt select(). This is an absolutely standard trick that is known as "the self-pipe trick". I believe DJB created and named it. It is used for turning APIs not based on file handles/descriptors into events for an event loop based on file handles/descriptors, especially for turning signals into events for select(2)/poll(2)/epoll/kqueue/...
- ptsneves 2y agoAgree. Maybe This is obvious for devs of OSes that are file (descriptor) centric like Linux and POSIX. Also that pattern is a bit too sweet as it can be a nice way to create condition variables or queues, the problem being one is now paying syscall overhead.
- cryptonector 2y agoIt works with file handles too. > Also that pattern is a bit too sweet as it can be a nice way to create condition variables or queues, the problem being one is now paying syscall overhead. Sure but you don't need to, and even if you did, the system call overhead is probably not the hill you're dying on, or if it is then you want io_uring or similar.
- Dwedit 2y agoTLDR: Kernel wrote memory back to a pointer provided by the user-mode program, as it was supposed to do. Unfortunately, it was a dangling pointer (Use-after-free) When the Kernel does the memory write, user-mode memory debuggers don't see it happen.
- explosion-s 2y ago> Somebody had been touching my sentinel’s privates - and it definitely wasn’t a friend Gotta love programmers out of context
- deleted 2y ago[deleted]