5 ms·

C needs to die. Pro tip for language designers: require all fields to be initialized any time an object is created. Really impressive debugging too.

by db48x 5y ago

kevincox 5y ago
I love Rust, but would it have prevented this problem? IIUC there was no memory corruption at the language level here. This was really just a logic error.
- pdw 5y ago
  You can't accidentally leave a field of a struct uninitialized in Rust or in other sane languages.
- db48x 5y ago
  Yes, it would have. Some code creates an instance of some struct, but doesn’t set the flags field to zero. It thus keeps whatever value happened to be in that spot in memory, an essentially random set of bits. Rust would force you to either explicitly name the flags field and give it a value, or use `..Default::default()` to initialize all remaining fields automatically. Anything else would be a compile–time error. The fix: +++ b/lib/iov_iter.c @@ -414,6 +414,7 @@ static size_t copy_page_to_iter_pipe(struct page \*page, size_t offset, size_t by return 0; buf->ops = &page_cache_pipe_buf_ops; + buf->flags = 0; get_page(page); buf->page = page; buf->offset = offset;
  - amelius 5y ago
    Wouldn't Lint have caught the error too?
  - kevincox 5y ago
    Ah thanks for explaining. I misunderstood the root cause and didn't read the patch. Rust definitely would have helped here. Or even just enforcing modern C practices such as overwriting the whole struct so that non-specified values would have been set to zero (although explicit is better than zero).
  - rfoo 5y ago
    No. Rust can not prevent this bug. The bug is that they are reusing (or, repurposing) an already-allocated-and-used buffer and forgot to reset flags. This is a logic bug, not a memory safety bug. In fact, this might be a prime example of "using Rust does not magically eliminate your temporal bugs because sometimes they are not about memory safety but logical". Before that my favorite such bug is a Use-After-Free in Redox OS's filesystem codes. Pro tip for random HN Rust evangelist: read the fucking code before posting your "sHoUlD HAVe uSED A BeTTER lANGUAGE" shit.
    - gpderetta 5y ago
      I agree with your sentiment. Only the most strict pure functional languages will prevent you from reusing objects. You could argue that some languages distinguish raw memory from actual objects and even when reusing memory you would still go through an initialization phase (for example placement new in C++) that would take care of putting the object into a known default state.
    - deredede 5y ago
      This is only partially fair. In Rust you would probably have assigned a new object into *buf here instead of overwriting the fields manually. It is good practice to do this (if the code is logically an object initialization, it should actually be an object initialization, not a bunch of field assignments), but it's clunky to do so in C because you can't use initializers in assignments.
      - TonyTrapp 5y ago
        The point is: You could have done this in Rust, but you wouldn't have been required to do so, so the exact same logic bug could have emerged. Maybe it would be more Rust-like to write the code like that, but it would have also been possible to write the code like that in C - and since we're talking about the kernel here, even if this code was written in Rust a developer might have written it in the more C-like way for performance reasons.
      - lanstin 5y ago
        People writing C don't re-use allocated objects because it's clunky but to improve performance. The general purpose allocators are almost always much slower than something where you know the pattern of allocations. I've no idea if Rust has a similar issue. I would think that most kernel code, whether C or Rust, would need to handle "allocation fails" case and not depend on language constructs to do allocations, but that's just a guess.
  - modshatereality 5y ago
    buf is a pointer to a pipe_buffer array, this whole function wreaks yes, but i don't think this is a simple "initialization would have fixed" bug buf = &pipe->bufs[i_head & p_mask]; i think it is a problem with merging and needing to reset the flags, but i didn't want to waste too much time really trying to find the root issue and wtf merging is/why do it.
max_k 5y ago
> require all fields to be initialized any time an object is created I'm not a fan of such a policy. That usually leads to people zero-initializing everything. For this bug, this would have been correct, but sometimes, there is no good "initial" value, and zero is just another random value like all the 2^32-1 others. Worse, if you zero-initialize everything, valgrind will be unable to find accesses to uninitialized variables, which hides the bug and makes it harder to find. If I have no good initial value for something, I'd rather leave it uninitialized.
- lmm 5y ago
  > I'm not a fan of such a policy. That usually leads to people zero-initializing everything. For this bug, this would have been correct, but sometimes, there is no good "initial" value, and zero is just another random value like all the 2^32-1 others. So use a language that has an option type, we've only had them for what, 50 years now.
  - dundarious 5y ago
    I think https://news.ycombinator.com/item?id=30588362 https://news.ycombinator.com/item?id=30588362 has shown that this wouldn't solve anything for this particular case.
- gpderetta 5y ago
  GCC has recently introduced a mode (-ftrivial-auto-var-init) that will zero initialize all automatic variables by default while still treating them as UB for sanitize/warning purposes. The issue is with dynamic memory allocation as that would be the responsibility of the allocator (and of course the kernel uses custom allocators).
  - max_k 5y ago
    Interesting compiler feature to work around (unknown) vulnerabilities similar to this one. However in this case, it wouldn't help; the initial allocation is with explicit zero-initialization, but this is a circular buffer, and the problem occurs when slots get reused (which is the basic idea of a circular buffer).
max_k 5y ago
btw. this is how I would make the code more robust: https://lore.kernel.org/lkml/20220225185431.2617232-4-max.kellermann@gmail.com/ https://lore.kernel.org/lkml/20220225185431.2617232-4-max.ke... I'm a C++ guy, and the lack of constructors is one of many things that bothers me with C.
- wtarreau 5y ago
  I tend to agree but only partially. I'm doing myself a lot of init functions for plenty of stuff, but I've faced plenty of issues due to the fact that these functions do not always initialize everything (and your patch above does exactly that for a good reason) and that it's much less obvious for those using them. And if they initialize too much, they can equally be a pain to work with. It's not uncommon to require 2 or 3 different init functions depending on what you're doing, but the name should explicitly indicate the promise, and that's where it's difficult.
  - max_k 5y ago
    Then let's have those 2 or 3 documented init functions. That's not perfect, but still much better than spraying different (undocumented) copies of the init code everywhere, that have to be located and adjusted every time somebody refactors something.
    - wtarreau 5y ago
      In any case, definitely ;-)
turminal 5y ago
> Pro tip for language designers: require all fields to be initialized any time an object is created. This proposal sounds great until you find out that this is a hard problem to solve reasonably well in the compiler and no matter what you do there will be valid programs that your compiler will reject.
- pjc50 5y ago
  No? This seems to work perfectly fine for other languages. > valid programs that your compiler will reject. Do you mean "valid programs where everything is initialized when an object is created will somehow fail to detect that"?
  - Arnavion 5y ago
    >Do you mean "valid programs where everything is initialized when an object is created will somehow fail to detect that"? Valid programs where a field is left uninitialized at creation time, but the programmer makes sure it's initialized before it's used.
    - pjc50 5y ago
      Sure, so we can easily change the definition of "valid" so that you have to initialize them at creation time. shrug
    - db48x 5y ago
      No, I don’t think that this should be considered a valid program. I would never allow any code that creates a partly–initialized struct in any code that I maintain. I agree that it’s a hard problem, but I don’t think that language designers should use that as an excuse. I think that language designers should really double–down and require that every field be initialized, otherwise people will just forget. The code will be carefully written at first, but then someone new will come along and add a new field, and then the existing code is insufficient. With no errors from the compiler there is no way to ensure that the new programmer updates everything to accommodate the new field. Rust does pretty well in this regard, but you can still use unsafe blocks to get a partly–initialized struct if you really want one. I like Rust, but I wish that it went all the way and didn’t allow it at all.

C needs to die. Pro tip for language designers: require all fields to be initialized any time an object is created. Really impressive debugging too.

by db48x 5y ago

C needs to die. Pro tip for language designers: require all fields to be initialized any time an object is created. Really impressive debugging too.

kevincox 5y ago

I love Rust, but would it have prevented this problem? IIUC there was no memory corruption at the language level here. This was really just a logic error.

pdw 5y ago
You can't accidentally leave a field of a struct uninitialized in Rust or in other sane languages.
db48x 5y ago
Yes, it would have. Some code creates an instance of some struct, but doesn’t set the flags field to zero. It thus keeps whatever value happened to be in that spot in memory, an essentially random set of bits. Rust would force you to either explicitly name the flags field and give it a value, or use `..Default::default()` to initialize all remaining fields automatically. Anything else would be a compile–time error. The fix: +++ b/lib/iov_iter.c @@ -414,6 +414,7 @@ static size_t copy_page_to_iter_pipe(struct page \*page, size_t offset, size_t by return 0; buf->ops = &page_cache_pipe_buf_ops; + buf->flags = 0; get_page(page); buf->page = page; buf->offset = offset;
- amelius 5y ago
  Wouldn't Lint have caught the error too?
- kevincox 5y ago
  Ah thanks for explaining. I misunderstood the root cause and didn't read the patch. Rust definitely would have helped here. Or even just enforcing modern C practices such as overwriting the whole struct so that non-specified values would have been set to zero (although explicit is better than zero).
- rfoo 5y ago
  No. Rust can not prevent this bug. The bug is that they are reusing (or, repurposing) an already-allocated-and-used buffer and forgot to reset flags. This is a logic bug, not a memory safety bug. In fact, this might be a prime example of "using Rust does not magically eliminate your temporal bugs because sometimes they are not about memory safety but logical". Before that my favorite such bug is a Use-After-Free in Redox OS's filesystem codes. Pro tip for random HN Rust evangelist: read the fucking code before posting your "sHoUlD HAVe uSED A BeTTER lANGUAGE" shit.
  - gpderetta 5y ago
    I agree with your sentiment. Only the most strict pure functional languages will prevent you from reusing objects. You could argue that some languages distinguish raw memory from actual objects and even when reusing memory you would still go through an initialization phase (for example placement new in C++) that would take care of putting the object into a known default state.
  - deredede 5y ago
    This is only partially fair. In Rust you would probably have assigned a new object into *buf here instead of overwriting the fields manually. It is good practice to do this (if the code is logically an object initialization, it should actually be an object initialization, not a bunch of field assignments), but it's clunky to do so in C because you can't use initializers in assignments.
    - TonyTrapp 5y ago
      The point is: You could have done this in Rust, but you wouldn't have been required to do so, so the exact same logic bug could have emerged. Maybe it would be more Rust-like to write the code like that, but it would have also been possible to write the code like that in C - and since we're talking about the kernel here, even if this code was written in Rust a developer might have written it in the more C-like way for performance reasons.
    - lanstin 5y ago
      People writing C don't re-use allocated objects because it's clunky but to improve performance. The general purpose allocators are almost always much slower than something where you know the pattern of allocations. I've no idea if Rust has a similar issue. I would think that most kernel code, whether C or Rust, would need to handle "allocation fails" case and not depend on language constructs to do allocations, but that's just a guess.
- modshatereality 5y ago
  buf is a pointer to a pipe_buffer array, this whole function wreaks yes, but i don't think this is a simple "initialization would have fixed" bug buf = &pipe->bufs[i_head & p_mask]; i think it is a problem with merging and needing to reset the flags, but i didn't want to waste too much time really trying to find the root issue and wtf merging is/why do it.

max_k 5y ago

> require all fields to be initialized any time an object is created I'm not a fan of such a policy. That usually leads to people zero-initializing everything. For this bug, this would have been correct, but sometimes, there is no good "initial" value, and zero is just another random value like all the 2^32-1 others. Worse, if you zero-initialize everything, valgrind will be unable to find accesses to uninitialized variables, which hides the bug and makes it harder to find. If I have no good initial value for something, I'd rather leave it uninitialized.

lmm 5y ago
> I'm not a fan of such a policy. That usually leads to people zero-initializing everything. For this bug, this would have been correct, but sometimes, there is no good "initial" value, and zero is just another random value like all the 2^32-1 others. So use a language that has an option type, we've only had them for what, 50 years now.
- dundarious 5y ago
  I think https://news.ycombinator.com/item?id=30588362 https://news.ycombinator.com/item?id=30588362 has shown that this wouldn't solve anything for this particular case.
gpderetta 5y ago
GCC has recently introduced a mode (-ftrivial-auto-var-init) that will zero initialize all automatic variables by default while still treating them as UB for sanitize/warning purposes. The issue is with dynamic memory allocation as that would be the responsibility of the allocator (and of course the kernel uses custom allocators).
- max_k 5y ago
  Interesting compiler feature to work around (unknown) vulnerabilities similar to this one. However in this case, it wouldn't help; the initial allocation is with explicit zero-initialization, but this is a circular buffer, and the problem occurs when slots get reused (which is the basic idea of a circular buffer).

max_k 5y ago

btw. this is how I would make the code more robust: https://lore.kernel.org/lkml/20220225185431.2617232-4-max.kellermann@gmail.com/ https://lore.kernel.org/lkml/20220225185431.2617232-4-max.ke... I'm a C++ guy, and the lack of constructors is one of many things that bothers me with C.

wtarreau 5y ago
I tend to agree but only partially. I'm doing myself a lot of init functions for plenty of stuff, but I've faced plenty of issues due to the fact that these functions do not always initialize everything (and your patch above does exactly that for a good reason) and that it's much less obvious for those using them. And if they initialize too much, they can equally be a pain to work with. It's not uncommon to require 2 or 3 different init functions depending on what you're doing, but the name should explicitly indicate the promise, and that's where it's difficult.
- max_k 5y ago
  Then let's have those 2 or 3 documented init functions. That's not perfect, but still much better than spraying different (undocumented) copies of the init code everywhere, that have to be located and adjusted every time somebody refactors something.
  - wtarreau 5y ago
    In any case, definitely ;-)

turminal 5y ago

> Pro tip for language designers: require all fields to be initialized any time an object is created. This proposal sounds great until you find out that this is a hard problem to solve reasonably well in the compiler and no matter what you do there will be valid programs that your compiler will reject.

pjc50 5y ago
No? This seems to work perfectly fine for other languages. > valid programs that your compiler will reject. Do you mean "valid programs where everything is initialized when an object is created will somehow fail to detect that"?
- Arnavion 5y ago
  >Do you mean "valid programs where everything is initialized when an object is created will somehow fail to detect that"? Valid programs where a field is left uninitialized at creation time, but the programmer makes sure it's initialized before it's used.
  - pjc50 5y ago
    Sure, so we can easily change the definition of "valid" so that you have to initialize them at creation time. shrug
  - db48x 5y ago
    No, I don’t think that this should be considered a valid program. I would never allow any code that creates a partly–initialized struct in any code that I maintain. I agree that it’s a hard problem, but I don’t think that language designers should use that as an excuse. I think that language designers should really double–down and require that every field be initialized, otherwise people will just forget. The code will be carefully written at first, but then someone new will come along and add a new field, and then the existing code is insufficient. With no errors from the compiler there is no way to ensure that the new programmer updates everything to accommodate the new field. Rust does pretty well in this regard, but you can still use unsafe blocks to get a partly–initialized struct if you really want one. I like Rust, but I wish that it went all the way and didn’t allow it at all.