10 ms·

So, and single line of code change? This is why the language choice is not the biggest issue here. Keeping all the necessary state in a programmer's head is the

So, and single line of code change? This is why the language choice is not the biggest issue here. Keeping all the necessary state in a programmer's head is the real issue. Using <insert fav lang>Rust</> would not have helped. bio->bi_partno = bio_src->bi_partno; https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=62530ed8b1d https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/lin...

khuey 9y ago
A language with copy constructors could have prevented this.
- std_throwaway 9y ago
  C++ would be a logical language upgrade for the kernel.
  - MBCook 9y ago
    Why is that?
  - cdmckay 9y ago
    Read Linus' rant about why the Linux kernel shouldn't use C++: https://lwn.net/Articles/249460/ https://lwn.net/Articles/249460/
    - steveklabnik 9y ago
      Thu, 6 Sep 2007 18:50:28 +0100 (BST) C++ has changed quite a bit in the last 10 years.
      - iforgotpassword 9y ago
        Yep, adding more and more bs so it's impossible to fully understand it ever. Understanding C thoroughly is already hard, but C++ is a whole new level. "But then only use the new and modern parts of it" No! You cannot simply ignore all the complicated cruft just because you don't like it because sooner or later it will come and bite you in the ass.
    - deathanatos 9y ago
      > [C++ is] made more horrible by the fact that a lot of substandard programmers use it Typical Linus rant. There's no substantive, logical, reasoned argument in there about why C++ would be inappropriate. The closest we get is that "You invariably start using the "nice" library features of the language like STL" — God forbid the language provide a dynamically sized array, and not force you to implement it manually. Not saying that Linux should switch from C to C++, nor that doing so wouldn't be a metric crap ton of work (it would be) and for benefits that might be hard to sell (e.g., at this point, we already have a decent vector implemented in C macros; what does switching actually gain us?) or that the kernel doesn't have special concerns (in a kernel, you have to implement malloc(), for example (and that's probably a gross simplification) and that could definitely affect a lot of things in the STL), etc. Just that the linked rant is not valid argument as to why C++ isn't a good fit.
      - hermitdev 9y ago
        Indeed. There is plenty evidence that there are a ton of shit C programmers out there, as well. It's not very hard to take a random OSS C (or C++) project and find bugs or potential bugs in the form of undefined or unspecified behavior. Correctly performing signed integer arithmetic without introducing undefined behavior is surprisingly hard. Additionally, most people never bother to do it correctly in either C or C++ because, well, it's a pain in the ass and it slows the code down. Linus's argument against C++ is basically akin to Dijkstra's argument against `goto`. Like any tool, it can be abused. But they've seen too many abuses for their own taste, so they ban the tool completely despite the advantages the tool may have.
      - koverstreet 9y ago
        > [C++ is] made more horrible by the fact that a lot of substandard programmers use it Thing is, it's a very real issue. C++ as written by the best of programmers might be a very reasonable language for the kernel, but in practice what happens is the additional abstraction and indirection mechanisms mean it becomes very hard to decipher with any confidence code written by mediocre or merely average programmers. And this is a real issue in practice, I saw it when I was at Google with code written by other programmers at Google, and honestly the majority of the code in the Linux kernel is at about the same level - some of it really good elegant code, more of it ugly and hacky but working, and a lot more (especially driver code) mediocre crap. And when having to decipher and refactor the mediocre crap - because let's be honest, that's the majority of the code - I'd much rather have to deal with C than C++. The reason this is such an issue with C++ is that - and it's been said before, but it bears repeating - C++ adds a lot of new ways to shoot yourself in the foot and it doesn't take any of the old ones away. With C, you can generally read a chunk of code and have confidence that it does what it appears to do - you can understand it in isolation. With C++, just figuring out the flow control can be a nightmare. Rust would be a different story. I am 100% in favor of using Rust whenever possible - it has some of the same issues as C++ or any other polymorphic language in that figuring out flow control can really suck, but it provides much stronger guarantees w.r.t. how code can screw you over that make it well worth it. C++... not so much.
- bonzini 9y ago
  A language with copy constructors doesn't prevent you from putting a bug in them. C++ or Rust might have put a 0 in the uninitialized member, but the bug would have remained.
  - steveklabnik 9y ago
    Rust doens't have copy constructors, to be clear.
  - masklinn 9y ago
    > A language with copy constructors doesn't prevent you from putting a bug in them. If you don't override it, it will copy everything properly. > Rust might have put a 0 in the uninitialized member Rust does not do that. If a member is not initialised, it's uninitialised and in this case the structure will be rejected. Here even if you could not just have derived Clone (struct bio looks non-trivial) your hand-rolled implementation would have complained that you had not properly initialised the member.
    - bonzini 9y ago
      > If you don't override it, it will copy everything properly. Sure, so does memcpy or bare struct assignment in C. The problem is that trivial copy constructors are relatively rare. > Here even if you could not just have derived Clone (struct bio looks non-trivial) your hand-rolled implementation would have complained that you had not properly initialised the member. Right, but this is more similar to an assignment operator. DerefMut in Rust I think wouldn't have caught the bug.
      - masklinn 9y ago
        > Right, but this is more similar to an assignment operator. Only if you don't zoom out. If you do, this is alloc/init/fill which in C++ or Rust would use a regular copy and RVO.
- barrkel 9y ago
  A language with copy constructors hides user defined code inside seemingly innocuous expressions, for great hilarity when they have side effects or other unexpected semantics.
icebraining 9y ago
Language definitively could have helped. For example, you could have a type that could only be created with both disk and partition info filled in. Then the compiler would complain if you forgot one of them.
- iforgotpassword 9y ago
  The field was set to 0 by a function it was passed to previously.. It didn't do that in older kernel versions so it kept the proper value, so a classical miscommunication (or none at all) between devs.
  - remar 9y ago
    This sounds exactly like the sort of behavior that should be specified/committed in the form of a unit test. That would've caused this code change to fail, unless I'm missing something.
  - mlyle 9y ago
    No-- the cloning operation didn't preserve the partition information. When I first bisected/found the issue I thought it was as you describe, but it's really just that cloning 0'd it out in the new copy and everything else was proper.
  - masklinn 9y ago
    > The field was set to 0 by a function it was passed to previously. The entire structure was just memset to 0, it's not like the struct was carefully initialised.
GlitchMr 9y ago
Actually, it would. This issue is rather straightforward, caused by not initializing memory. Rust forbids that, consider a following Rust code. struct Example { x: i32, y: i32, } fn example_clone(input: &Example) -> Example { Example { x: input.x, // Oops, forgot y } } This fails with a following error: error[E0063]: missing field `y` in initializer of `Example` --> src/main.rs:7:5 | 7 | Example { | ^^^^^^^ missing `y`
- deleted 9y ago
  [deleted]
- benmmurphy 9y ago
  in this kernel's case the caller passed in the output so the bug would not be caught in rust if the same pattern was used. though, possibly this pattern would not be used in a rust kernel.
  - GlitchMr 9y ago
    The pointer return pattern is mostly due to C limitations. Rust is smart enough to automatically output into a pointer when a structure is big enough (this is invisible in source code, this should be rather optimizer's decision really whether to do it) and supports multiple return arguments by use of tuples. Also, even if somebody did insist on explicit mut-output pattern in Rust anyway, they would quickly realize that they need to initialize the value before getting a mutable reference to it - Rust NEVER allows access to uninitialized memory (other than unsafe blocks where you can trick Rust into accesing uninitialized memory and cause undefined behavior by using `std::mem::uninitialized`). The noise related to it should hint that this is not the intended way to do it. Here is a code example to clarify what I mean. fn write2(out: &mut i32) { *out = 2; } fn main() { let mut two; write2(&mut two); } And error message: error[E0381]: use of possibly uninitialized variable: `two` --> src/main.rs:7:17 | 7 | write2(&mut two); | ^^^ use of possibly uninitialized `two` Fixing that requires explicitly writing a value into `two`, for instance with `let mut two = 0`, which for complex structs... yeah, you may as well return a struct directly instead of bothering with pointer output. [std::mem::uninitialized]: https://doc.rust-lang.org/std/mem/fn.uninitialized.html https://doc.rust-lang.org/std/mem/fn.uninitialized.html
geofft 9y ago
Not only would this exact error (a new field is added to a struct, and old basic operations on the struct weren't manually updated to take the field into account) have been solved by idiomatic Rust with #[deriving], it is remarkably similar to another CVE in curl that I commented on when the author was all "Rust doesn't solve all our problems because not everything is memory safety". True, not everything is memory safety. Rust isn't just memory safety. There are other things we've learned about programming languages since C was invented and today that also help eliminate bugs. Quoting myself from https://www.reddit.com/r/programming/comments/61rh9j/curl_is_c/dfgwqrn/ https://www.reddit.com/r/programming/comments/61rh9j/curl_is... : > For instance, the patch for CVE-2016-5420, "Incorrect reuse of client certificates", is about making sure that when you compare two structs, you don't forget to compare a field in the two structs. In C, you have to open-code this comparison. In Rust, you just stick #[deriving(Eq)] before the struct, because unlike C, Rust knows that comparison of structs is a thing that people might want to do ever. The compiler will generate a trait Eq implementation that compares all the fields, using the Eq implementations (either automatically-derived or manual) of each field, so you even get secure string compares if you define some SecureBuffer type and use it for the strings. (As a bonus, doing that also makes sure that you don't accidentally use a normal strcmp on the strings.) When you add a new field to the struct, the automatic Eq implementation on the struct will automatically compare it, too. (Disclaimer: while I happen to like Rust, there are plenty of other languages you could also use instead—Microsoft Research even made a fork of C# designed for kernelspace. And while I am interested in writing Rust, I'm not particularly interested in telling people who are happy writing C that they should stop, just in making sure that people considering a language for a new project know what each language brings to the table.)
- akkartik 9y ago
  Minor tangent: I think your links into the curl repo may have rotted in the past 7 months, so they're not pointing at the lines you intended them to. For comments like this that are likely to be cited far in the future, I recommend using a link at a specific tree rather than just HEAD. So this: https://github.com/curl/curl/blob/8aee8a6a2d/lib/vtls/vtls.c#L94 and not this: https://github.com/curl/curl/blob/master/lib/vtls/vtls.c#L94 (Deliberately disabling truncation on the URL, at the cost of losing clickability. This comment isn't really about these specific links anyway, just the 'syntax' of the URL.)
khc 9y ago
simply because Rust/safe language does not prevent all bugs is not a reason to dis it.

khuey 9y ago

A language with copy constructors could have prevented this.

std_throwaway 9y ago
C++ would be a logical language upgrade for the kernel.
- MBCook 9y ago
  Why is that?
- cdmckay 9y ago
  Read Linus' rant about why the Linux kernel shouldn't use C++: https://lwn.net/Articles/249460/ https://lwn.net/Articles/249460/
  - steveklabnik 9y ago
    Thu, 6 Sep 2007 18:50:28 +0100 (BST) C++ has changed quite a bit in the last 10 years.
    - iforgotpassword 9y ago
      Yep, adding more and more bs so it's impossible to fully understand it ever. Understanding C thoroughly is already hard, but C++ is a whole new level. "But then only use the new and modern parts of it" No! You cannot simply ignore all the complicated cruft just because you don't like it because sooner or later it will come and bite you in the ass.
  - deathanatos 9y ago
    > [C++ is] made more horrible by the fact that a lot of substandard programmers use it Typical Linus rant. There's no substantive, logical, reasoned argument in there about why C++ would be inappropriate. The closest we get is that "You invariably start using the "nice" library features of the language like STL" — God forbid the language provide a dynamically sized array, and not force you to implement it manually. Not saying that Linux should switch from C to C++, nor that doing so wouldn't be a metric crap ton of work (it would be) and for benefits that might be hard to sell (e.g., at this point, we already have a decent vector implemented in C macros; what does switching actually gain us?) or that the kernel doesn't have special concerns (in a kernel, you have to implement malloc(), for example (and that's probably a gross simplification) and that could definitely affect a lot of things in the STL), etc. Just that the linked rant is not valid argument as to why C++ isn't a good fit.
    - hermitdev 9y ago
      Indeed. There is plenty evidence that there are a ton of shit C programmers out there, as well. It's not very hard to take a random OSS C (or C++) project and find bugs or potential bugs in the form of undefined or unspecified behavior. Correctly performing signed integer arithmetic without introducing undefined behavior is surprisingly hard. Additionally, most people never bother to do it correctly in either C or C++ because, well, it's a pain in the ass and it slows the code down. Linus's argument against C++ is basically akin to Dijkstra's argument against `goto`. Like any tool, it can be abused. But they've seen too many abuses for their own taste, so they ban the tool completely despite the advantages the tool may have.
    - koverstreet 9y ago
      > [C++ is] made more horrible by the fact that a lot of substandard programmers use it Thing is, it's a very real issue. C++ as written by the best of programmers might be a very reasonable language for the kernel, but in practice what happens is the additional abstraction and indirection mechanisms mean it becomes very hard to decipher with any confidence code written by mediocre or merely average programmers. And this is a real issue in practice, I saw it when I was at Google with code written by other programmers at Google, and honestly the majority of the code in the Linux kernel is at about the same level - some of it really good elegant code, more of it ugly and hacky but working, and a lot more (especially driver code) mediocre crap. And when having to decipher and refactor the mediocre crap - because let's be honest, that's the majority of the code - I'd much rather have to deal with C than C++. The reason this is such an issue with C++ is that - and it's been said before, but it bears repeating - C++ adds a lot of new ways to shoot yourself in the foot and it doesn't take any of the old ones away. With C, you can generally read a chunk of code and have confidence that it does what it appears to do - you can understand it in isolation. With C++, just figuring out the flow control can be a nightmare. Rust would be a different story. I am 100% in favor of using Rust whenever possible - it has some of the same issues as C++ or any other polymorphic language in that figuring out flow control can really suck, but it provides much stronger guarantees w.r.t. how code can screw you over that make it well worth it. C++... not so much.
bonzini 9y ago
A language with copy constructors doesn't prevent you from putting a bug in them. C++ or Rust might have put a 0 in the uninitialized member, but the bug would have remained.
- steveklabnik 9y ago
  Rust doens't have copy constructors, to be clear.
- masklinn 9y ago
  > A language with copy constructors doesn't prevent you from putting a bug in them. If you don't override it, it will copy everything properly. > Rust might have put a 0 in the uninitialized member Rust does not do that. If a member is not initialised, it's uninitialised and in this case the structure will be rejected. Here even if you could not just have derived Clone (struct bio looks non-trivial) your hand-rolled implementation would have complained that you had not properly initialised the member.
  - bonzini 9y ago
    > If you don't override it, it will copy everything properly. Sure, so does memcpy or bare struct assignment in C. The problem is that trivial copy constructors are relatively rare. > Here even if you could not just have derived Clone (struct bio looks non-trivial) your hand-rolled implementation would have complained that you had not properly initialised the member. Right, but this is more similar to an assignment operator. DerefMut in Rust I think wouldn't have caught the bug.
    - masklinn 9y ago
      > Right, but this is more similar to an assignment operator. Only if you don't zoom out. If you do, this is alloc/init/fill which in C++ or Rust would use a regular copy and RVO.
barrkel 9y ago
A language with copy constructors hides user defined code inside seemingly innocuous expressions, for great hilarity when they have side effects or other unexpected semantics.

icebraining 9y ago

Language definitively could have helped. For example, you could have a type that could only be created with both disk and partition info filled in. Then the compiler would complain if you forgot one of them.

iforgotpassword 9y ago
The field was set to 0 by a function it was passed to previously.. It didn't do that in older kernel versions so it kept the proper value, so a classical miscommunication (or none at all) between devs.
- remar 9y ago
  This sounds exactly like the sort of behavior that should be specified/committed in the form of a unit test. That would've caused this code change to fail, unless I'm missing something.
- mlyle 9y ago
  No-- the cloning operation didn't preserve the partition information. When I first bisected/found the issue I thought it was as you describe, but it's really just that cloning 0'd it out in the new copy and everything else was proper.
- masklinn 9y ago
  > The field was set to 0 by a function it was passed to previously. The entire structure was just memset to 0, it's not like the struct was carefully initialised.

GlitchMr 9y ago

Actually, it would. This issue is rather straightforward, caused by not initializing memory. Rust forbids that, consider a following Rust code. struct Example { x: i32, y: i32, } fn example_clone(input: &Example) -> Example { Example { x: input.x, // Oops, forgot y } } This fails with a following error: error[E0063]: missing field `y` in initializer of `Example` --> src/main.rs:7:5 | 7 | Example { | ^^^^^^^ missing `y`

deleted 9y ago
[deleted]
benmmurphy 9y ago
in this kernel's case the caller passed in the output so the bug would not be caught in rust if the same pattern was used. though, possibly this pattern would not be used in a rust kernel.
- GlitchMr 9y ago
  The pointer return pattern is mostly due to C limitations. Rust is smart enough to automatically output into a pointer when a structure is big enough (this is invisible in source code, this should be rather optimizer's decision really whether to do it) and supports multiple return arguments by use of tuples. Also, even if somebody did insist on explicit mut-output pattern in Rust anyway, they would quickly realize that they need to initialize the value before getting a mutable reference to it - Rust NEVER allows access to uninitialized memory (other than unsafe blocks where you can trick Rust into accesing uninitialized memory and cause undefined behavior by using `std::mem::uninitialized`). The noise related to it should hint that this is not the intended way to do it. Here is a code example to clarify what I mean. fn write2(out: &mut i32) { *out = 2; } fn main() { let mut two; write2(&mut two); } And error message: error[E0381]: use of possibly uninitialized variable: `two` --> src/main.rs:7:17 | 7 | write2(&mut two); | ^^^ use of possibly uninitialized `two` Fixing that requires explicitly writing a value into `two`, for instance with `let mut two = 0`, which for complex structs... yeah, you may as well return a struct directly instead of bothering with pointer output. [std::mem::uninitialized]: https://doc.rust-lang.org/std/mem/fn.uninitialized.html https://doc.rust-lang.org/std/mem/fn.uninitialized.html

geofft 9y ago

Not only would this exact error (a new field is added to a struct, and old basic operations on the struct weren't manually updated to take the field into account) have been solved by idiomatic Rust with #[deriving], it is remarkably similar to another CVE in curl that I commented on when the author was all "Rust doesn't solve all our problems because not everything is memory safety". True, not everything is memory safety. Rust isn't just memory safety. There are other things we've learned about programming languages since C was invented and today that also help eliminate bugs. Quoting myself from https://www.reddit.com/r/programming/comments/61rh9j/curl_is_c/dfgwqrn/ https://www.reddit.com/r/programming/comments/61rh9j/curl_is... : > For instance, the patch for CVE-2016-5420, "Incorrect reuse of client certificates", is about making sure that when you compare two structs, you don't forget to compare a field in the two structs. In C, you have to open-code this comparison. In Rust, you just stick #[deriving(Eq)] before the struct, because unlike C, Rust knows that comparison of structs is a thing that people might want to do ever. The compiler will generate a trait Eq implementation that compares all the fields, using the Eq implementations (either automatically-derived or manual) of each field, so you even get secure string compares if you define some SecureBuffer type and use it for the strings. (As a bonus, doing that also makes sure that you don't accidentally use a normal strcmp on the strings.) When you add a new field to the struct, the automatic Eq implementation on the struct will automatically compare it, too. (Disclaimer: while I happen to like Rust, there are plenty of other languages you could also use instead—Microsoft Research even made a fork of C# designed for kernelspace. And while I am interested in writing Rust, I'm not particularly interested in telling people who are happy writing C that they should stop, just in making sure that people considering a language for a new project know what each language brings to the table.)

akkartik 9y ago
Minor tangent: I think your links into the curl repo may have rotted in the past 7 months, so they're not pointing at the lines you intended them to. For comments like this that are likely to be cited far in the future, I recommend using a link at a specific tree rather than just HEAD. So this: https://github.com/curl/curl/blob/8aee8a6a2d/lib/vtls/vtls.c#L94 and not this: https://github.com/curl/curl/blob/master/lib/vtls/vtls.c#L94 (Deliberately disabling truncation on the URL, at the cost of losing clickability. This comment isn't really about these specific links anyway, just the 'syntax' of the URL.)

khc 9y ago

simply because Rust/safe language does not prevent all bugs is not a reason to dis it.