10 ms·

If I understand correctly, everything is working as intended: a fuzzer caught a bug in an unreleased version of clang. The title makes it sound like somebody fu

by klysm 6y ago

If I understand correctly, everything is working as intended: a fuzzer caught a bug in an unreleased version of clang. The title makes it sound like somebody fucked up pretty badly.

JDevlieghere 6y ago
Indeed, the commit hash reported in the version corresponds to something that landed on master at 11PM this Monday.
- sitkack 6y ago
  Clang is the lucky one here, it has both SQLite and Dr Hipp on its side. Rust has Crater which allows new rustc builds to be run against the entire public crates.io dgraph. https://github.com/rust-lang/crater https://github.com/rust-lang/crater
  - zoomablemind 6y ago
    > Clang is the lucky one here, it has both SQLite and Dr Hipp on its side. Clang is lucky that OSSFuzz does testing of unreleased clang version on other OSS projects, like SQLite.
    - renewiltord 6y ago
      Love it. This is the dream of open-source.
schlupa 6y ago
It is a fuck up. Ignoring a sequence point (function call) is serious business. Probably a too agressive inlining.
- saagarjha 6y ago
  Relax, it’s a bug in a prerelease version of a compiler that was caught early and fixed promptly. Sure, it’s a bug in an important part of the compiler, but there’s not need to call foul.
  - sz4kerto 6y ago
    It might not be that simple: bugs like this should preferably be caught by the test suite of the compiler.
    - saagarjha 6y ago
      It should, but I don’t think it’s fair to say it’s a huge issue. Perhaps it’s just highlighting an area of improvement for the test suite or perhaps a need for more caution when working in that area of the compiler.
    - kryptiskt 6y ago
      Anything and everything with tests implemented in the right languages can be used as a test suite for a compiler, though. So, I'd rather see that that was done systematically than trying to catch everything with the compiler's test suite. I mean something like setting up something building and running the tests for everything included in Debian, homebrew and vcpkg and looking for regressions compared to the last release of the compiler.
    - fluffything 6y ago
      LLVM is one of the most critical pieces of infrastructure out there. And yet you can modify master without actually passing any tests. I build LLVM from source a lot, and master not even building because somebody made a change that doesn't even compiler is astonishing. FWIW, LLVM has a pretty big test suite. But... to catch errors... you actually... have to... run it... It is often the case that Rust, which does have a policy of "master always passes all tests" catches bugs in LLVM before LLVM does.
      - mehrdadn 6y ago
        Honestly "master doesn't pass tests" is just a naming problem. Solving is just a matter of renaming master to something else ('dev'? 'potentially-incorrect'?) and making a new version of it that does pass tests.
fluffything 6y ago
Not really, at least, not given the current info. Does anyone have a link to the reported clang bug? For example, it might well be that the function that clang believes does not modify pmem->flags "promises" that it does not modify the value. In which case it might be an SQLite bug, that clang just did not happen to "correctly optimize" before. This can happen, e.g., if you mark your function with __attribute__((const)), for example, which promises that your function does not access memory via arguments that have pointers. It then becomes trivial for clang to make the transformation above. The bug wouldn't even need to be on the function definition, it might just be on the declaration, or on a declaration somewhere, in which case you have an ODR violation as well. All that seems very unlikely since it appears to be clear that the function does modify memory, but still, I'd just wait for the clang bug to be filled with a reproducer, and the issue to be tracked down. It does strongly hint at a bug in clang, but from the linked page, I at least cannot conclude that yet.
- mrjin 6y ago
  Please, the code is there, read it rather than make assumptions this might happen or that might happen. It's just plain C code all the way there was no fancy "const" stuff anywhere involved in this case. Just as claimed, CLang tried to do some optimization and unfortunately it was FUBAR.
  - deleted 6y ago
    [deleted]
  - fluffything 6y ago
    Sorry, but no. As a compiler writer, the burden of proof that this is a bug isn't on me, its on you. If you believe this is a bug in clang, open up a bug with a reproducer, and get somebody to analyze it and confirm it. SQLite isn't a tiny project, so doing that should be quite easy. I have better things to do than reading the whole SQLite code base like, for example, fixing the bugs of the people that actually bothered to fill in a bug report. Until this bug is confirmed, it can be anything, and unfortunately, bugs reported for C and C++ projects are quite often not bugs in the compiler, but UB in user code. The fact that you think that just by looking at a single function signature one can deduce where this could be UB related to const depicts the problem. UB is unfortunately not that simple, and I can think of a couple of ways of const-related UB in which the declaration "looked fine".
globular-toast 6y ago
A fuzzer should be one of the last lines of defence against human errors. This was also caught by fuzzing sqlite, not the offending compiler clang. When all but the last lines of defence are breached it's absolutely cause for concern and shouldn't be brushed away like all is fine.
- jfkebwjsbx 6y ago
  Testing compiler outputs by testing popular libraries an apps generated with it is a fine and common approach. Expecting a compiler (and the hardware it runs on) to be perfect is a mistake.
  - thechao 6y ago
    Except for compcert, which is basically perfect.
    - jraph 6y ago
      Well, not quite, because there are parts of CompCert that are not proven [1]: - Parsing, type-checking, and pre-simplifications - Assembling and linking And there have been bugs on edge cases there. Moreover, correctness is limited to what is specified. COQ does not guarantee that there are no missing specifications or theorems. The C standard is written in English, people had to manually encode the standard in COQ. [1] http://compcert.inria.fr/compcert-C.html http://compcert.inria.fr/compcert-C.html
      - touisteur 6y ago
        And the performance of the generated binary might be underwhelming for some.
  - cesarb 6y ago
    It's all right if it's running the testcases of these popular libraries; however, it seems that this was not caught by the testcases, only by a fuzzer (which not only is not as deterministic, but also can be much more costly to run).
iforgotpassword 6y ago
No, the point here is that the fuzzer "caught" a bug in sqlite. The blog post mostly clears things up. To me it seemed pretty neutral just explaining it was actually with clang. No snarkyness our anything.
- tom_mellior 6y ago
  > No, the point here is that the fuzzer "caught" a bug in sqlite. The blog post mostly clears things up. Could you explain what you mean? What bug is there in sqlite? What blog post are you referring to? Via someone else in the thread, this was reported as a bug in LLVM, which was confirmed and is now fixed: https://bugs.llvm.org/show_bug.cgi?id=46194 https://bugs.llvm.org/show_bug.cgi?id=46194. In light of this, I wonder why you claim that this was a bug in sqlite.
  - iforgotpassword 6y ago
    There is no bug in sqlite. That's the reason this blog post was written. And that's why it was interesting. Not because anyone claimed clang has a major fuckup.
    - loeg 6y ago
      You wrote earlier: > No, the point here is that the fuzzer "caught" a bug in sqlite. Which is where the confusion stems from. Perhaps "'caught a bug' in sqlite" would have been a more clear way of writing what you intended?
      - iforgotpassword 6y ago
        Ah I see now. Yes I could have extended the quotation. I was assuming the context was given by the start of the second paragraph: > OSSFuzz has been reported bug 23003 against SQLite.
  - SQLite 6y ago
    What happened: 1. OSSFuzz reports a bug against SQLite. 2. SQLite dev tries to fix the reported problem but is unable to repro the bug on his desktop 3. SQLite dev replicates the OSSFuzz build environment which uses clang-11.0.0 and is then able to repro the reported bug 4. SQLite dev finds that the bug isn't in SQLite at all, but rather in clang-11.0.0 5. SQLite dev patches SQLite to work around the clang bug, posts a brief note about this on the SQLite forum, and goes to bed. 6. The post on the SQLite forum is picked up by HN while the SQLite dev is asleep. LLVM devs read HN, isolate and fix the clang problem, all before the SQLite dev wakes up.
jfkebwjsbx 6y ago
What strikes me as odd is why SQLite is committing changes just to make an unreleased compiler happy...
- Tomte 6y ago
  I read the SQLite commit such that they weren't aware it's not a released compiler.
- SQLite 6y ago
  It was late at night. I didn't realize the clang-11.0.0 was unreleased until I saw this HN thread the next morning. The only reason I was using clang-11.0.0 is because the OSSFuzz bug report against SQLite used clang-11.0.0 and I couldn't get the (alleged) SQLite problem to repro using any other compiler.
- Someone 6y ago
  I think it’s a code improvement, too. It now computes the new flags in one place. Just looking at these lines, I don’t understand the code, though. Apparently, sqlite3VdbeMemRelease can change pMem->flags, but that change can be ignored. I can’t think of a simple explanation for that. Also, the function name sqlite3VdbeMemRelease seems to indicate memory gets released, but pMem->flags = MEM_Str|MEM_Term…, to me, indicates it contains a string.
  - jfkebwjsbx 6y ago
    Perhaps, but the justification behind the change should be that one, not the miscompilation...
  - dathinab 6y ago
    > but that change can be ignore No the changes _must_ be ignored. This is where the bug came from. The code does store the flags, does an operation and then restores some flags while setting some new ones. But after clang reorders the operations it will not restore the flags from _before_ the function calls but instead takes them from _after_ the function call. I.e. any change `sqlite3VdbeMemRelease` does to `pMem` mutst be ignored but after reordering changes to `MEM_AffMask` `MEM_Subtype` are no longer ignored.

by klysm 6y ago

If I understand correctly, everything is working as intended: a fuzzer caught a bug in an unreleased version of clang. The title makes it sound like somebody fucked up pretty badly.

JDevlieghere 6y ago

Indeed, the commit hash reported in the version corresponds to something that landed on master at 11PM this Monday.

sitkack 6y ago
Clang is the lucky one here, it has both SQLite and Dr Hipp on its side. Rust has Crater which allows new rustc builds to be run against the entire public crates.io dgraph. https://github.com/rust-lang/crater https://github.com/rust-lang/crater
- zoomablemind 6y ago
  > Clang is the lucky one here, it has both SQLite and Dr Hipp on its side. Clang is lucky that OSSFuzz does testing of unreleased clang version on other OSS projects, like SQLite.
  - renewiltord 6y ago
    Love it. This is the dream of open-source.

schlupa 6y ago

It is a fuck up. Ignoring a sequence point (function call) is serious business. Probably a too agressive inlining.

saagarjha 6y ago
Relax, it’s a bug in a prerelease version of a compiler that was caught early and fixed promptly. Sure, it’s a bug in an important part of the compiler, but there’s not need to call foul.
- sz4kerto 6y ago
  It might not be that simple: bugs like this should preferably be caught by the test suite of the compiler.
  - saagarjha 6y ago
    It should, but I don’t think it’s fair to say it’s a huge issue. Perhaps it’s just highlighting an area of improvement for the test suite or perhaps a need for more caution when working in that area of the compiler.
  - kryptiskt 6y ago
    Anything and everything with tests implemented in the right languages can be used as a test suite for a compiler, though. So, I'd rather see that that was done systematically than trying to catch everything with the compiler's test suite. I mean something like setting up something building and running the tests for everything included in Debian, homebrew and vcpkg and looking for regressions compared to the last release of the compiler.
  - fluffything 6y ago
    LLVM is one of the most critical pieces of infrastructure out there. And yet you can modify master without actually passing any tests. I build LLVM from source a lot, and master not even building because somebody made a change that doesn't even compiler is astonishing. FWIW, LLVM has a pretty big test suite. But... to catch errors... you actually... have to... run it... It is often the case that Rust, which does have a policy of "master always passes all tests" catches bugs in LLVM before LLVM does.
    - mehrdadn 6y ago
      Honestly "master doesn't pass tests" is just a naming problem. Solving is just a matter of renaming master to something else ('dev'? 'potentially-incorrect'?) and making a new version of it that does pass tests.

fluffything 6y ago

Not really, at least, not given the current info. Does anyone have a link to the reported clang bug? For example, it might well be that the function that clang believes does not modify pmem->flags "promises" that it does not modify the value. In which case it might be an SQLite bug, that clang just did not happen to "correctly optimize" before. This can happen, e.g., if you mark your function with __attribute__((const)), for example, which promises that your function does not access memory via arguments that have pointers. It then becomes trivial for clang to make the transformation above. The bug wouldn't even need to be on the function definition, it might just be on the declaration, or on a declaration somewhere, in which case you have an ODR violation as well. All that seems very unlikely since it appears to be clear that the function does modify memory, but still, I'd just wait for the clang bug to be filled with a reproducer, and the issue to be tracked down. It does strongly hint at a bug in clang, but from the linked page, I at least cannot conclude that yet.

mrjin 6y ago
Please, the code is there, read it rather than make assumptions this might happen or that might happen. It's just plain C code all the way there was no fancy "const" stuff anywhere involved in this case. Just as claimed, CLang tried to do some optimization and unfortunately it was FUBAR.
- deleted 6y ago
  [deleted]
- fluffything 6y ago
  Sorry, but no. As a compiler writer, the burden of proof that this is a bug isn't on me, its on you. If you believe this is a bug in clang, open up a bug with a reproducer, and get somebody to analyze it and confirm it. SQLite isn't a tiny project, so doing that should be quite easy. I have better things to do than reading the whole SQLite code base like, for example, fixing the bugs of the people that actually bothered to fill in a bug report. Until this bug is confirmed, it can be anything, and unfortunately, bugs reported for C and C++ projects are quite often not bugs in the compiler, but UB in user code. The fact that you think that just by looking at a single function signature one can deduce where this could be UB related to const depicts the problem. UB is unfortunately not that simple, and I can think of a couple of ways of const-related UB in which the declaration "looked fine".

globular-toast 6y ago

A fuzzer should be one of the last lines of defence against human errors. This was also caught by fuzzing sqlite, not the offending compiler clang. When all but the last lines of defence are breached it's absolutely cause for concern and shouldn't be brushed away like all is fine.

jfkebwjsbx 6y ago
Testing compiler outputs by testing popular libraries an apps generated with it is a fine and common approach. Expecting a compiler (and the hardware it runs on) to be perfect is a mistake.
- thechao 6y ago
  Except for compcert, which is basically perfect.
  - jraph 6y ago
    Well, not quite, because there are parts of CompCert that are not proven [1]: - Parsing, type-checking, and pre-simplifications - Assembling and linking And there have been bugs on edge cases there. Moreover, correctness is limited to what is specified. COQ does not guarantee that there are no missing specifications or theorems. The C standard is written in English, people had to manually encode the standard in COQ. [1] http://compcert.inria.fr/compcert-C.html http://compcert.inria.fr/compcert-C.html
    - touisteur 6y ago
      And the performance of the generated binary might be underwhelming for some.
- cesarb 6y ago
  It's all right if it's running the testcases of these popular libraries; however, it seems that this was not caught by the testcases, only by a fuzzer (which not only is not as deterministic, but also can be much more costly to run).

iforgotpassword 6y ago

No, the point here is that the fuzzer "caught" a bug in sqlite. The blog post mostly clears things up. To me it seemed pretty neutral just explaining it was actually with clang. No snarkyness our anything.

tom_mellior 6y ago
> No, the point here is that the fuzzer "caught" a bug in sqlite. The blog post mostly clears things up. Could you explain what you mean? What bug is there in sqlite? What blog post are you referring to? Via someone else in the thread, this was reported as a bug in LLVM, which was confirmed and is now fixed: https://bugs.llvm.org/show_bug.cgi?id=46194 https://bugs.llvm.org/show_bug.cgi?id=46194. In light of this, I wonder why you claim that this was a bug in sqlite.
- iforgotpassword 6y ago
  There is no bug in sqlite. That's the reason this blog post was written. And that's why it was interesting. Not because anyone claimed clang has a major fuckup.
  - loeg 6y ago
    You wrote earlier: > No, the point here is that the fuzzer "caught" a bug in sqlite. Which is where the confusion stems from. Perhaps "'caught a bug' in sqlite" would have been a more clear way of writing what you intended?
    - iforgotpassword 6y ago
      Ah I see now. Yes I could have extended the quotation. I was assuming the context was given by the start of the second paragraph: > OSSFuzz has been reported bug 23003 against SQLite.
- SQLite 6y ago
  What happened: 1. OSSFuzz reports a bug against SQLite. 2. SQLite dev tries to fix the reported problem but is unable to repro the bug on his desktop 3. SQLite dev replicates the OSSFuzz build environment which uses clang-11.0.0 and is then able to repro the reported bug 4. SQLite dev finds that the bug isn't in SQLite at all, but rather in clang-11.0.0 5. SQLite dev patches SQLite to work around the clang bug, posts a brief note about this on the SQLite forum, and goes to bed. 6. The post on the SQLite forum is picked up by HN while the SQLite dev is asleep. LLVM devs read HN, isolate and fix the clang problem, all before the SQLite dev wakes up.

jfkebwjsbx 6y ago

What strikes me as odd is why SQLite is committing changes just to make an unreleased compiler happy...

Tomte 6y ago
I read the SQLite commit such that they weren't aware it's not a released compiler.
SQLite 6y ago
It was late at night. I didn't realize the clang-11.0.0 was unreleased until I saw this HN thread the next morning. The only reason I was using clang-11.0.0 is because the OSSFuzz bug report against SQLite used clang-11.0.0 and I couldn't get the (alleged) SQLite problem to repro using any other compiler.
Someone 6y ago
I think it’s a code improvement, too. It now computes the new flags in one place. Just looking at these lines, I don’t understand the code, though. Apparently, sqlite3VdbeMemRelease can change pMem->flags, but that change can be ignored. I can’t think of a simple explanation for that. Also, the function name sqlite3VdbeMemRelease seems to indicate memory gets released, but pMem->flags = MEM_Str|MEM_Term…, to me, indicates it contains a string.
- jfkebwjsbx 6y ago
  Perhaps, but the justification behind the change should be that one, not the miscompilation...
- dathinab 6y ago
  > but that change can be ignore No the changes _must_ be ignored. This is where the bug came from. The code does store the flags, does an operation and then restores some flags while setting some new ones. But after clang reorders the operations it will not restore the flags from _before_ the function calls but instead takes them from _after_ the function call. I.e. any change `sqlite3VdbeMemRelease` does to `pMem` mutst be ignored but after reordering changes to `MEM_AffMask` `MEM_Subtype` are no longer ignored.