4 ms·

Another bit of cleverness not mentioned in the article is that assignment expressions always evaluate to the rvalue. So the expression `current->uid = 0` has th

Another bit of cleverness not mentioned in the article is that assignment expressions always evaluate to the rvalue. So the expression `current->uid = 0` has the effect of making sure that entire conditional never actually runs (or at least, the return never runs), which means the overall behavior of wait4 doesn't change in an observable way. Very clever if you're trying to pass all of the existing tests

AnimalMuppet 3y ago
Ohh, that is clever - unless someone writes a test for these two new lines, and finds that they never return -EINVAL.
- TheJoeMan 3y ago
  Maybe the unit tests should test that the variables you intend to "not touch" do not in fact change. So record UID before and after the function. When I'm writing critical code like this, one gets a tingly feeling when typing in that variable name.
- marssaxman 3y ago
  Did unit tests exist in 2003? I don't clearly remember when that idea came along, but comprehensive unit testing certainly was not standard practice 20 years ago... not in any organization I knew about at the time, anyway!
  - usr1106 3y ago
    I believe unit tests existed. I had a test training in 2000 and things were pretty systematic already then. Not 100% sure whether the exact term was used then. Edit: JUnit is from 1997. So the name was definitely in use in 2003. I attended a TDD tutorial before 2004 (don't remember the exact year). CI wasn't a thing yet, so you executed your unit tests manually. /edit Do unit tests exist in the kernel today? There is some (or some would say a lot) of automatic testing for the kernel, but I don't remember seeing a single unit test.
GuB-42 3y ago
But that should be something the compiler could catch. The expression is always false and the condition would never be executed. You usually get a warning for that. And if the compiler doesn't, linters do. This is a common mistake, and I believe most linters have rules for that. And I don't think there is any situation where there is a good reason for code like this to exist. Either the expression is wrong, or it doesn't belong in a "if". You may get stuff like that in legitimate code with macro expansion, but again, it is not the case here, and from my experience, you get a warning anyways.
- _notreallyme_ 3y ago
  You didn't get a warning for that in 2003... or maybe with the pedantic flag. And even if it were the case, it would have been drowned by all the other unfixed warnings... The only people using linters at that time was because it was forced by regulation (like automotive, aeronautics, ...)
- lelanthran 3y ago
  > But that should be something the compiler could catch. Today, certainly. My compiler even catches errors in the format strings to printf[1]. But back then? I doubt it, even with all the warnings turned up. [1] Removing yet another common source of bugs.
- diath 3y ago
  The compiler does not, but tools like clang-tidy do, (bugprone-assignment-in-if-condition in clang-tidy) but that didn't exist back then.
- rcxdude 3y ago
  Even now, and I'm pretty sure back then, the relevant warning in GCC is suppressed if the assignment occurs in parentheses, like it did in the diff (clang I believe has the same behaviour). So you would likely need a static analysis tool to flag up the behaviour, and those are quite noisy at the best of times.

AnimalMuppet 3y ago

Ohh, that is clever - unless someone writes a test for these two new lines, and finds that they never return -EINVAL.

TheJoeMan 3y ago
Maybe the unit tests should test that the variables you intend to "not touch" do not in fact change. So record UID before and after the function. When I'm writing critical code like this, one gets a tingly feeling when typing in that variable name.
marssaxman 3y ago
Did unit tests exist in 2003? I don't clearly remember when that idea came along, but comprehensive unit testing certainly was not standard practice 20 years ago... not in any organization I knew about at the time, anyway!
- usr1106 3y ago
  I believe unit tests existed. I had a test training in 2000 and things were pretty systematic already then. Not 100% sure whether the exact term was used then. Edit: JUnit is from 1997. So the name was definitely in use in 2003. I attended a TDD tutorial before 2004 (don't remember the exact year). CI wasn't a thing yet, so you executed your unit tests manually. /edit Do unit tests exist in the kernel today? There is some (or some would say a lot) of automatic testing for the kernel, but I don't remember seeing a single unit test.

GuB-42 3y ago

But that should be something the compiler could catch. The expression is always false and the condition would never be executed. You usually get a warning for that. And if the compiler doesn't, linters do. This is a common mistake, and I believe most linters have rules for that. And I don't think there is any situation where there is a good reason for code like this to exist. Either the expression is wrong, or it doesn't belong in a "if". You may get stuff like that in legitimate code with macro expansion, but again, it is not the case here, and from my experience, you get a warning anyways.

_notreallyme_ 3y ago
You didn't get a warning for that in 2003... or maybe with the pedantic flag. And even if it were the case, it would have been drowned by all the other unfixed warnings... The only people using linters at that time was because it was forced by regulation (like automotive, aeronautics, ...)
lelanthran 3y ago
> But that should be something the compiler could catch. Today, certainly. My compiler even catches errors in the format strings to printf[1]. But back then? I doubt it, even with all the warnings turned up. [1] Removing yet another common source of bugs.
diath 3y ago
The compiler does not, but tools like clang-tidy do, (bugprone-assignment-in-if-condition in clang-tidy) but that didn't exist back then.
rcxdude 3y ago
Even now, and I'm pretty sure back then, the relevant warning in GCC is suppressed if the assignment occurs in parentheses, like it did in the diff (clang I believe has the same behaviour). So you would likely need a static analysis tool to flag up the behaviour, and those are quite noisy at the best of times.