Detect or avoid dead references to temporary on compile time

Question

The following minimal-ish program segfaults when compiling with -O3 and perhaps with -O2, but executes fine with -O0 (with clang 4.0):

#include <iostream>

class A {
public:
  virtual void me() const { std::cerr << "hi!\n"; }
};

class B {
public:
  B(const A& a_) : a(a_) {}
  virtual void me() const { a.me(); }

private:
  const A& a;
};

class C {
public:
  C(const B& b_) : b(b_) {}
  void me() const { b.me(); }

public:
  const B& b;
};

int main() {
  C c = C(A());
  c.me();
}

The reason is that c.b is initialized with a reference to a temporary object of class B that is constructed from a temporary A. After the constructor c.C() exits, the temporary B is gone but the reference to it remains in c.b.

What good practices can I employ to avoid this situation, given that I don't control the implementation of B or A? Are there static analyzers that are able to detect this condition? (My version of scan-build did not find the problem.)

Related: Detect dangling references to temporary

Well, u now, rvalue references. They bind only to temporaries. I think maybe that can be abstracted up as an lvalue-only-ref-wrapper. — Cheers and hth. - Alf, Sep 15 '17 at 07:19
As a general rule, do not declare member variables of type reference. Use a pointer instead. And if you get a pointer in the constructor the resulting code will be obvious. — rodrigo, Sep 15 '17 at 07:22
@rodrigo: This sounds interesting. I guess it would also work if I only declare the constructor argument as a pointer, and then initialize the reference? I'd rather not rewrite *all* of the code that uses the reference... — krlmlr, Sep 15 '17 at 07:32
Try passing `-fsanitize=undefined,address` to the compiler and linker. [ASan](http://clang.llvm.org/docs/AddressSanitizer.html) and [UBSan](http://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html) should notify you at runtime. — nwp, Sep 15 '17 at 07:33
Well, yes, changing just the constructor will make the mistake obvious: if you write `C c = C(&A());` you deserve the crash. I just don't like classes with member references, they make move and copies difficult. — rodrigo, Sep 15 '17 at 07:58
@nwp: Only `-fsanitize-address-use-after-scope` reliably detects the error on my system. — krlmlr, Sep 15 '17 at 15:57
@krlmlr: I've added an answer explaining what I commented earlier... — rodrigo, Sep 15 '17 at 16:02

score 2 · Answer 1 · answered Sep 15 '17 at 07:20

2

I would derive separate classes from B and C (possibly even using a template class).

These classes would contain a non-reference member which becomes the thing that a and b refer to.

I'd then implement the necessary copy constructors / assignment operators in these derived classes to prevent dangling references.

(Then I'd have a robust conversation with the author of B and C).

answered Sep 15 '17 at 07:20

Bathsheba

231,907
34
361
483

This is for an existing code base. What's a good way to find all uses of this anti-pattern? – krlmlr Sep 15 '17 at 07:22

score 1 · Accepted Answer · answered Sep 15 '17 at 16:00

I personally don't like having member variables as references. They cannot be copied or moved, and the user of the class must ensure that the object outlives the reference itself. An exception might be an internal auxiliary class designed to be used as temporary only objects, such as functors.

Replacing the reference with a pointer should be straightforward, and then if you also use a pointer in the constructor:

class C {
public:
  C(const A *a_) : a(a_) {}
private:
  const A *a;
};

...and so on. If the class is very big and you feel lazy and don't want to change the member, you can just change the constructor:

class C {
public:
  C(const A *a_) : a(*a_) {}
private:
  const A &a;
};

In order to misuse this class, as the OP says, you'll have to write something like:

C c = C(&A());

And then the error should be obvious: taking a pointer to a temporary is a very bad idea!

PS: I would add an explicit to your constructor, just to avoid it from participating in automatic conversions, which, I think, are part of your problem.

Thanks. I can't change class `B` or `A`, if that's you mean by adding `explicit`. — krlmlr, Sep 15 '17 at 16:42
@krimir No, I mean adding a literal `explicit C(const A *a_)` so that the constructor will not be called implicitly as part of type conversions. Some compiler errors may arise, but you will suppress unexpected calls to your constructor — rodrigo, Sep 15 '17 at 16:54
Personally don't like classes with reference members? Classes with reference members cannot be copied? Huh? Actually they can have a default copy constructor, just like classes with pointers. — Eric Roller, Sep 14 '19 at 12:49

Detect or avoid dead references to temporary on compile time

2 Answers2