6

This is a variant of the questions Downcasting using the Static_cast in C++ and Safety of invalid downcast using static_cast (or reinterpret_cast) for inheritance without added members

I am not clear on the phrase in the standard "B that is actually a subobject of an object of type D, the resulting pointer points to the enclosing object of type D" with respect to behavior in ~B. If you cast to D in ~B, is it still a subobject at that point ? The following simple example shows the question:

void f(B* b);

class B {
public:
  B() {}
  ~B() { f(this); }
};

class D : public B { public: D() {} };

std::set<D*> ds;

void f(B* b) {
  D* d = static_cast<D*>(b);  // UB or subobject of type D?
  ds.erase(d);
}

I know that the cast is an open door to disaster, and doing anything like this from the dtor is a bad idea, but a co-worker claims "The code is valid and works correctly. That cast is perfectly valid. The comment clearly states that it should not be dereferenced".

I pointed out that the cast is unnecessary and we should prefer the protection provided by the type system to comments. The sad part is that he is one of the senior/lead developers and a supposed c++ "expert".

Can I tell him the cast is UB ?

Community
  • 1
  • 1
Chris Morley
  • 891
  • 6
  • 16
  • 2
    I _think_ this is UB, but I'm not sure. However, the code [at least in this sample] absolute reeks worse than my socks after two-weeks in a hot summer without washing... The correct thing here would be to have a desctructor in `D` that removed the object from `ds` - it should not be done in `B`. This would of course also avoid any problem with UB. The fact that it may indeed work and be well defined is beside the point. Or make `ds` into a `std::set bs`... – Mats Petersson Mar 08 '15 at 16:34
  • As long as there do not exist any other classes derived from B and D has no additional members, this might work alright. But the whole thing smells. How can you make sure that every B is a D and if this is sure why would you have different classes in the first place? – Kit Fisto Mar 08 '15 at 16:37
  • @MatsPetersson: Why do you think this is UB? The standard at 5.2.9 gives an example almost equal to the OP's, although it uses references: `struct B { }; struct D : public B { }; D d; B &br = d; static_cast(br);` Of course, this is just a theoretical question. The code in the OP's question is horrible. – Christian Hackl Mar 08 '15 at 16:48
  • 4
    @ChristianHackl The point is that the conversion in the code in the OP happens *during destruction*, and indeed after `D`'s destructor body has finished execution. – T.C. Mar 08 '15 at 16:57
  • @T.C.: Ah, I see. You're right, this complicates everything even more. Nice senior developer the OP has there... :) – Christian Hackl Mar 08 '15 at 17:06
  • [basic.life]/p1 says that an object's lifetime ends as soon as the destructor call starts, and [class.dtor]/p15 says "Once a destructor is invoked for an object, the object no longer exists", which arguably implies that the `D` object "no longer exists" at the time of the `static_cast`. But this is pretty murky. – T.C. Mar 08 '15 at 17:08
  • FWIW, if we give `B` a virtual function (i.e., making it polymorphic), clang's UB sanitizer will error on this code: http://coliru.stacked-crooked.com/a/df0100a0248695a4 – T.C. Mar 08 '15 at 17:10
  • This code is idiotic. Why would anyone think this to be a good idea?? – Lightness Races in Orbit Mar 08 '15 at 18:21

3 Answers3

5

[expr.static.cast]/p11:

A prvalue of type “pointer to cv1 B,” where B is a class type, can be converted to a prvalue of type “pointer to cv2 D,” where D is a class derived (Clause 10) from B, if a valid standard conversion from “pointer to D” to “pointer to B” exists (4.10), cv2 is the same cv-qualification as, or greater cv-qualification than, cv1, and B is neither a virtual base class of D nor a base class of a virtual base class of D. The null pointer value (4.10) is converted to the null pointer value of the destination type. If the prvalue of type “pointer to cv1 B” points to a B that is actually a subobject of an object of type D, the resulting pointer points to the enclosing object of type D. Otherwise, the behavior is undefined.

The question, then, is whether, at the time of the static_cast, the pointer actually points to "a B that is actually a subobject of an object of type D". If so, there is no UB; if not, then the behavior is undefined whether or not the resulting pointer is dereferenced or otherwise used.

[class.dtor]/p15 says that (emphasis mine)

Once a destructor is invoked for an object, the object no longer exists

and [basic.life]/p1 says that

The lifetime of an object of type T ends when:

  • if T is a class type with a non-trivial destructor (12.4), the destructor call starts, or
  • [...]

From this, then, the D object's lifetime has ended as soon as its destructor is invoked, and certainly by the time B's destructor began to execute - which is after D's destructor body has finished execution. At this point, there is no "object of type D" left that this B can be a subobject of - it "no longer exists". Thus, you have UB.

Clang with UBsan will report an error on this code if B is made polymorphic (given a virtual function), which supports this reading.

T.C.
  • 133,968
  • 17
  • 288
  • 421
  • "if a valid standard conversion from “pointer to D” to “pointer to B” exists" - please refer to 12.7/3: which explains that this pointer conversion is valid as long as "destruction of these classes shall not have **completed**" and UB otherwhise. As here desturction of D is completed and the one of B has started, it's definitively UB ! – Christophe Mar 08 '15 at 19:04
  • @Christophe “pointer to D” and “pointer to B” are types, and that sentence is dealing with types, not values. A "valid standard conversion from “pointer to D” to “pointer to B” exists" as long as B is an accessible and unambiguous base of `D` (see [conv.ptr]/p3). – T.C. Mar 08 '15 at 19:08
  • For my own understanding: If the object does no more exist after the destructor has startet, why am I allowed to call any clean-up function within it? – Kit Fisto Mar 09 '15 at 15:36
  • @KitFisto There's a paragraph in the standard ([class.cdtor]/p4) that explicitly allows member function calls during construction or destruction. – T.C. Mar 09 '15 at 16:48
  • Thanks, T.C. that is what I was thinking too. I was hoping to find a clear & compelling definition of "subobject" relevant to this context since that seems to be the key term as you point out. – Chris Morley Mar 09 '15 at 18:59
  • @T.C: Thanks, that's what I was missing. – Kit Fisto Mar 10 '15 at 10:37
2

Apparently your co-worker is under the impression that as long as you do not dereference an invalid pointer, you are fine.

He is wrong.

Merely evaluating such a pointer has undefined behaviour. This code is obviously broken.

Lightness Races in Orbit
  • 378,754
  • 76
  • 643
  • 1,055
  • It's implementation-defined, not undefined, but the implementation-defined behaviour has a footnote attached that says "Some implementations might define that copying an invalid pointer value causes a system-generated runtime fault." However, the pointer doesn't become invalid until after the deallocation function runs. Pointers to uninitialised storage remain valid pointers, and the storage remains available until the deallocation function runs. I think T.C.'s answer gives a more convincing argument that the code nonetheless has UB. –  Mar 08 '15 at 20:21
0

You should definitively tell him that it's UB ! !

Why ?

12.4/7: Bases and members are destroyed in the reverse order of the completion of their constructor The objects are detroyed in the reverse order of their constuction.

12.6.2/10: First (...) virtual base classes are initialized (...) then, direct base classes are initialized

So when destructing a D, first the D members are destructed and then the D sub object, and only then will B be destructed.

This code makes sure that f() is called when the B object is destroyed: 

 ~B() { f(this); }

So when a D object is destroyed, the D suboject is destroyed first, and then ~B() is executed, calling f().

In f() you cast the pointer to a B as pointer to a D. This is UB:

3.8/5: (...) after the lifetime of an object has ended and before the storage which the object occupied is reused or released, any pointer that refers to the storage location where the object will be or was located may be used but only in limited ways. (...) The program has undefined behavior if the pointer is used to access a non-static data member or call a non-static member function of the object, or (...) the pointer is used as the operand of a static_cast.

Christophe
  • 68,716
  • 7
  • 72
  • 138
  • 1
    "static casting a pointer will not touch the object itself. So this statement alone is not UB in the sence that nothing undefined will happen immediately." No, a `static_cast` can itself be UB. – T.C. Mar 08 '15 at 18:06
  • Indeed, a `static_cast` on something that is not really of that type is UB. `reinterpret_cast` would be OK, as long as it's not dereferenced. – Jonathan Wakely Mar 08 '15 at 18:27
  • 1
    @T.C. Ooops ! You're completely right ! I thought that only the derefencing of suc a casted poitner was UB, but following your remark, I've found a quote of the standard that explicitely addresses the static_casting after end of life of object. I edited the post: no longer any doubt: that's UB ! Thanks for the hint ! – Christophe Mar 08 '15 at 18:33
  • I'm doubtful about the 3.8/5 quote because that paragraph explicitly excludes "an object under construction or destruction". – T.C. Mar 08 '15 at 18:35
  • @JonathanWakely in fact T.C. was right: the static_cast is UB except if the cast is to `void*`, `char*` or `unsigned char*` – Christophe Mar 08 '15 at 18:37
  • @T.C: It's not clear how much of the rest of the paragraph is discounted by trhat – Lightness Races in Orbit Mar 08 '15 at 18:37
  • @LightnessRacesinOrbit Well, you are certainly allowed to access your own non-static data members in a destructor body, even though the object's lifetime has ended by then. – T.C. Mar 08 '15 at 18:39
  • @Christophe, yes, I know, that's why I agreed with T.C. (and upvoted the comment) – Jonathan Wakely Mar 08 '15 at 18:48
  • @T.C. I agree with *Lightness Races in Orbit* : for the exlusion refers to 12.7 which addresses some special cases, but doesn't invalidate what I quoted. The last sentence of 12.7/3 tend even to reinforces it : the simple fact forming a pointer to a member after the object was destroyed is already UB. – Christophe Mar 08 '15 at 18:54
  • @T.C.: "In the destructor body" and "after destruction" are two completely different things. Though yes I acknowledge that the object's lifetime has already ended in both cases. – Lightness Races in Orbit Mar 08 '15 at 18:56
  • @Christophe I disagree; "For X, see Y; otherwise Z" implies that Z doesn't apply in X. Remember that in the body of `~D()`, `D`'s lifetime has already ended. `~D() { delete this->D::ptr;}` is obviously valid, which implies that the bullets in 3.8/5 cannot possibly apply to the "object under construction and destruction" case; instead the rules in 12.7 apply. – T.C. Mar 08 '15 at 19:01
  • @LightnessRacesinOrbit I honestly like that argument, but annoyingly `D`'s destructor doesn't technically finish execution until all the members and bases have been destroyed. Their destructors are specified to be called by the destructor of `D` ([class.dtor]/p8), so until those calls finishes `D`'s destructor is still executing. – T.C. Mar 08 '15 at 19:04
  • @LightnessRacesinOrbit There's no "direct non-static member" here...? – T.C. Mar 08 '15 at 19:11
  • 1
    @T.C. Whoops ;) Right well then there are no semantics defined for this so it's UB. – Lightness Races in Orbit Mar 08 '15 at 19:18