3 ms·

The article says you have to clone the vector, as if it's not completely normal to have one object hold a reference to another, "unsafely," in C++, without the

The article says you have to clone the vector, as if it's not completely normal to have one object hold a reference to another, "unsafely," in C++, without the world coming to an end. Having one object contain a reference or pointer to another in C++ is completely normal and something you certainly would "dream" of doing -- it happens all the time, you can find it everywhere in the STL, so I don't know what the blog author is getting at. Looking at the diff, the type SubstructureFields already has the lifetime parameter on it, and making FieldInfo, which is only used as a field in the SubstructureFields type (which owns its FieldInfo objects), be subject to the same lifetime constraint, is something that would be perfectly ordinary C++ and it registers near the bottom of the use-after-free risk scale.

pcwalton 12y ago
> Looking at the diff, the type SubstructureFields already has the lifetime parameter on it, and making FieldInfo, which is only used as a field in the SubstructureFields type (which owns its FieldInfo objects), be subject to the same lifetime constraint, is something that would be perfectly ordinary C++ and it registers near the bottom of the use-after-free risk scale. Which is why it works. But isn't it nice when the compiler is aware of the "use-after-free risk scale" for your data structures?
- SamReidHughes 12y ago
  I prefer technical accuracy in technical blog posts. If you want somebody to preach at about the merits of compiler-checked safety, it's not me.
  - Manishearth 12y ago
    It was accurate. See my comment below -- just because `SubstructureFields` had a lifetime parameter already doesn't mean it was borrowing data of a similar lifetime. That data being borrowed could probably live longer than the attributes, but fortunately the attributes live long enough that I can equate the lifetimes. I was able to use the same lifetime parameter by luck -- there was a good chance it wouldn't have compiled and I would have had to introduce another lifetime. (Though since this was a fix the compiler suggested I didn't need to worry too much about that happening -- generally those just work) Just having a lifetime doesn't mean that it's safe to put any random borrowed data in. In C++ we could have a single pointer to a very long-lived struct ("SubstructureFields"), and wish to introduce another struct ("FieldInfo") which contains a pointer to something that is shorter lived. Note that in large codebases knowing which is "longer lived" is not easy, so from the programmer's perspective there are just two pointers. Assuming that "Okay, we don't have any segfaults now due to the first pointer, introducing the second FieldInfo pointer should be fine then" would be fallacious -- we might be accessing data during a period of time when the first, original pointer is alive, but the second is invalidated. Use after free.
    - SamReidHughes 12y ago
      I'm trying to look more closely at the situation and will compose an answer, hopefully soon, but after going to Staples to get a $50 DVI cable so I can plug this workstation into a monitor (I just moved) and running make -j33 to build the rust compiler and play around with it, it's been thirty minutes and this is what I see: https://i.imgur.com/2JL66Fg.png https://i.imgur.com/2JL66Fg.png so bear with me. Update: You don't have to wade through a lot of code at all. All the code is taking AST parameters that are the source of these Attribute vectors by const reference and returning stuff like a P<Expr> and P<Item> and the like, AST stuff that by its nature doesn't have references with tricky lifetime dependencies to other far-flung AST stuff. So in C++ you can see right from the type signatures (and some basic institutional knowledge) that you're OK. You can see that the lifetimes are not long or dangerous. SubstructureFields is used in Substructure. A simple grep for that type shows a bunch of functions that take a Substructure by const reference and return a P<Expr>. There is nothing holding wiggly little references to Substructure objects or the like.
      - pcwalton 12y ago
        You can easily write a function in C++ that takes an object by const reference and creates a dangling reference without returning it. Such functions regularly result in use-after-free (sometimes causing security vulnerabilities) in the wild. Running grep on the signatures of functions is not a sound analysis.
        SamReidHughes 12y ago
        You can pretty well see by grepping whether it's going to create a dangling reference or not. For example, in C++ codebases I'm most familiar with, anything that takes a const foo& isn't going to retain a reference, and for unfamiliar stuff that takes a const foo* or foo*, you can look at the outputs, which, if it's a P<Expr>, all you need is the institutional knowledge that Exprs don't have dangling references. You don't need to debate with me the merits of replacing visual analysis with sound analysis. (If I have a negative opinion of Rust on the matter, it's that it's not good enough at that.) My beef here is with the way the blog article overstates the case, saying you just wouldn't do this sort of thing or this specific thing. You so would make temporary references deep into an AST while expanding derived implementations.
Manishearth 12y ago
Oh, the general situation is something that might be common in C++, but in this specific situation -- the vector was created and held (and most probably being mutated) somewhere far away, and I have no clue what is being done with it -- and the relevant portion of the codebase is huge -- holding a reference is something I wouldn't dream of doing. SubstructureFields could have had a lifetime parameter of something that was supposed to live longer, we can't be easily sure.
- SamReidHughes 12y ago
  You can wrap the vector parameter or pointer/size parameter in a type, chase the compilation errors up to the places where the original vector exists, and make sure the usages there be well-protected.
  - Manishearth 12y ago
    That original vector could be used in a bajillion places all over the codebase (hint: It is. It's central to the AST representation in this case and is used everywhere). I'd be wading through a lot of code which doesn't even come close to the expansion phase. This trick doesn't let me track the flow of a program to find out which portions of code will be run while my pointer is active. Tracking the flow only happens in Rust.
    - SamReidHughes 12y ago
      So it turns out you wouldn't be wading through a lot of code. Especially when it's in an AST, which, even if it's the sort of compiler that mutably updates its AST, probably isn't going to have surprise updates to source-position-tied attributes at macro-expansion time. Especially not when you have some basic const correctness to convince you of this in C++ (the equivalent of what we're seeing in this Rust code), but even in C and without const safety, and with a compiler that practices mutating an AST (with metadata) in-place, this would be a comfortable thing to do. (In C there'd be a clearly named function that adds "internal" attributes and you'd see its existence and where it's getting called, while in C++ if you didn't have type safety you'd make the field private, give the functions modifying it funny names, and grep for them.)

pcwalton 12y ago

> Looking at the diff, the type SubstructureFields already has the lifetime parameter on it, and making FieldInfo, which is only used as a field in the SubstructureFields type (which owns its FieldInfo objects), be subject to the same lifetime constraint, is something that would be perfectly ordinary C++ and it registers near the bottom of the use-after-free risk scale. Which is why it works. But isn't it nice when the compiler is aware of the "use-after-free risk scale" for your data structures?

SamReidHughes 12y ago
I prefer technical accuracy in technical blog posts. If you want somebody to preach at about the merits of compiler-checked safety, it's not me.
- Manishearth 12y ago
  It was accurate. See my comment below -- just because `SubstructureFields` had a lifetime parameter already doesn't mean it was borrowing data of a similar lifetime. That data being borrowed could probably live longer than the attributes, but fortunately the attributes live long enough that I can equate the lifetimes. I was able to use the same lifetime parameter by luck -- there was a good chance it wouldn't have compiled and I would have had to introduce another lifetime. (Though since this was a fix the compiler suggested I didn't need to worry too much about that happening -- generally those just work) Just having a lifetime doesn't mean that it's safe to put any random borrowed data in. In C++ we could have a single pointer to a very long-lived struct ("SubstructureFields"), and wish to introduce another struct ("FieldInfo") which contains a pointer to something that is shorter lived. Note that in large codebases knowing which is "longer lived" is not easy, so from the programmer's perspective there are just two pointers. Assuming that "Okay, we don't have any segfaults now due to the first pointer, introducing the second FieldInfo pointer should be fine then" would be fallacious -- we might be accessing data during a period of time when the first, original pointer is alive, but the second is invalidated. Use after free.
  - SamReidHughes 12y ago
    I'm trying to look more closely at the situation and will compose an answer, hopefully soon, but after going to Staples to get a $50 DVI cable so I can plug this workstation into a monitor (I just moved) and running make -j33 to build the rust compiler and play around with it, it's been thirty minutes and this is what I see: https://i.imgur.com/2JL66Fg.png https://i.imgur.com/2JL66Fg.png so bear with me. Update: You don't have to wade through a lot of code at all. All the code is taking AST parameters that are the source of these Attribute vectors by const reference and returning stuff like a P<Expr> and P<Item> and the like, AST stuff that by its nature doesn't have references with tricky lifetime dependencies to other far-flung AST stuff. So in C++ you can see right from the type signatures (and some basic institutional knowledge) that you're OK. You can see that the lifetimes are not long or dangerous. SubstructureFields is used in Substructure. A simple grep for that type shows a bunch of functions that take a Substructure by const reference and return a P<Expr>. There is nothing holding wiggly little references to Substructure objects or the like.
    - pcwalton 12y ago
      You can easily write a function in C++ that takes an object by const reference and creates a dangling reference without returning it. Such functions regularly result in use-after-free (sometimes causing security vulnerabilities) in the wild. Running grep on the signatures of functions is not a sound analysis.
      - SamReidHughes 12y ago
        You can pretty well see by grepping whether it's going to create a dangling reference or not. For example, in C++ codebases I'm most familiar with, anything that takes a const foo& isn't going to retain a reference, and for unfamiliar stuff that takes a const foo* or foo*, you can look at the outputs, which, if it's a P<Expr>, all you need is the institutional knowledge that Exprs don't have dangling references. You don't need to debate with me the merits of replacing visual analysis with sound analysis. (If I have a negative opinion of Rust on the matter, it's that it's not good enough at that.) My beef here is with the way the blog article overstates the case, saying you just wouldn't do this sort of thing or this specific thing. You so would make temporary references deep into an AST while expanding derived implementations.

Manishearth 12y ago

Oh, the general situation is something that might be common in C++, but in this specific situation -- the vector was created and held (and most probably being mutated) somewhere far away, and I have no clue what is being done with it -- and the relevant portion of the codebase is huge -- holding a reference is something I wouldn't dream of doing. SubstructureFields could have had a lifetime parameter of something that was supposed to live longer, we can't be easily sure.

SamReidHughes 12y ago
You can wrap the vector parameter or pointer/size parameter in a type, chase the compilation errors up to the places where the original vector exists, and make sure the usages there be well-protected.
- Manishearth 12y ago
  That original vector could be used in a bajillion places all over the codebase (hint: It is. It's central to the AST representation in this case and is used everywhere). I'd be wading through a lot of code which doesn't even come close to the expansion phase. This trick doesn't let me track the flow of a program to find out which portions of code will be run while my pointer is active. Tracking the flow only happens in Rust.
  - SamReidHughes 12y ago
    So it turns out you wouldn't be wading through a lot of code. Especially when it's in an AST, which, even if it's the sort of compiler that mutably updates its AST, probably isn't going to have surprise updates to source-position-tied attributes at macro-expansion time. Especially not when you have some basic const correctness to convince you of this in C++ (the equivalent of what we're seeing in this Rust code), but even in C and without const safety, and with a compiler that practices mutating an AST (with metadata) in-place, this would be a comfortable thing to do. (In C there'd be a clearly named function that adds "internal" attributes and you'd see its existence and where it's getting called, while in C++ if you didn't have type safety you'd make the field private, give the functions modifying it funny names, and grep for them.)