4 ms·

There's no bug in that commit, the commit is correct, it only makes the bug exploitable. The buggy commit is older, it's https://github.com/torvalds/linux/commi

by max_k 5y ago

There's no bug in that commit, the commit is correct, it only makes the bug exploitable. The buggy commit is older, it's https://github.com/torvalds/linux/commit/241699cd72a8489c9446ae3910ddd243e9b9061b https://github.com/torvalds/linux/commit/241699cd72a8489c944... but not exploitable. > I always try to shy away from making things look nicer That's understandable, though from my experience, lots of old bugs can be found while refactoring code, even at the (small) risk of introducing new bugs.

Cthulhu_ 5y ago
While true, it's important to ensure there is adequate test coverage before trying to refactor, in case you miss something. Also, try to avoid small commits / changes; churn in code should be avoided, especially in kernel code. IIRC the Linux project and a lot of open source projects do not accept 'refactoring' pull requests, among other things for this exact reason.
- max_k 5y ago
  Agree, but even 100% test coverage can't catch this kind of bug. I don't know of any systematic testing method which would be able to catch it. Maybe something like valgrind which detects accesses to uninitialized memory, but then you'd still have to execute very special code paths (which is "more" than 100% coverage).
  - aaronmdjones 5y ago
    Valgrind cannot be used for/in the kernel. However, the kernel has an almost-equivalent use-of-uninitialized-memory detector; https://www.kernel.org/doc/html/v4.14/dev-tools/kmemcheck.html https://www.kernel.org/doc/html/v4.14/dev-tools/kmemcheck.ht...
- cwilkes 5y ago
  > try to avoid small commits / changes Not sure what you mean by that
ahartmetz 5y ago
As (almost) always, the expert's answer is: "It depends". How risky is the change, how big the consequences, how un-nice is the code before, how easy is it to test that the code still works afterwards, etc... FWIW, I tend to err on the side of "do it", and I usually do it. But I have been in a situation where a customer asked for the risk level, I answered to the best of my knowledge (quite low but it's hard to be 100% sure), and they declined the change. The consequences of a bug would have been pretty horrible, too. Hundreds of thousands of (things) shipped with buggy software that is somewhat cumbersome to update.

by max_k 5y ago

Cthulhu_ 5y ago

While true, it's important to ensure there is adequate test coverage before trying to refactor, in case you miss something. Also, try to avoid small commits / changes; churn in code should be avoided, especially in kernel code. IIRC the Linux project and a lot of open source projects do not accept 'refactoring' pull requests, among other things for this exact reason.

max_k 5y ago
Agree, but even 100% test coverage can't catch this kind of bug. I don't know of any systematic testing method which would be able to catch it. Maybe something like valgrind which detects accesses to uninitialized memory, but then you'd still have to execute very special code paths (which is "more" than 100% coverage).
- aaronmdjones 5y ago
  Valgrind cannot be used for/in the kernel. However, the kernel has an almost-equivalent use-of-uninitialized-memory detector; https://www.kernel.org/doc/html/v4.14/dev-tools/kmemcheck.html https://www.kernel.org/doc/html/v4.14/dev-tools/kmemcheck.ht...
cwilkes 5y ago
> try to avoid small commits / changes Not sure what you mean by that

ahartmetz 5y ago

As (almost) always, the expert's answer is: "It depends". How risky is the change, how big the consequences, how un-nice is the code before, how easy is it to test that the code still works afterwards, etc... FWIW, I tend to err on the side of "do it", and I usually do it. But I have been in a situation where a customer asked for the risk level, I answered to the best of my knowledge (quite low but it's hard to be 100% sure), and they declined the change. The consequences of a bug would have been pretty horrible, too. Hundreds of thousands of (things) shipped with buggy software that is somewhat cumbersome to update.