A bug in GCC that may cause memory leaks in valid C++ programs

https://akrzemi1.wordpress.com/2017/04/27/a-serious-bug-in-gcc/

by ingve • 9 years ago

235 103 9 years ago

akrzemi1.wordpress.com

jhasse2• 9 years ago

Nice find!

Btw: Is it correct that this won't get fixed in Debian because it isn't a security issue?

matheweis2• 9 years ago

Not sure on the current Debian policy but this bug could certainly be framed as a potential security issue; exploiting the bug could result in resource exhaustion and denial of service.

ensiferum• 9 years ago

Exploited by who? The programmers ?

xorblurb2• 9 years ago

For released distro, I doubt Debian will rebuild all their packages and upload them all on security.debian.org...

vvanders6• 9 years ago

Good stuff.

Only compiler bugs I've seen in C++ have all been around exceptions. I think this is just another reason towards force-handled error codes. You don't have a whole new control flow that needs to be verified and is much simpler from an implementation/runtime perspective.

jhasse2• 9 years ago

I think this would just move the bugs to the applications.

vvanders6• 9 years ago

Hence the "force-handled" part of my comment :).

wvenable2• 9 years ago

It doesn't solve the problem because you'd have move to 2 stage construction/initialization and that creates even more problems than this.

vvanders6• 9 years ago

Nope, if you have an operation that can fail then wrap it in a type that represents that(I'm partial to Rust's Result<T,E>) then you force consumers to deal with potential errors and you can do 1-call construct + init if you want to.

Exceptions cause all sorts of problems(like violating the principal of only paying for what you use). They aren't checked so you can't enforce handling via type system and in general they're just a mess in C++.

hermitdev• 9 years ago

A number of years ago, I hit against a GCC bug that generated an ICE (internal compiler error). Cause? Usage of an anonymous namespace in the global namespace. It was a short-lived regression, I think in the 4.1.2 series that came to me as a result of a supposedly minor OS update.

user5994461• 9 years ago

That makes sense. Exceptions in C++ are a mess. It's completely platform, architecture, OS and compiler version specific. That's not well defined.

metaobject• 9 years ago

Where in the C++ spec are implementations of features defined?

int_19h8• 9 years ago

The behavior of exceptions in C++ is well-defined. But implementations do have bugs about many things that are well-defined - this is one of those cases.

pjmlp• 9 years ago

Of course it is not defined, that is a language specification not a compiler implementation guide.

Ace17• 9 years ago

Just because a language feature is hard to implement doesn't mean we should abandon it.

I'm not going back to explicit error handling.

warmwaffles• 9 years ago

Come on in, the water is fine. Handling errors without using exceptions is not that bad.

codedokode• 9 years ago

I disagree. You are suggesting everyone should write error handling code manually rather than rely on compiler.

vertex-four3• 9 years ago

Not really. See Rust/Scala/Haskell's Result/Either type, which has compiler support, but that compiler support is lightweight - you use the same control structures and flows as with regular code.

josefx• 9 years ago

By that logic we should write hand optimized assembly. I think there was even a rant by Linus on GCC messing up the stack and he writes C. Clearly compilers shouldn't be trusted with important programs.

mnarayan012• 9 years ago

If you assign the initialized value to a variable then you get the expected behavior: https://wandbox.org/permlink/r6csgR44BZBWudLV. What exactly is the defined destruction behavior for an object that's never (necessarily) on the stack? Or is it actually required to be on the stack in this case?

JonathonW2• 9 years ago

In C++14, the spec specifies that a temporary object's destructor is called as the last step in evaluating the full expression that contains the point where it was created (C++14 §12.2).

I'm presuming it's the same in earlier versions of the language, but I don't have those specs handy ATM.

maxlybbert• 9 years ago

I don't have the specs either, but I do remember that in Design and Evolution of C++, Stroustrup discusses nailing down the lifetime of temporary objects. I believe it was in the first standard (i.e. 1998).

mnarayan012• 9 years ago

I'm more talking about the exception behavior looking at e.g. https://wandbox.org/permlink/S5paK3Z9NpeCv3I0 (which exhibits the same bug). I mean you're right, and the fact that the User object is temporary should presumably not keep the first Resource destructor from firing, just it seems weird. I guess in my defense, apparently it seems weird to GCC as well.

ambrop7• 9 years ago

How can it be this was reported almost two years ago and nothing has been done?

Kenji• 9 years ago

Show me a program that someone used in production that became unstable because of this bug, and I will give you the answer to your question ;)

DoofusOfDeath2• 9 years ago

I imagine it's because those who maintain GCC have varying agendas for prioritizing their contributions.

The fact that is hasn't been fixed yet may be a signal that, in practice, this hasn't bitten anyone hard enough to motivate a patch submission. In which case, one could argue that the bugs are being tackled in a somewhat rational order.

It may be instructive to consider why you yourself haven't submitted a fix. Perhaps that reason is common across many of those capable of submitting a fix.

dsharlet• 9 years ago

I really disagree with this. I work on compilers, so I feel for the people who work on GCC, but, I think it is totally unreasonable to expect the average GCC user who might have encountered this bug to be capable of fixing this themselves.

I hacked around in clang for a bit for a compiler project. I think it would have taken me weeks or months to get to the point where I was able to fix the equivalent bug (if it existed) in Clang. Compilers are big, hard to debug projects. And, IMHO, the Clang/LLVM code is much more approachable than GCC. Not all developers are hobbyists that can afford to detour from their work for weeks or months to ramp up on GCC to fix this.

I'd also point out that this is the kind of bug that could exist and be triggering in a lot of code as we speak but no one (besides the author of this blog post) have caught it. I think it's insane to ignore a bug like this.

reallifecoder• 9 years ago

Any C++ programmer knows that bad things happen when throwing in a ctor. Just don't do it. This is a non problem.

YSFEJ4SWJUVU6• 9 years ago

No. Just no.

CamperBob22• 9 years ago

The RAII doctrine leaves you little choice, since the architects of C++ decided that constructors wouldn't be allowed to return NULL to indicate failure, or otherwise signal it in an easily-detectable manner.

That's why I don't use RAII. It comes with a massive amount of baggage.

anarazel2• 9 years ago

Because there's actually not that many gcc developers, and very few companies support its development?

throwaway91111• 9 years ago

Yea, the core project of GNU has "very few companies" supporting it.

zvrba• 9 years ago

A meta-comment on this: had this been a bug in VC people here would shit on Microsoft and their non-compliance to standards, not caring and so on and so on. But since it's in GCC, people are discussing a language feature.

al2o3cr• 9 years ago

As others have pointed out up-thread, the bug only triggers on a very specific use case - changing the body of the sample to:

        User u{make_1(), make_2()};
        process(u);

causes the first resource to be correctly destroyed.

wfunction4• 9 years ago

I would like to point out that if you write code like this (that is, creating a temporary object only to have it instantly deleted), your code smells.

Backwards-compatibility notwithstanding, it would not be totally illogical for C++ to allow optimizing out construct-destruct chains even in the presence of side effects, in a similar manner to the existing copy elision behavior. After all, the object is unused, and the destructor should be merely undoing whatever the constructor did, so afterward it should be as if the object was never constructed.

If you rely on the side effects of normal construction, that's not so different from relying on the side effects of copy construction, which you can't already do. Hence you shouldn't do the first either.

So yes, it's a bug, and needs to be fixed, but regardless of that, you shouldn't write code like this in the first place.

EDIT: See comment below, apparently the bug comes up in more cases than those I referred to here.

MereInterest• 9 years ago

I don't think it seems like a code smell at all. For example, I could be adding together multiple vectors, some of which are created as temporaries.

    auto total_offset = Vector{x_offset, y_offset} + global_offset;

Granted, in this case, there are no resources being held by such a Vector class, but having immediately-destroyed temporary objects is not necessarily a code smell.

wfunction4• 9 years ago

That's definitely not "immediately destroyed", there's operator+ being called between construction and destruction.

I didn't realize this can trigger the bug though, so if it does, I should clarify I didn't mean to include it in my comment. I was only referring to cases where there is no intervening operation between construction and destruction.

ars2• 9 years ago

I would like to point out that if you write code like this (that is, creating a temporary object only to have it instantly deleted), your code smells.

Not necessarily. What if all you want are the side effects of the object?

In particular I/O. Either disk or network.

Creating the object opens the connection, destroying it closes the connection. And you use the object in between to write data.

Or, don't assign the new object to anything, and simply write the data you pass in to it, and close the stream.

It's a perfectly reasonable use case.

wfunction4• 9 years ago

And you use the object in between to write data

That's not what I was talking about. See the original comment and follow-ups.

nuntius• 9 years ago

Instantly, as in next line of code? Yeah, probable human error (or auto-generated code).

Without being used? Not surprising. It is surprisingly common to allocate a variable, pass it to a function, and have the call never actually use the variable. Unwind, and the variable is destructed without use.

I've seen other nasty framework errors exposed by such (non-)usage patterns.

homerowilson• 9 years ago

read http://250bpm.com/blog:4 for an interesting related discussion.

rsfinn• 9 years ago

If you're curious (as I was), this bug does not occur in clang, as you can demonstrate for yourself by following the link to wandbox.org in the original story and changing the compiler.

isaac_is_goat• 9 years ago

I tested it with Visual Studio 2017 and it seems to work correctly there as well.

Uptrenda• 9 years ago

Reminds me of this: https://www.win.tue.nl/~aeb/linux/hh/thompson/trust.html

Trusting Trust.

72deluxe• 9 years ago

Very interesting. This is impossible in C++03; this must be related to the very useful changes in aggregate initialisation for C++11 (particularly useful as there's one great syntax for that and uniform initialisation).

targeted• 9 years ago

This reminds me of a bug that I found in Borland C++ 14 years ago: http://qc.embarcadero.com/wc/qcmain.aspx?d=3478

Sad to know that after all that time compilers still can't get even the core semantics right.

faragon2• 9 years ago

C++ exceptions are dangerous, even when working as expected. In my opinion, C++ without exceptions (or the minimal for e.g. logging the error -division by 0, invalid memory access, etc.-) is a fine language.

xorblurb2• 9 years ago

Note: there is no such thing as standard C++ exceptions on invalid memory accesses. I'm not sure about division by 0, might be in the same case.

faragon2• 9 years ago

Standard or not, if you use an OS with memory protection (MMU), you'll get those exceptions in C++.

lmm3• 9 years ago

Without exceptions you can't do RAII (because you can't do nontrivial work in constructors, because you have no way to report failure) which is probably C++'s one real selling point. (There are other reasons I'd say it isn't a fine language - mainly the lack of sum types and the way templates are only checked after full expansion (that one will at least be fixed if and when concepts make it into the language)).

nathell• 9 years ago

RAII used to be incompatible with exceptions (I'm not sure whether anything changed for the better in C++17). The reason is that you can't throw exceptions from destructors, and e.g., on Linux, destruction of the file object (wrapping a file descriptor) can result in an error because close() may well return -1 setting errno to EIO. See `man 2 close` for details.

tcbawo• 9 years ago

That's really not true. It takes a little discipline to ensure your constructor will succeed prior to calling it, but it's worth it. You generally want to make your destructor noexcept, so you get free implicit move optimizations.

A bug in GCC that may cause memory leaks in valid C++ programs

Related Stories