7 ms·
A production bug that made me care about undefined behavior
- titzer 10mo agotldr; the UB was reading uninitialized data in a struct. The C++ rules for when default initialization occurs are crazy complex. I think a sanitizer probably would have caught this, but IMHO this is the language's fault. Hopefully future versions of C++ will mandate default initialization for all cases that are UB today and we can be free of this class of bug.
- trueismywork 10mo agoFor now, best strategy is to initialize everything explicitly.
- torstenvl 10mo agoYeah... but I wouldn't characterize the bug itself (in its essential form) as UB. Even if the implementation specified that the data would be indeterminate depending on what existed in that memory location previously, the bug would still exist. Even if you hand-coded this in assembly, the bug would still exist. The essence of the bug is uninitialized data being garbage. That's always gonna be a latent bug, regardless of whether the behavior is defined in an ISO standard.
- forrestthewoods 10mo agoYeah I agree. This is a classic “uninitialized variable has garbage memory value” bug. But it is not a “undefined nasal demons behavior” bug. That said, we all learn this one! I spent like two weeks debugging a super rare desync bug in a multiplayer game with a P2P lockstep synchronous architecture. Suffice to say I am now a zealot about providing default values all the time. Thankfully it’s a lot easier since C++11 came out and lets you define default values at the declaration site!
- titzer 10mo agoI prefer language constructs define that new storage is zero-initialized. It doesn't prevent all bugs (i.e. application logic bugs) but at least gives deterministic results. These days it's zero cost for local variables and near-zero cost for fields. This is the case in Virgil.
- kevin_thibedeau 10mo agoC & C++ run on systems where it may not be zero cost. If you need low latency startup it could be a liability to zero out large chunks of memory.
- ablob 10mo agoI think it's acceptable to leave an escape hatch for these situations instead of leaving it to easy to misunderstand nooks and crannies of the standard. You don't want to zero out the memory? Slap a "foo = uninitialized" in there to have that exact behavior and get the here be demons sign for free.
- forrestthewoods 10mo agoYeah this issue is super obvious and non-controversial. Uninitialized state is totally fine as an opt-in performance optimization. But having a well defined non-garbage default value should obviously be the default. Did C fuck that up 50 years ago? Yeah probably. They should have known better even then. But that’s ok. It’s a historical artifact. All languages are full of them. We learn and improve!
- deleted 10mo ago[deleted]
- 1718627440 10mo agoI don't know, I expect all variables to be uninitialized until proven otherwise. It makes it easier for me to reason about code, especially convoluted code. But I also like C a lot and actually explicitly invoke UB quite often, so there is that.
- tialaramex 10mo agoIn C++ 26 reading an uninitialized variable is by default Erroneous Behaviour, which means your compiler is encouraged to diagnose this (it's an error) but if it happens anyway (perhaps because the compiler can't tell before runtime) there's a specified behaviour, it isn't Undefined Behaviour. The compiler will have chosen some value for that uninitialized variable and if it can't just diagnose that what you wrote was nonsense, it has some arbitrary value, perhaps configurable or perhaps described in your compiler's documentation. So these variables will be more or less what the current "defanged" Rust std::mem::uninitialized() function gets you. A bit slower than "truly" uninitialized variables, but not instant death in most cases if you made a mistake because you're human. Those C++ people who feel they actually need uninitialized variables can tell the compiler explicitly [for that particular variable] in C++ 26 that they opt out of this safeguard. They get the same behaviour you've seen described in this thread today, arbitrary Undefined Behaviour if you read the uninitialized variable. This would be similar to modern Rust's MaybeUninit::uninit().assume_init() - you are explicitly telling the compiler it's OK to set fire to everything, you should probably not do this, but we did warn you.
- koyote 10mo ago> Hopefully future versions of C++ will mandate default initialization for all cases that are UB today and we can be free of this class of bug. I have production code where we specifically do not initialise some data in order to be more performant (it gets set before it is read, but not at declaration as we do not have the values at that point). I do agree that this (and numerous other footguns) make C++ a pain to work with. I also think it's too late to fix. Ideally, all values would be initialised by default and instead you could forcefully construct something that is not initialised (e.g. something like `no_init double x[100];`). Instead, we have the bug-prone default and twenty different ways to initialise something, each with their own caveats. C++ would be a much better language if most if not all defaults were reversed.
- simonask 10mo agoEvery C++ compiler is perfectly able to optimize stale writes, so I'm always skeptical of code that leaves uninitialized fields around. I would always strongly prefer rearranging the code to be easier on the optimizer.
- kayo_20211030 10mo agoGreat post. It was both funny and humble. Of course, it probably wasn't at all funny at the time.
- vhantz 10mo agoThe two fields in the struct are expected to be false unless changed, then initialize them as such. Nothing is gained by leaving it to the compiler, and a lot is lost.
- gwd 10mo agoI think the point is that sometimes variables are defined by the language spec as initialized to zero, and sometimes they aren't. Perhaps what you mean is, "Nothing is to be gained by relying on the language spec to initialize things to zero, and a lot is lost"; I'd agree with that.
- vhantz 10mo agoPlease don't be pedantic. Compilers implement the standard, otherwise it's just a text document.
- hn_go_brrrrr 10mo agoCompilers implement the parts of the standard they agree with, in the way they think is best. They also implement it in the way they understand the standardese. Read a complex enough project that's meant to be used across compiler venrdos and versions, and you'll find plenty of instances where they're working around the compiler not implementing the standard. Also, if you attended the standards committee, you would hear plenty of complaints from compiler vendors that certain things are implementable. Sometimes the committee listens and makes changes, other times they put their fingers in their ears and ignore reality. There are also plenty of places where the standard lets the compiler make it's own decision (implementation defined behavior). You need to know what your compiler vendor(s) chose to do. tl;dr: With a standard as complex as C++'s, the compilers very much do not just "implement the standard". Sometimes you can get away with pretending that, but others very much not.
- vhantz 10mo agoWho said compilers "just" implement the standard? The standard (to the extent that it is implemented) is implemented by compilers. At this point this whole thread has nothing to do with my original point, just weird one-upping all around
- nneonneo 10mo agoEven calling uninitialized data “garbage” is misleading. You might expect that the compiler would just leave out some initialization code and compile the remaining code in the expected way, causing the values to be “whatever was in memory previously”. But no - the compiler can (and absolutely will) optimize by assuming the values are whatever would be most convenient for optimization reasons, even if it would be vanishingly unlikely or even impossible. As an example, consider this code (godbolt: https://godbolt.org/z/TrMrYTKG9 https://godbolt.org/z/TrMrYTKG9): struct foo { unsigned char a, b; }; foo make(int x) { foo result; if (x) { result.a = 13; } else { result.b = 37; } return result; } At high enough optimization levels, the function compiles to “mov eax, 9485; ret”, which sets both a=13 and b=37 without testing the condition at all - as if both branches of the test were executed. This is perfectly reasonable because the lack of initialization means the values could already have been set that way (even if unlikely), so the compiler just goes ahead and sets them that way. It’s faster!
- arrowsmith 10mo agoHow is this an "optimization" if the compiled result is incorrect? Why would you design a compiler that can produce errors?
- tehjoker 10mo agoIt's not incorrect. Where is the flaw?
- Negitivefrags 10mo agoIt’s not incorrect. The code says that if x is true then a=13 and if it is false than b=37. This is the case. Its just that a=13 even if x is false. A thing that the code had nothing to say about, and so the compiler is free to do.
- foltik 10mo agoOk, so you’re saying it’s “technically correct?” Practically speaking, I’d argue that a compiler assuming uninitialized stack or heap memory is always equal to some arbitrary convenient constant is obviously incorrect, actively harmful, and benefits no one.
- mac3n 10mo agoMany years had a customer complaint about undefined data changing value in Fortran 77. It turned out that the compiler never allocated storage for uninitialized variables, so it was aliased to something else. Compiler was changed to allocate storage for any referenced varibles.
- panstromek 10mo agoI have bumped into this myself, too. It's really annoying. The biggest footgun isn't even discussed explicitly and it might be how the error got introduced - it's when the struct goes from POD to non-POD or vice-versa, the rules change, so completely innocent change, like adding a string field, can suddenly create undefined behaviour in unrelated code that was correct previously.
- feelamee 10mo agowow, can you elaborate how adding a string field can break some assumptions?
- nneonneo 10mo agoNot the OP, but note that adding a std::string to a POD type makes it non-POD. If you were doing something like using malloc() to make the struct (not recommended in C++!), then suddenly your std::string is uninitialized, and touching that object will be instant UB. Uninitialized primitives are benign unless read, but uninitialized objects are extremely dangerous.
- IshKebab 10mo agoThat's not what was happening in this example though. It would be UB even if it was a POD.
- inglor_cz 10mo agoSymbian's way of avoiding this was to use a class called CBase to derive from. CBase would memset the entire allocated memory for the object to binary zeros, thus zeroizing any member variable. And by convention, all classes derived from CBase would start their name with C, so something like CHash or CRectangle.
- jenadine 10mo agoI'm afraid that's still not defined behaviour in many case. For example, pointer and bool can be initialized with `=0`, but that doesn't mean the binary representation in memory has to be 0, and so initializing with memset would still be wrong. (Even if it works with all compilers I know of.) Also, how does CBase knows the size of its allocated memory?
- ksherlock 10mo agoThe symbian source code is available. Looks like it uses a class-specific operator new() overload. https://github.com/SymbianSource/oss.FCL.sf.os.kernelhwsrv/blob/0c3208650587ac0230aed8a74e9bddb5288023eb/kernel/eka/include/e32base.h#L29 https://github.com/SymbianSource/oss.FCL.sf.os.kernelhwsrv/b... 2. Initialisation of the CBase derived object to binary zeroes through a specific CBase::operator new() - this means that members, whose initial value should be zero, do not have to be initialised in the constructor. This allows safe destruction of a partially-constructed object.
- MutableLambda 10mo agoYeah, looks pretty straightforward to me, but I used to write C++ for a living. I mean, there are complicated cases in C++ starting with C++11, this one is not really one of them. Just init the fields to false. Most of these cases is just C++ trying to bring in new features without breaking legacy code, it has become pretty difficult to keep up with it all.
- fizzynut 10mo agoEven if you fixed the initialized data problem, this code is still a bug waiting to happen. It should be a single bool in the struct to handle the state for the function as there are only two states that actually make sense. succeeded = true; error = true; //This makes no sense succeeded = false; error = false; //This makes no sense Otherwise if I'm checking a response, I am generally going to check just "succeeded" or "error" and miss one of the two above states that "shouldn't happen", or if I check both it's both a lot of awkward extra code and I'm left with trying to output an error for a state that again makes no sense.
- deepsun 10mo agoIt happens often when "error" field is not a bool, but a string, aka error_message. Could be empty string, or _null_, or even _undefined_ if we're in JS. Then the obvious question why do we need _succeeded_ at all, if we can always check for _error_. Sometimes it can be useful, when the server doesn't know itself if the operation is succeeded (e.g. an IO/database operation timed out), so it might be succeeded, but should also show an error message to user. Another possibility if the succeeded is not a bool, but, say, "succeeded_at" timestamp. In general, I noticed that almost always any boolean value in database can be replaced with a timestamp or an error code.
- yongjik 10mo agoI think UB doesn't have much to do with this bug after all. The original code defined a struct with two bools that were not initialized. Therefore, when you instantiate one, the initial values of the two bools could be anything. In particular, they could be both true. This is a bit like defining a local int and getting surprised that its initial value is not always zero. (Even if the compiler did nothing funny with UB, its initial value could be anything.)
- baobun 10mo ago> The original code defined a struct with two bools that were not initialized. Therefore, when you instantiate one, the initial values of the two bools could be anything. In particular, they could be both true. Then reading from that struct like in OP constitutes UB.
- yongjik 10mo agoWell yes, that would be UB, but even if the C++ compiler had no concept of UB, it would still be wrong code.
- masklinn 10mo agoThe entire "its initial value could be anything" is one of the possible consequences of UB. It is not the most dire but in C and C++ it is an outcome from an UB. Could a language define un-initialized variables as readable garbage? Sure, but that would be a different language with different semantics, and such languages can also define declaration such that > defining a local int and getting surprised that its initial value is not always zero. is in fact reasonable. That is what Java and Go opted to do, for instance.
- pornel 10mo agoTo me the real horror is that the exact same syntax can be either a perfectly normal thing to do, or a horrible mistake that gives the compiler a license to kill, and this doesn't depend on something locally explicit, but on details of a definition that lives somewhere else and may have multiple layers of indirection.
- AdieuToLogic 10mo agoThere are a few problems with this post: 1 - In C++, a struct is no different than a class other than a default scope of public instead of private. 2 - The use of braces for property initialization in a constructor is malformed C++. 3 - C++ is not C, as the author eventually concedes: At this point, my C developer spider senses are tingling: is Response response; the culprit? It has to be, right? In C, that's clear undefined behavior to read fields from response: The C struct is not initialized. In short, if the author employed C++ instead of trying to use C techniques, all they would have needed is a zero cost constructor definition such as: inline Response () : error (false), succeeded (false) { ; }
- jhasse 10mo agoinline and ; are redundant
- AdieuToLogic 10mo ago> inline and ; are redundant One of my s/w engineering axioms is: Better to express intent than assume a future reader of a solution, including myself, will intrinsically understand the decisions made. If this costs a few extra keystrokes when authoring an implementation, so be it.
- Panzerschrek 10mo agoThat's why I always specify default initializers for fields of fundamental types and other types which don't have default constructor.
- letmetweakit 10mo agoI once reported several UB bugs to a HackerOne-led cryptocurrency bounty program. They were rejected because the software was working as intended and that they would "inspect the assembly every time they compiled". Yeah right.
- nurumaik 10mo agoExample from this article looks more like "unspecified" behavior rather than "undefined". Title made me expect nasal demons, now I'm a bit disappointed
- canucker2016 10mo agoBut there's nothing in your code that suggests that there's a problem if the error and success fields are both true. Typically you'd have at least an assert (and hopefully some unit tests) to ensure that invariant (.success ^ .error == true). But the code has just been getting by on the good graces of the previous stack contents. One random day, the app behaviour changed and left a non-zero byte that the response struct picked up and left the app in the alternate reality where .success == .error Others have mentioned sanitizers that may expose the problem. Microsoft's Visual C++ compiler has the RTCs/RTC1 compiler switch which fills the stack frame with a non-zero value (0xCC). Using that compiler switch would have exposed the problem. You could also create a custom __chkstk stack probe function and have GCC/Clang use this to fill the stack as well as probing the stack. I did this years ago when there was no RTCs/RTC1 compiler option available in VC++.
- quuxplusone 10mo agoI mean, "obviously" if you don't initialize your variables, they'll contain garbage. You can't assume that garbage is zero/false, or any other meaningful value. But re the distinction at the end of TFA — that a garbage char is slightly more OK than a garbage bool — that's also intuitive. Eight bits of garbage is always going to be at least some valid char (physically speaking), whereas it's highly unlikely that eight bits of garbage will happen to form a valid bool (there being only two valid values for bool out of those 256 possible octets). This also relates to the (old in GCC but super new in Clang, IIUC) compiler option -fstrict-bool.