4 ms·

Your post sounds like it's a bad thing, but "nicer" code is easier to maintain, i.e. there will be fewer bugs (and fewer vulnerabilities). This bug is an except

by max_k 5y ago

Your post sounds like it's a bad thing, but "nicer" code is easier to maintain, i.e. there will be fewer bugs (and fewer vulnerabilities). This bug is an exception of the rule - shit happens. But refactoring code to be "nicer" prevents more bugs than it causes. Two patches were involved in making this bug happen, and minus the bug, I value both of them (and their authors).

dncornholio 5y ago
I might have sounded harsh but I think shit happens is not the way to look at this. Don't claim I'm a better developer, but I always try to shy away from making things look nicer. Experience have thought me, deal with problems when it is a problem. Dealing with could be problems can be a deep, very deep rabbit hole. The commit message gave me the feeling that we should have just trust the author. https://github.com/torvalds/linux/commit/f6dd975583bd8ce088400648fd9819e4691c8958 https://github.com/torvalds/linux/commit/f6dd975583bd8ce0884...
- 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.
- Supermancho 5y ago
  Caveat - I know this doesn't directly apply to the vulnerability at hand, but is a discussion of a tangential view. > Experience have thought me, deal with problems when it is a problem. Experience has taught me that disparate ways of doing the same thing tend to have bugs in one or more of the implementations. Then trying to figure out if a specific bug exists other places requires digging into those other places. Make it work. Make it good. Make it faster (as necessary) is the way my long-lived code tends to evolve.
- Traubenfuchs 5y ago
  > I always try to shy away from making things look nicer Anyone who doesn't, hasn't been burnt enough so far, but will be burnt in the future.
  - IshKebab 5y ago
    Nonsense. It's just easy to blame refactoring when it breaks something. "You fool! Why did you change things? It was perfectly fine before.". Much harder to say "Why has this bug been here for 10 years? Why did nobody refactor the code?" even when it would have helped. Not refactoring code also sacrifices long term issues in return for short term risk reduction. Look at all of the government systems stuck on COBOL. I guarantee there was someone in the 90s offering to rewrite it in Java, and someone else saying "no it's too risky!". Then your ancient system crashes in 2022 and nobody knows how it works let alone how to fix it.
- theamk 5y ago
  A lot of times, this is just shifting the problem to the future and making life harder. We have a team like this -- their processes often failing, and their error reporting is lacking in important details. But they are not willing to improve reporting / make errors nicer (=with relevant details), instead they have to manually dig into the logs to see what happens. They waste a lot of time because they "shy away from making things look nicer."
Cthulhu_ 5y ago
"There are two ways of constructing a software design: One way is to make it so simple that there are obviously no deficiencies and the other way is to make it so complicated that there are no obvious deficiencies." — C.A.R. Hoare, The 1980 ACM Turing Award Lecture
throwawayboise 5y ago
Unless the code is very well covered by unit tests, any refactoring can introduce bugs. If the code is well established and no longer changing, there is no ease of maintenance to be gained. There is only downside to changing it. If the code is causing more work to maintenance and new development, sure it may make sense to refactor it. Otherwise, like the human appendix, just leave it alone until it causes a problem.

by max_k 5y ago

dncornholio 5y ago

I might have sounded harsh but I think shit happens is not the way to look at this. Don't claim I'm a better developer, but I always try to shy away from making things look nicer. Experience have thought me, deal with problems when it is a problem. Dealing with could be problems can be a deep, very deep rabbit hole. The commit message gave me the feeling that we should have just trust the author. https://github.com/torvalds/linux/commit/f6dd975583bd8ce088400648fd9819e4691c8958 https://github.com/torvalds/linux/commit/f6dd975583bd8ce0884...

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.
Supermancho 5y ago
Caveat - I know this doesn't directly apply to the vulnerability at hand, but is a discussion of a tangential view. > Experience have thought me, deal with problems when it is a problem. Experience has taught me that disparate ways of doing the same thing tend to have bugs in one or more of the implementations. Then trying to figure out if a specific bug exists other places requires digging into those other places. Make it work. Make it good. Make it faster (as necessary) is the way my long-lived code tends to evolve.
Traubenfuchs 5y ago
> I always try to shy away from making things look nicer Anyone who doesn't, hasn't been burnt enough so far, but will be burnt in the future.
- IshKebab 5y ago
  Nonsense. It's just easy to blame refactoring when it breaks something. "You fool! Why did you change things? It was perfectly fine before.". Much harder to say "Why has this bug been here for 10 years? Why did nobody refactor the code?" even when it would have helped. Not refactoring code also sacrifices long term issues in return for short term risk reduction. Look at all of the government systems stuck on COBOL. I guarantee there was someone in the 90s offering to rewrite it in Java, and someone else saying "no it's too risky!". Then your ancient system crashes in 2022 and nobody knows how it works let alone how to fix it.
theamk 5y ago
A lot of times, this is just shifting the problem to the future and making life harder. We have a team like this -- their processes often failing, and their error reporting is lacking in important details. But they are not willing to improve reporting / make errors nicer (=with relevant details), instead they have to manually dig into the logs to see what happens. They waste a lot of time because they "shy away from making things look nicer."

Cthulhu_ 5y ago

"There are two ways of constructing a software design: One way is to make it so simple that there are obviously no deficiencies and the other way is to make it so complicated that there are no obvious deficiencies." — C.A.R. Hoare, The 1980 ACM Turing Award Lecture

throwawayboise 5y ago

Unless the code is very well covered by unit tests, any refactoring can introduce bugs. If the code is well established and no longer changing, there is no ease of maintenance to be gained. There is only downside to changing it. If the code is causing more work to maintenance and new development, sure it may make sense to refactor it. Otherwise, like the human appendix, just leave it alone until it causes a problem.