Undiagnosed "reference to stack memory [...] returned"

Hi guys,

here is a very simple test case:

int const& get_reference_warning()
{
int w = 0;
return w; // expected-warning{{reference to stack memory associated with local variable ‘w’ returned}}
}

int get_value();

int const& get_reference_no_warning() { return get_value(); } // Diagnosed by VC++ 2010 (haven’t tested on gcc), but unnoticed by Clang

int const& get_reference_no_warning_2()
{
int const& w2 = get_value(); // Correct, lifetime of the value is extended to the lifetime of the const reference
return w2; // Undefined behavior: goes unnoticed by Clang, VC++ 2010 and gcc 3.4.2
}

I compiled it with: clang.exe -fsyntax-only -Wall -pedantic test.cpp (on Windows, using PowerShell)

test.cpp(9) : warning: reference to stack memory associated with local variable ‘w’ returned
return w; // expected-warning{{reference to stack memory associated with local variable ‘w’ returned}}
^
1 warning generated.

As expected, the first return provoked a warning.

However in the two other cases, no warning is generated.

I was wondering if it would be possible for clang to diagnose those cases as well. I guess the third case is the trickier, though since the lifetime of the value is correctly extended, therre should be a way to detect that w2 is not just an ordinary const reference. I must admit I haven’t come over the Clang code base yet so it’s still a bit blurry to me (by the way if someone could kindly point me to some doc for developers…)

Should I file a bug ? (or perhaps one already exists…)

Matthieu.

I'm not certain this is a bug. The 'const int&' causes the lifetime of the object returned from get_value() to be extended to its last use. Does this extend to the caller of get_reference_no_warning_2() since that also returns a const reference?

Hi Ted,

I haven’t got to generate an executable with Clang (issues with the linker, I am still trying to configure my environment…), on gcc I got a garbage value in the 3rd case.

Matthieu

2010/9/15 Ted Kremenek <kremenek@apple.com>

In get_reference_no_warning_2, w2 is bound to a temporary which is
live for the lifetime of w2; returning a reference to the temporary
doesn't extend its lifetime.

-Eli

I don’t think it could get extended either.

I must admit I don’t know much about compilers, but naively I would say in order to implement this lifetime extension that the standard mandates, I would examine the signature of get_value(), which returns an int, and allocate enough space for an int on the stack (instead of a const reference) while treating it a const reference to an int for the purpose of compilation (it cannot be modified). That’s the only trick I can think of short of allocating this on the heap… and this would be a performance killer.

However the caller of get_reference_no_warning() or get_reference_no_warning_2() expects a const reference, and thus is unlikely to prepare the stack for a copy instead (just in case). While it could possibly be done with an inline function, it would get hairy as soon as the function is defined in another translation unit.

Therefore it seems that the proper thing to do would be to diagnose the issue (in both get_reference_no_warning cases) and let the developer fix them. I surmise it should be possible to detect them (since VC++ already detects the first case), but then, as I said, I am very naive about compilers yet.

I hope to look at this further during the week-end, guess it would be as good a way as any to try and understand how Clang work

For reference I fixed such an issue today in real code (compiled with gcc 3.4.2), and it had gone undiagnosed too (and generated some mess because it was a string instead of an int…).

  • Matthieu

2010/9/15 Eli Friedman <eli.friedman@gmail.com>

Yes, we should be able to diagnose both cases... the relevant code is
Sema::CheckReturnStackAddr in SemaChecking.cpp.

-Eli

Of course, that makes perfect sense.

I'll fix this. It will be good for me (after being confused about this one).

Does the analyzer support C++ now?

-Alexei

No it doesn’t. There is active work on this front, but no ETA right now.

Okay, with the week-end I have been able to look at this issue a bit more in-depth:

So, the first issue is:

struct S {
int array[32];
};

S value() { return S(); }

S const& return_temporary() {
return value();
}

The return statement yields the following AST:

However static DeclRefExpr* EvalVal(Expr *E) in SemaChecking.cpp (line 1979) does not handle CallExpr (or CXXOperatorCallExpr or CXXMemberCallExpr for that matter)

I was thinking about adding a case in the switch:

case Stmt::CallExprClass:
case Stmt::CXXMemberCallExprClass:
case Stmt::CXXOperatorCallExprClass: {
// Functions / Methods / Operators
// that return a pointer or prvalue
CallExpr* CE = cast(E);
if (!CE->getCallReturnType()->isReferenceType()) {
return CE;
}
return NULL;
}

since at the moment it’s not handled at all. However there are MANY possible kinds for Type, so I am quite puzzled when it comes to figuring out which ones are okay and which are not… Furthermore there is it seems the issue of typedef which might embed a reference within and that we’ll need to account for.

Regarding the second issue:

S const& return_bounded_temporary() {
S const& bind = value();
return bind;
}

This returns the following AST:

So there is no indication here that the reference variable has bounded a temporary at all, I would have expected something like CXXBindTemporaryExpr.

Comments are welcome :slight_smile:

2010/9/16 Ted Kremenek <kremenek@apple.com>

Hi Matthieu,

Sorry for the (very…) late response. We now warn for all cases since http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20101129/036838.html

Thanks for reporting the issue!

-Argiris

Hi Argyrios,

Thank you very much!

It’s great that clang will diagnose this UB, it’s a really nasty bug to track down :slight_smile:

Matthieu.

2010/11/29 Argyrios Kyrtzidis <akyrtzi@gmail.com>

Hi Argyrios,

Thank you very much!

It’s great that clang will diagnose this UB, it’s a really nasty bug to track down :slight_smile:

I reverted the previous commit, much better fix in http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20101129/036875.html.
e.g. for

struct S {
int x;
};

int &get_ref() {
S s;
S &s2 = s;
int &x2 = s2.x;
return x2;
}

we get

t3.cpp:9:10: warning: reference to stack memory associated with local variable ‘s’ returned
return x2;
^~
t3.cpp:8:8: note: binding reference variable ‘x2’ here
int &x2 = s2.x;
^ ~~
t3.cpp:7:6: note: binding reference variable ‘s2’ here
S &s2 = s;
^ ~
1 warning generated.

Hi Argyrios,

this one is astonishing! It goes even beyond what I expected truth to be told.

I really hope it passes the self-hosting test and you don’t have to revert it.

Thanks,
Matthieu.

2010/12/1 Argyrios Kyrtzidis <kyrtzidis@apple.com>