39 ms·

Catching use-after-move C++ bugs with Clang's consumed annotations

175 points by akling 7y ago

pjmlp 7y ago
Cool idea.
saagarjha 7y ago
Looks like a lightweight borrow checker, although I wonder how well it fares in places where lifetimes are difficult to track. Or is there a way to annotate methods with this information as well?
- cesarb 7y ago
  To me, it looked more like Rust's move semantics: in Rust, when an object is moved it's "consumed" and cannot be used anymore. The borrow checker is for when the object is not "consumed", only temporarily borrowed by some other code.
  - saagarjha 7y ago
    I'm using the term "borrow checker" to encompass Rust's whole memory model, but yes: this only seems to provide information about when something's been "moved" (or "dropped") rather than "borrowed" in the temporary sense.
- twic 7y ago
  Or maybe a heavyweight one. I note that the clang annotations refer to "typestate", which was a mechanism that existed in primordial Rust, but which was dropped because it was unnecessarily complicated: http://pcwalton.github.io/2012/12/26/typestate-is-dead.html http://pcwalton.github.io/2012/12/26/typestate-is-dead.html Although to be fair, clang's version is a highly limited particular case of typestate, with one axis of state with three values, rather than a general heavyweight typestate mechanism.
haberman 7y ago
Why isn't a moved-from object always considered "consumed"?
- jdsully 7y ago
  Because its still a legal and valid object according to the standard. A moved from vector might get reused to store new things for example.
  - ndesaulniers 7y ago
    Are you sure? I thought a moved from object was left in an indeterminate state and further use was undefined behavior. Move constructors and move assignment operators that zero out the rhs members try to prevent duplicate references from an object likely to be destructed soon, which might lead to dangling pointers and then use after free in lhs. Can someone cite the spec and prove me wrong, please?
    - jdsully 7y ago
      A move constructor is supposed to leave the donor object in a valid state. All STL objects with one will do so. If you write a poor move constructor that doesn’t do this you will get exactly what you asked for. However the compiler has no way to know you half assed it and must support a legal valid case. See this stack overflow: https://stackoverflow.com/questions/9168823/reusing-a-moved-container https://stackoverflow.com/questions/9168823/reusing-a-moved-... To quote office space, “Your moved from object only needs to be destructible. But some people choose to allow more and we encourage that.”
      - comex 7y ago
        "if (x = y)" is also a legal valid use case (it sets x to y, and then tests whether it's nonzero), but that doesn't stop compilers from complaining if you write it, on the grounds that it's most likely a mistake. They should do the same with use-after-move. There could then be some workaround or annotation to disable the warning if you really did want a use-after-move, as there is for the warning I mentioned (adding extra parentheses).
gumby 7y ago
Very nice!
innot 7y ago
> Once you std::move an object and pass it to someone who takes an rvalue-reference, your local object may very well end up in an invalid state. As far as I remember, move constructors/assignments must leave the moved-from object in a valid state - just that the standard doesn't say anything about what that state is for standard classes. Also, I have seen code where some kind of "result" can be moved from an object and then re-generated from scratch with different inputs. In that case it was perfectly valid to use it after move. But that's nitpicking, anyways.
- foota 7y ago
  I believe the standard says that move does "valid but unspecified" for standard library objects, but does not generally guarantee that moved from objects must be valid.
  - ori_b 7y ago
    The destructor gets called on them. They need to be valid enough for that.
    - foota 7y ago
      Hm, technically don't think this would be required. Take for instance: auto no_destroy = new MoveNoDestroy(); MoveNoDestroy* moved_into; *moved_into = std::move(*no_destroy); Wouldn't call the destructor of the moved from MoveNoDestroy.
      - 0xffff2 7y ago
        Yes, if you leak resources, their destructors won't be called. That really has nothing at all to do with move semantics though. I think the important point is that move semantics don't alter the lifetime management of the moved-from object.
- hermitdev 7y ago
  Yes, valid, but unspecified. Typically what I've seen is that a move operation is effectively a swap between the source and destination. I've also seen where a move leaves the source in effectively a default constructed state.
matthewbauer 7y ago
Perhaps the future of software isn't "rewrite everything in Rust", but instead we end up annotating existing C/C++ with borrow checking information. This would be similar to how there is a push in JavaScript to add TypeScript annotations everywhere. It gives you the flexibility of the existing language and ecosystem while still allowing you to use new techniques in programming languages.
- swsieber 7y ago
  I think we'll c/c++ increasingly use those (in addition to the other wonderful tooling it has). I think Rust still has a strong future though - it has safety by default, and I really like it apart from the borrow checker. That said, I like this push for more programmatic checks.
  - ohazi 7y ago
    > we'll c/c++ increasingly use those We'll c/c++ increasingly use We'll see c++ increasingly use My brain. It's too early for puns. So good though.
    - swsieber 7y ago
      I'll just act like I meant to make that pun. I guess it was too early for me too. Good catch.
- rubber_duck 7y ago
  My problem with C++ isn't the lack of borrow checker - this is the feature I like the least in Rust (I know it's their core design goal but frankly the inconvenience and limitations it imposes don't seem worth it for my use case, and then theres the compile times). C++ lack of modules and package management on the other hand is a huge PITA and I'm not optimistic either of those bolted on so late in to the language lifecycle will provide a useful solution. It's a pity D took it too far in the other direction with GC and runtime - I really could use a C with classes and modules.
  - 7y ago
rsp1984 7y ago
I've been writing C++ for 21 years now (started when I was 14). To be honest, I have never seen a solid case where move semantics provided added value (in terms of code readability and maintainability) over just passing object references as function parameters. That big ugly object that would get copied on function return -- just create it before the function call and pass it in as a reference! No copy required.
- namirez 7y ago
  Move semantics is the language support for shallow copies. If you never use shallow copies, fine! But a lot of code bases use shallow copies, and it's a valuable tool.
  - Someone 7y ago
    Move semantics moves objects, it doesn’t copy them. For example, if you have a class C containing a pointer P to some data, where C’s destructor frees that pointer: - a shallow copy of object O would return an object O2, containing that same pointer P, and leave O unmodified. - a move of O to O2 would (1) make O2 contain P, but also would update O to no longer have that pointer P (it has to make that change, as destructing O at a later time shouldn’t free P anymore) (1) yes, an implementation could also copy the data or increase a reference count, or, possibly, a zillion different things without running into problems.
    - namirez 7y ago
      Shallow copy is the general concept. Move semantics is a special case where the unique ownership is enforced. Here is simple example of the idea: Widget::Widget(Widget&& w) { this->ptr = w.ptr; // shallow copy w.ptr = nullptr; // nullify the source }
      - clappski 7y ago
        You could use; auto* p2 = std::exchange( p1, nullptr );
sudeepj 7y ago
With C++ even if your project follows this, there is no way to enforce this across your project's dependencies & wider eco-system. With Rust, its entire ecosystem (however nascent) is subjected to the same strict rules. This is big plus when Rust eco-system matures functionality wise.
- akling 7y ago
  My project doesn’t have this issue ;) https://github.com/SerenityOS/serenity https://github.com/SerenityOS/serenity
  - nerdponx 7y ago
    This looks great! I'm going to try this out over the weekend.
  - logicprog 7y ago
    Wait, we enforce borrow checking rules on SOS?
    - akling 7y ago
      At the moment it only shows up as an on-screen warning in my Qt Creator thanks to its clang integration. The default Serenity toolchain is using GCC-8.3.0 which doesn't support this trick at all, so we don't get any compile-time enforcement. :( I keep thinking about switching over to a clang toolchain to be able to do more stuff like this.
- tfha 7y ago
  Sure you can. Just reject any dependencies that don't pass your own linter. By switching to rust, you are doing the same thing anyway.
  - dpc_pw 7y ago
    But with Rust you will get a whole ecosystem working. In your method you will get 0 dependencies working, no?

175 points by akling 7y ago

Cool idea.

Looks like a lightweight borrow checker, although I wonder how well it fares in places where lifetimes are difficult to track. Or is there a way to annotate methods with this information as well?

cesarb 7y ago
To me, it looked more like Rust's move semantics: in Rust, when an object is moved it's "consumed" and cannot be used anymore. The borrow checker is for when the object is not "consumed", only temporarily borrowed by some other code.
- saagarjha 7y ago
  I'm using the term "borrow checker" to encompass Rust's whole memory model, but yes: this only seems to provide information about when something's been "moved" (or "dropped") rather than "borrowed" in the temporary sense.
twic 7y ago
Or maybe a heavyweight one. I note that the clang annotations refer to "typestate", which was a mechanism that existed in primordial Rust, but which was dropped because it was unnecessarily complicated: http://pcwalton.github.io/2012/12/26/typestate-is-dead.html http://pcwalton.github.io/2012/12/26/typestate-is-dead.html Although to be fair, clang's version is a highly limited particular case of typestate, with one axis of state with three values, rather than a general heavyweight typestate mechanism.

haberman 7y ago

Why isn't a moved-from object always considered "consumed"?

jdsully 7y ago
Because its still a legal and valid object according to the standard. A moved from vector might get reused to store new things for example.
- ndesaulniers 7y ago
  Are you sure? I thought a moved from object was left in an indeterminate state and further use was undefined behavior. Move constructors and move assignment operators that zero out the rhs members try to prevent duplicate references from an object likely to be destructed soon, which might lead to dangling pointers and then use after free in lhs. Can someone cite the spec and prove me wrong, please?
  - jdsully 7y ago
    A move constructor is supposed to leave the donor object in a valid state. All STL objects with one will do so. If you write a poor move constructor that doesn’t do this you will get exactly what you asked for. However the compiler has no way to know you half assed it and must support a legal valid case. See this stack overflow: https://stackoverflow.com/questions/9168823/reusing-a-moved-container https://stackoverflow.com/questions/9168823/reusing-a-moved-... To quote office space, “Your moved from object only needs to be destructible. But some people choose to allow more and we encourage that.”
    - comex 7y ago
      "if (x = y)" is also a legal valid use case (it sets x to y, and then tests whether it's nonzero), but that doesn't stop compilers from complaining if you write it, on the grounds that it's most likely a mistake. They should do the same with use-after-move. There could then be some workaround or annotation to disable the warning if you really did want a use-after-move, as there is for the warning I mentioned (adding extra parentheses).

gumby 7y ago

Very nice!

innot 7y ago

> Once you std::move an object and pass it to someone who takes an rvalue-reference, your local object may very well end up in an invalid state. As far as I remember, move constructors/assignments must leave the moved-from object in a valid state - just that the standard doesn't say anything about what that state is for standard classes. Also, I have seen code where some kind of "result" can be moved from an object and then re-generated from scratch with different inputs. In that case it was perfectly valid to use it after move. But that's nitpicking, anyways.

foota 7y ago
I believe the standard says that move does "valid but unspecified" for standard library objects, but does not generally guarantee that moved from objects must be valid.
- ori_b 7y ago
  The destructor gets called on them. They need to be valid enough for that.
  - foota 7y ago
    Hm, technically don't think this would be required. Take for instance: auto no_destroy = new MoveNoDestroy(); MoveNoDestroy* moved_into; *moved_into = std::move(*no_destroy); Wouldn't call the destructor of the moved from MoveNoDestroy.
    - 0xffff2 7y ago
      Yes, if you leak resources, their destructors won't be called. That really has nothing at all to do with move semantics though. I think the important point is that move semantics don't alter the lifetime management of the moved-from object.
hermitdev 7y ago
Yes, valid, but unspecified. Typically what I've seen is that a move operation is effectively a swap between the source and destination. I've also seen where a move leaves the source in effectively a default constructed state.

matthewbauer 7y ago

Perhaps the future of software isn't "rewrite everything in Rust", but instead we end up annotating existing C/C++ with borrow checking information. This would be similar to how there is a push in JavaScript to add TypeScript annotations everywhere. It gives you the flexibility of the existing language and ecosystem while still allowing you to use new techniques in programming languages.

swsieber 7y ago
I think we'll c/c++ increasingly use those (in addition to the other wonderful tooling it has). I think Rust still has a strong future though - it has safety by default, and I really like it apart from the borrow checker. That said, I like this push for more programmatic checks.
- ohazi 7y ago
  > we'll c/c++ increasingly use those We'll c/c++ increasingly use We'll see c++ increasingly use My brain. It's too early for puns. So good though.
  - swsieber 7y ago
    I'll just act like I meant to make that pun. I guess it was too early for me too. Good catch.
rubber_duck 7y ago
My problem with C++ isn't the lack of borrow checker - this is the feature I like the least in Rust (I know it's their core design goal but frankly the inconvenience and limitations it imposes don't seem worth it for my use case, and then theres the compile times). C++ lack of modules and package management on the other hand is a huge PITA and I'm not optimistic either of those bolted on so late in to the language lifecycle will provide a useful solution. It's a pity D took it too far in the other direction with GC and runtime - I really could use a C with classes and modules.
- 7y ago

rsp1984 7y ago

I've been writing C++ for 21 years now (started when I was 14). To be honest, I have never seen a solid case where move semantics provided added value (in terms of code readability and maintainability) over just passing object references as function parameters. That big ugly object that would get copied on function return -- just create it before the function call and pass it in as a reference! No copy required.

namirez 7y ago
Move semantics is the language support for shallow copies. If you never use shallow copies, fine! But a lot of code bases use shallow copies, and it's a valuable tool.
- Someone 7y ago
  Move semantics moves objects, it doesn’t copy them. For example, if you have a class C containing a pointer P to some data, where C’s destructor frees that pointer: - a shallow copy of object O would return an object O2, containing that same pointer P, and leave O unmodified. - a move of O to O2 would (1) make O2 contain P, but also would update O to no longer have that pointer P (it has to make that change, as destructing O at a later time shouldn’t free P anymore) (1) yes, an implementation could also copy the data or increase a reference count, or, possibly, a zillion different things without running into problems.
  - namirez 7y ago
    Shallow copy is the general concept. Move semantics is a special case where the unique ownership is enforced. Here is simple example of the idea: Widget::Widget(Widget&& w) { this->ptr = w.ptr; // shallow copy w.ptr = nullptr; // nullify the source }
    - clappski 7y ago
      You could use; auto* p2 = std::exchange( p1, nullptr );

sudeepj 7y ago

With C++ even if your project follows this, there is no way to enforce this across your project's dependencies & wider eco-system. With Rust, its entire ecosystem (however nascent) is subjected to the same strict rules. This is big plus when Rust eco-system matures functionality wise.

akling 7y ago
My project doesn’t have this issue ;) https://github.com/SerenityOS/serenity https://github.com/SerenityOS/serenity
- nerdponx 7y ago
  This looks great! I'm going to try this out over the weekend.
- logicprog 7y ago
  Wait, we enforce borrow checking rules on SOS?
  - akling 7y ago
    At the moment it only shows up as an on-screen warning in my Qt Creator thanks to its clang integration. The default Serenity toolchain is using GCC-8.3.0 which doesn't support this trick at all, so we don't get any compile-time enforcement. :( I keep thinking about switching over to a clang toolchain to be able to do more stuff like this.
tfha 7y ago
Sure you can. Just reject any dependencies that don't pass your own linter. By switching to rust, you are doing the same thing anyway.
- dpc_pw 7y ago
  But with Rust you will get a whole ecosystem working. In your method you will get 0 dependencies working, no?