5 ms·

An almost catastrophic OpenZFS bug and the humans that made it

53 points by r4um 1y ago

jpfr 1y ago
The problems with C are real. At the same time, the tooling has gotten much better in the last years. Clang-analyzer is fast enough to run as part of the CI. Newer gcc also give quite a few more warnings for unused results. My recommendation to the project is to - Remove all compiler warnings and enable warning-as-error - Increase the coverage of unit tests to >80% That is a lot of work. But that's what is required for high-criticality systems engineering.
- rollcat 1y ago
  I hate to be the that OpenBSD guy, but "the people who do the work are the ones to decide how it's done". Yes, people are paid to maintain OpenZFS, but so far nobody is ready to pay for (or volunteer to) meet your bar. Side note: OpenZFS already has an extensive test suite. Merely hitting a code branch wouldn't have caught this one.
fredoralive 1y ago
I missed it, but I was distracted by cols variable being initialised with the original width, but then being immediately overwritten with the logical width.
a_t48 1y ago
FWIW you can do the same thing in cpp, too - but Rust’s syntax certainly makes it easier.
flohofwoe 1y ago
I'm not sure why C is blamed in this case when you can do exactly the same strong typing fix in C, and with C99 struct literals it's also not much worse to work with: typedef struct { size_t size; } PhysicalSize; typedef struct { size_t size; } AllocatedSize; PhysicalSize psize = { 123 }; // or { .size = 123 } AllocatedSize asize = { 234 }; psize = asize; // error ...and in reverse, Rust wouldn't protect you from that exact same bug if the programmer decided to use usize like in the original C code. IME overly strong typing is a double-edged sword though, on one hand it makes the code more robust, but on the other hand also more 'change resistant'. I still would like to see a strong typedef variant in C so that the struct wrapping wouldn't be needed, e.g.: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3320.htm https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3320.htm
- beeb 1y ago
  Rust would warn you of an unused variable: "warning: value assigned to `psize` is never read"
  - bloak 1y ago
    And so would GCC: warning: variable psize set but not used [-Wunused-but-set-variable]
    - beeb 1y ago
      The fact that the bug slipped through the cracks highlights the importance of sane defaults.
      - flohofwoe 1y ago
        The warning is in the -Wall warning set, which tbh should be the bare minimum each C/C++ project enables (along with -Wextra, and -Werror).
Cthulhu_ 1y ago
Wouldn't this have been caught by an exhaustive unit test or even a fuzz test? I don't know what kind of testing practices are applied to projects like zfs, nor what kinds or amounts of tests functions like this are subject to, but I would imagine that especially for low-level functions like this, unit tests with all of the known edge cases would be written. (yes this is very much an armchair opinion, I mostly do front-end JS for a living)
- bspammer 1y ago
  Yeah that surprised me too - I would have assumed that ZFS had a bunch of "store and retrieve" tests in many different configurations that would have caught this.
- 4gotunameagain 1y ago
  Does it even need an exhaustive unit test? A single call where the psize is different than the asize would have caught it.
topspin 1y ago
I found it. All that tells you is that it's a simple problem. Had I not been told it was broken I likely would not have. It's the kind of bug that makes you stop breathing for a brief moment. So I ran this function through Gemini 2.5 Pro, ChatGPT o3 and Grok 3. No context other than the source code fragment was provided. All three also clearly identified the problem. There is no longer a reason for trivial flaws such as this surviving to release code. We're past that now, and that's an astonishing thing. These are the questions I ponder: Should we consider the possibility that the ongoing, incomplete and decades long pursuit of "better" languages is now misguided? Or, that perhaps "better" might now mean whatever properties make code easier for LLMs to analyze and validate against specifications, as opposed to merely preventing the flaws humans are prone to make?
- nabla9 1y ago
  [flagged]
  - topspin 1y ago
    I don't believe I could have been more self-deprecating there. I know for certain I don't care to.
- kalaksi 1y ago
  > whatever properties make code easier for LLMs to analyze So double down on languages that are more represented in training data? I think it's still worthwhile to actually improve programming languages. Relying only on LLMs is fragile. So ideally do both: improve language and AI.
  - topspin 1y ago
    > So double down on languages that are more represented in training data? The pragmatic answer to that is: this appears to be highly effective. What I have in mind is something else, however. Consider the safety nets we try to build with, for example, elaborate type systems. These new analysis tools can operate on enormous contexts and infer and enforce "type" with a capacity far greater what a human mind can hope to approach. So perhaps there is little value to complex types. Instead, a simple type system supported by specification will be the superior approach. seL4 is an example of the concept: C, but exhaustively specified and verified.
apple1417 1y ago
Having run into similar problems several times, I've also never really been satisfied with the solutions. You have to have different types to cause compile errors, but then you have throw those types away whenever you perform any actual operations on them. struct TypeA { int inner; }; struct TypeB { int inner; }; struct TypeA a1, a2; // whoops TypeB result = {.inner = a1.inner + a2.inner}; Don't get me wrong, where I've used this sort of pattern it's definitely caught issues, and it definitely massively lowers the surface area for this type of bug. But I still feel like it's quite fragile. The use case I felt worked best was seperating out UTC datetime vs localised datetime structs - mainly since it's something you already had to use function calls to add/subtract from, you couldn't play with raw values.
Narew 1y ago
It remind me a devlog from andrew kelley on the same topics : https://ziglang.org/devlog/2024/#2024-11-04 https://ziglang.org/devlog/2024/#2024-11-04
atiq-ca 1y ago
Thanks contribution to openzfs.
Nokinside 1y ago
Software verification tools based on abstract Interpretation are really good today. If you want free software I recommend IKOS - a is a sound static analyzer for C/C++ developed at NASA. Checks: https://github.com/NASA-SW-VnV/ikos/blob/master/analyzer/README.md#checks https://github.com/NASA-SW-VnV/ikos/blob/master/analyzer/REA... Numerical abstract domains: https://github.com/NASA-SW-VnV/ikos/blob/master/analyzer/README.md#numerical-abstract-domains https://github.com/NASA-SW-VnV/ikos/blob/master/analyzer/REA... Commercial tool like Astree https://www.absint.com/astree/index.htm https://www.absint.com/astree/index.htm if you have money.
prmoustache 1y ago
Isn't it a case for eliminating all warnings and treat them as bugs? It seems you are dooming your project the minute you start ignoring your first warning.
- bloak 1y ago
  Most projects I've worked on treat warnings as bugs. It's annoying, sometimes, when you have to fiddle with the code and add lines to prevent bogus warnings from breaking the build, particularly when you're making a temporary change for debugging purposes, and I remember a couple of occasions when we had to insert a comment something like "This was needed to prevent a warning caused by a bug in GCC X.Y.Z (link to compiler bug on issue tracker)". But, on balance, it's worth it.
nottorp 1y ago
So why is the solution Rust, and not any of the other 2000 "modern" languages?
- desdenova 1y ago
  So give me a list of the 2000 languages OpenZFS could've been written in.
  - mrlonglong 1y ago
    Cobol /s
Uriopass 1y ago
In this case any simple unit test would have catched it. Surprised it wasn't mentioned in the post.
0points 1y ago
Unexpected issue (for me). I would have expected that the compiler should complain that psize is computed but unused. Why isn't -Wunused-variable enabled in OpenZFS?
- pja 1y ago
  The blog post suggests that, probably because it’s never been used, it’s just too noisy to turn on. Arguably every unused variable in the code base is a potential bug like this waiting to chomp on some poor user’s data though, so arguably they should be turning it on & dealing with the consequences?
  - 0points 1y ago
    > so arguably they should be turning it on & dealing with the consequences? Most definitely, yes.
    - 0points 1y ago
      FWIW: I learned in a sibling comment, that this compiler flag doesn't produce a warning in this specific case anyway (but I think it should!). In general (for any programming language, really), I would advocate for enabling warnings and address them as you go along. Because that unlocks the developer feature of being able to react when code changes introduces warnings, as they generally point to unsound code.
v5v3 1y ago
Gpt4o-mini response: >The provided code snippet is a function that converts an allocation size (asize) into the largest physical size (psize) that can be safely written to an allocation for a given virtual device (vdev). Here are some observations and potential issues in the code: Return Value: The function currently returns asize instead of psize. This seems to be a mistake, as the purpose of the function is to compute and return the psize. The correct return statement should be: return psize; Comment Clarity: The comments mention that the function does not take into account the effect of gang headers. It might be helpful to clarify what "gang headers" are and how they affect the allocation sizes, especially for someone unfamiliar with the context. Variable Initialization: The variable psize is calculated based on asize, ashift, cols, and nparity. Ensure that these values are correctly initialized and that vdev_raidz_get_logical_width returns a valid value for cols. If cols is zero, it could lead to division by zero in the DIV_ROUND_UP macro. Assertions: The assertion ASSERT0(asize % (1 << ashift)); checks that asize is a multiple of 2^ashift. If this assertion fails, it will cause the program to terminate. Ensure that this condition is always met before calling this function, or consider handling this case more gracefully. Macro Usage: The DIV_ROUND_UP macro should be checked to ensure it handles edge cases correctly, such as when psize is zero. If psize is zero, the division could lead to unexpected behavior. Code Readability: Consider adding more comments to explain the logic behind the calculations, especially for someone who may not be familiar with the RAID-Z implementation. Error Handling: There is no error handling in the function. Depending on the context in which this function is used, it might be beneficial to add checks for invalid input values and return an error code or handle exceptions accordingly. Here is a revised version of the function with the return value corrected: .....(Snipped for formatting issues) Make sure to test the function thoroughly to ensure it behaves as expected under various conditions.
v5v3 1y ago
How much would it cost/how long to give an LLM the entire OpenZfs repo and ask it to find every bug?
- v5v3 1y ago
  To the person who downvoted this... accept the world has changed my friend!! And more so, as the NSA, China, Russia etc will be using llms to comb repo's to find zero day vulnerabilities.
Voultapher 1y ago
I'm surprised their testing didn't catch it. And while I get that turning on warnings in a code-base not designed with them can be annoying, `-Wunused` is certainly within reach. I've enabled and maintained `-Weverything` in a 30m LoC C code base that is older than myself. OpenZFS should try harder to enable such warnings in their CI.
- rollcat 1y ago
  That's a bit unfair. The size and age of the codebase speaks nothing of its quality. I've thrown out 50k 5yro Python code without a second thought. Also, filesystems are an insanely complex problem, and ZFS is by far the most complex one to be widely deployed.
giingyui 1y ago
[dead]
jmclnx 1y ago
For my personal use, I have no need for ZFS on Linux (ext4) or *BSD (UFS/ffs). I have been playing with NetBSD for a bit and I discovered their FS has a log parameter, using that on mount to me makes their file system almost as good as ext4. Interesting to me it is not advertised too much.
aus10d 1y ago
This was a really good point

53 points by r4um 1y ago

jpfr 1y ago

The problems with C are real. At the same time, the tooling has gotten much better in the last years. Clang-analyzer is fast enough to run as part of the CI. Newer gcc also give quite a few more warnings for unused results. My recommendation to the project is to - Remove all compiler warnings and enable warning-as-error - Increase the coverage of unit tests to >80% That is a lot of work. But that's what is required for high-criticality systems engineering.

rollcat 1y ago
I hate to be the that OpenBSD guy, but "the people who do the work are the ones to decide how it's done". Yes, people are paid to maintain OpenZFS, but so far nobody is ready to pay for (or volunteer to) meet your bar. Side note: OpenZFS already has an extensive test suite. Merely hitting a code branch wouldn't have caught this one.

fredoralive 1y ago

I missed it, but I was distracted by cols variable being initialised with the original width, but then being immediately overwritten with the logical width.

a_t48 1y ago

FWIW you can do the same thing in cpp, too - but Rust’s syntax certainly makes it easier.

flohofwoe 1y ago

I'm not sure why C is blamed in this case when you can do exactly the same strong typing fix in C, and with C99 struct literals it's also not much worse to work with: typedef struct { size_t size; } PhysicalSize; typedef struct { size_t size; } AllocatedSize; PhysicalSize psize = { 123 }; // or { .size = 123 } AllocatedSize asize = { 234 }; psize = asize; // error ...and in reverse, Rust wouldn't protect you from that exact same bug if the programmer decided to use usize like in the original C code. IME overly strong typing is a double-edged sword though, on one hand it makes the code more robust, but on the other hand also more 'change resistant'. I still would like to see a strong typedef variant in C so that the struct wrapping wouldn't be needed, e.g.: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3320.htm https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3320.htm

beeb 1y ago
Rust would warn you of an unused variable: "warning: value assigned to `psize` is never read"
- bloak 1y ago
  And so would GCC: warning: variable psize set but not used [-Wunused-but-set-variable]
  - beeb 1y ago
    The fact that the bug slipped through the cracks highlights the importance of sane defaults.
    - flohofwoe 1y ago
      The warning is in the -Wall warning set, which tbh should be the bare minimum each C/C++ project enables (along with -Wextra, and -Werror).

Cthulhu_ 1y ago

Wouldn't this have been caught by an exhaustive unit test or even a fuzz test? I don't know what kind of testing practices are applied to projects like zfs, nor what kinds or amounts of tests functions like this are subject to, but I would imagine that especially for low-level functions like this, unit tests with all of the known edge cases would be written. (yes this is very much an armchair opinion, I mostly do front-end JS for a living)

bspammer 1y ago
Yeah that surprised me too - I would have assumed that ZFS had a bunch of "store and retrieve" tests in many different configurations that would have caught this.
4gotunameagain 1y ago
Does it even need an exhaustive unit test? A single call where the psize is different than the asize would have caught it.

topspin 1y ago

I found it. All that tells you is that it's a simple problem. Had I not been told it was broken I likely would not have. It's the kind of bug that makes you stop breathing for a brief moment. So I ran this function through Gemini 2.5 Pro, ChatGPT o3 and Grok 3. No context other than the source code fragment was provided. All three also clearly identified the problem. There is no longer a reason for trivial flaws such as this surviving to release code. We're past that now, and that's an astonishing thing. These are the questions I ponder: Should we consider the possibility that the ongoing, incomplete and decades long pursuit of "better" languages is now misguided? Or, that perhaps "better" might now mean whatever properties make code easier for LLMs to analyze and validate against specifications, as opposed to merely preventing the flaws humans are prone to make?

nabla9 1y ago
[flagged]
- topspin 1y ago
  I don't believe I could have been more self-deprecating there. I know for certain I don't care to.
kalaksi 1y ago
> whatever properties make code easier for LLMs to analyze So double down on languages that are more represented in training data? I think it's still worthwhile to actually improve programming languages. Relying only on LLMs is fragile. So ideally do both: improve language and AI.
- topspin 1y ago
  > So double down on languages that are more represented in training data? The pragmatic answer to that is: this appears to be highly effective. What I have in mind is something else, however. Consider the safety nets we try to build with, for example, elaborate type systems. These new analysis tools can operate on enormous contexts and infer and enforce "type" with a capacity far greater what a human mind can hope to approach. So perhaps there is little value to complex types. Instead, a simple type system supported by specification will be the superior approach. seL4 is an example of the concept: C, but exhaustively specified and verified.

apple1417 1y ago

Having run into similar problems several times, I've also never really been satisfied with the solutions. You have to have different types to cause compile errors, but then you have throw those types away whenever you perform any actual operations on them. struct TypeA { int inner; }; struct TypeB { int inner; }; struct TypeA a1, a2; // whoops TypeB result = {.inner = a1.inner + a2.inner}; Don't get me wrong, where I've used this sort of pattern it's definitely caught issues, and it definitely massively lowers the surface area for this type of bug. But I still feel like it's quite fragile. The use case I felt worked best was seperating out UTC datetime vs localised datetime structs - mainly since it's something you already had to use function calls to add/subtract from, you couldn't play with raw values.

Narew 1y ago

It remind me a devlog from andrew kelley on the same topics : https://ziglang.org/devlog/2024/#2024-11-04 https://ziglang.org/devlog/2024/#2024-11-04

atiq-ca 1y ago

Thanks contribution to openzfs.

Nokinside 1y ago

Software verification tools based on abstract Interpretation are really good today. If you want free software I recommend IKOS - a is a sound static analyzer for C/C++ developed at NASA. Checks: https://github.com/NASA-SW-VnV/ikos/blob/master/analyzer/README.md#checks https://github.com/NASA-SW-VnV/ikos/blob/master/analyzer/REA... Numerical abstract domains: https://github.com/NASA-SW-VnV/ikos/blob/master/analyzer/README.md#numerical-abstract-domains https://github.com/NASA-SW-VnV/ikos/blob/master/analyzer/REA... Commercial tool like Astree https://www.absint.com/astree/index.htm https://www.absint.com/astree/index.htm if you have money.

prmoustache 1y ago

Isn't it a case for eliminating all warnings and treat them as bugs? It seems you are dooming your project the minute you start ignoring your first warning.

bloak 1y ago
Most projects I've worked on treat warnings as bugs. It's annoying, sometimes, when you have to fiddle with the code and add lines to prevent bogus warnings from breaking the build, particularly when you're making a temporary change for debugging purposes, and I remember a couple of occasions when we had to insert a comment something like "This was needed to prevent a warning caused by a bug in GCC X.Y.Z (link to compiler bug on issue tracker)". But, on balance, it's worth it.

nottorp 1y ago

So why is the solution Rust, and not any of the other 2000 "modern" languages?

desdenova 1y ago
So give me a list of the 2000 languages OpenZFS could've been written in.
- mrlonglong 1y ago
  Cobol /s

Uriopass 1y ago

In this case any simple unit test would have catched it. Surprised it wasn't mentioned in the post.

0points 1y ago

Unexpected issue (for me). I would have expected that the compiler should complain that psize is computed but unused. Why isn't -Wunused-variable enabled in OpenZFS?

pja 1y ago
The blog post suggests that, probably because it’s never been used, it’s just too noisy to turn on. Arguably every unused variable in the code base is a potential bug like this waiting to chomp on some poor user’s data though, so arguably they should be turning it on & dealing with the consequences?
- 0points 1y ago
  > so arguably they should be turning it on & dealing with the consequences? Most definitely, yes.
  - 0points 1y ago
    FWIW: I learned in a sibling comment, that this compiler flag doesn't produce a warning in this specific case anyway (but I think it should!). In general (for any programming language, really), I would advocate for enabling warnings and address them as you go along. Because that unlocks the developer feature of being able to react when code changes introduces warnings, as they generally point to unsound code.

v5v3 1y ago

Gpt4o-mini response: >The provided code snippet is a function that converts an allocation size (asize) into the largest physical size (psize) that can be safely written to an allocation for a given virtual device (vdev). Here are some observations and potential issues in the code: Return Value: The function currently returns asize instead of psize. This seems to be a mistake, as the purpose of the function is to compute and return the psize. The correct return statement should be: return psize; Comment Clarity: The comments mention that the function does not take into account the effect of gang headers. It might be helpful to clarify what "gang headers" are and how they affect the allocation sizes, especially for someone unfamiliar with the context. Variable Initialization: The variable psize is calculated based on asize, ashift, cols, and nparity. Ensure that these values are correctly initialized and that vdev_raidz_get_logical_width returns a valid value for cols. If cols is zero, it could lead to division by zero in the DIV_ROUND_UP macro. Assertions: The assertion ASSERT0(asize % (1 << ashift)); checks that asize is a multiple of 2^ashift. If this assertion fails, it will cause the program to terminate. Ensure that this condition is always met before calling this function, or consider handling this case more gracefully. Macro Usage: The DIV_ROUND_UP macro should be checked to ensure it handles edge cases correctly, such as when psize is zero. If psize is zero, the division could lead to unexpected behavior. Code Readability: Consider adding more comments to explain the logic behind the calculations, especially for someone who may not be familiar with the RAID-Z implementation. Error Handling: There is no error handling in the function. Depending on the context in which this function is used, it might be beneficial to add checks for invalid input values and return an error code or handle exceptions accordingly. Here is a revised version of the function with the return value corrected: .....(Snipped for formatting issues) Make sure to test the function thoroughly to ensure it behaves as expected under various conditions.

v5v3 1y ago

How much would it cost/how long to give an LLM the entire OpenZfs repo and ask it to find every bug?

v5v3 1y ago
To the person who downvoted this... accept the world has changed my friend!! And more so, as the NSA, China, Russia etc will be using llms to comb repo's to find zero day vulnerabilities.

Voultapher 1y ago

I'm surprised their testing didn't catch it. And while I get that turning on warnings in a code-base not designed with them can be annoying, `-Wunused` is certainly within reach. I've enabled and maintained `-Weverything` in a 30m LoC C code base that is older than myself. OpenZFS should try harder to enable such warnings in their CI.

rollcat 1y ago
That's a bit unfair. The size and age of the codebase speaks nothing of its quality. I've thrown out 50k 5yro Python code without a second thought. Also, filesystems are an insanely complex problem, and ZFS is by far the most complex one to be widely deployed.

giingyui 1y ago

[dead]

jmclnx 1y ago

For my personal use, I have no need for ZFS on Linux (ext4) or *BSD (UFS/ffs). I have been playing with NetBSD for a bit and I discovered their FS has a log parameter, using that on mount to me makes their file system almost as good as ext4. Interesting to me it is not advertised too much.

aus10d 1y ago

This was a really good point