13

On a recent bug hunt, I found an issue with returning a pointer to a member of a temporary variable. The offending (simplified) code was:

struct S {
    S(int i) : i(i) {}
    int i;
    int* ptr() { return &i; }
};

int* fun(int i) { return S(i).ptr(); }  // temporary S dies but pointer lives on

int main() {
    int* p = fun(1);
    return *p;  // undefined
}

How to prevent this? GCC & Clang have -Waddress-of-temporary and -Wreturn-stack-address but they seem to loose trail because of ptr() acting as a middle man for the dirty deeds. They are only triggered when the pointer is taken directly:

int* fun(int i) { return &S(i).i; }  // rightly fails to compile

My project also incorporates cppcheck in continuous integration but it also can't pick it up (raised here).

Which static analysis tool can prevent this class of bugs?

EDIT: GCC does pick it up since version 6.1.0 with -Wreturn-local-addr and (surprisingly) -O2 switched on.

orbitcowboy
  • 1,438
  • 13
  • 25
Tomek Sowiński
  • 863
  • 5
  • 16
  • 3
    If you compile with `-O2`, gcc catches this bug: http://melpon.org/wandbox/permlink/KaXY4ktTM1vMNTiX – krzaq Nov 08 '16 at 21:20
  • 11
    "How to prevent this?" there is only one solution - stop using raw pointers – Slava Nov 08 '16 at 21:20
  • 1
    @krzaq `-O2` has nothing to do with it but the version of GCC does (detects since 6.1.0). I can't use a GCC that new, but it's a good argument to troll our maintainers to switch to a newer one:) – Tomek Sowiński Nov 08 '16 at 21:30
  • 1
    @TomekSowiński It doesn't show this warning when you compile without `-O2` on wandbox. But It's still a good idea to update :) – krzaq Nov 08 '16 at 21:31
  • 2
    Well, `int* ptr() & { return &i; }` works in your specific case, but you can easily get the issue back by doing `int* fun(int i) { S s{i}; return s.ptr(); }`. – skypjack Nov 08 '16 at 21:32
  • 2
    @krzaq ah, you're right (it doesn't detect with or without `-O2` before 6.1.0, though). Come to think again... why would an optimization switch influence static analysis? I may file a bug for GCC:) – Tomek Sowiński Nov 08 '16 at 21:38
  • 5
    @Slava not really, same happens with references. Besides, there are valid reasons to return a member by reference (pointer). – Tomek Sowiński Nov 08 '16 at 21:40
  • 3
    @Slava What he really means is "How to get the compiler to warn me so I'll know to fix it?" Obviously the compiler can't prevent you from doing it in the first place. – Barmar Nov 08 '16 at 21:56
  • 2
    @TomekSowiński yes same happens, bad design is a bad design either by pointer or reference – Slava Nov 08 '16 at 21:59
  • 1
    To maintainers: I'm asking which tool **can** detect this programming error, which is not a matter of opinion and doesn't attract opinionanted answers. Please don't close this question. – Tomek Sowiński Nov 08 '16 at 21:59
  • 2
    @Barmar obviously compiler can prevent you only in trivial cases. Create a local variable and call that method and it is not possible to detect that – Slava Nov 08 '16 at 22:00
  • 2
    "why would an optimization switch influence static analysis?" Because GCC is not a static analyzer. – T.C. Nov 08 '16 at 22:02
  • I have created a ticket on Cppcheck's bug tracker about this issue in order to catch issues like this in the future. Ticket: http://trac.cppcheck.net/ticket/7812#ticket – orbitcowboy Nov 13 '16 at 10:18
  • @TomekSowiński asking for third party tools is considered off-topic here – M.M Nov 13 '16 at 10:22
  • Slightly off-topic, but avoiding these classes of bugs is the raison d'être of Rust. Learning Rust made me a better C++ developer. – experquisite Nov 13 '16 at 20:07
  • You should not write code like that in 2016... First, you should avoid as much as possible to return pointer/reference to internal objects (bad design) and if you need, then you should use smart pointers instead. – Phil1970 Nov 16 '16 at 00:30
  • 1
    The problem with the code above is that the compiler need to do multiple level analysis to detect the problem. It it legal to return the address of a member of a class as long as you don't use that pointer after the containing object is destroyed. And then in your `fun` function, the compiler won't check the function and assume that it return a pointer that do not depend on the object. With optimization, the compiler would do inlining and see it as it would be a single function. With external function, it would not be able to do the check (at least without global analysis). – Phil1970 Nov 16 '16 at 00:35

1 Answers1

3

I am a Cppcheck developer.

My project also incorporates cppcheck in continuous integration but it also can't pick it up.

interesting bug. This is the kind of bug that cppcheck wants to warn about. We have some related checks but this slipped through unfortunately.

Not really surprising given the regex nature of cppcheck.

I don't personally understand why some people say cppcheck is a regex tool.

It uses AST, context sensitive valueflow analysis, etc to detect bugs. So does GCC and Clang. Cppcheck is sometimes claimed to be a regex tool but GCC and Clang are not.

Daniel Marjamäki
  • 2,907
  • 15
  • 16
  • 1
    FYI: A ticket about this issue is already created http://trac.cppcheck.net/ticket/7812 – orbitcowboy Nov 13 '16 at 19:41
  • 3
    I admit I didn't look into cppcheck internals and probably got the impression it's regex-based on the way custom rules are defined. I removed that remark from the question not to proliferate wrong info. – Tomek Sowiński Nov 15 '16 at 23:15