yamt commented on pull request #695: URL: https://github.com/apache/incubator-nuttx-apps/pull/695#issuecomment-839347064
> > > > the "defensive" style can make it difficult to find such bugs. > > > > > > > > > @yamt It doesn't make sense. If you are following the recommended mitigation methods and therefore preventing these bugs from happening, which bugs are you expecting to find then? > > > > > > unintended uses of values. > > Fine. So as long the recommended mitigation methods are being followed, you won't find them. Better safe than sorry. i don't understand your logic here. > > > > > i certainly don't recommend to apply the method blindly. > > > > (i feel it "blindly" because you are suggesting to add "ctx->ws = NULL" even in a place where it's already known to be NULL.) > > > > > > > > > Indeed, one of the issues I've raised is actually wrong and I acknowledge my mistake. However, my review process is far from being considered "blind", and that was an unfortunate statement of yours. > > > > > > sorry if you felt that way. i have no intention to attack you. > > "blind" might have been an inappropriate word. maybe "mechanical"? > > I accept your apologies, but it is important for you to know that either by saying "blind" or "mechanical" you seem to disregard the fact that I've indeed evaluated the scenario before raising the issue. It is not like I had copied some text and pasted it. Otherwise I wouldn't even waste my time reviewing PRs. ok. > > > > By the way, instead of undervaluing the reviewer's methods, please explain clearly on why an issue is not valid. If I am still wrong on the other raised issues, I'd like to learn from them. > > > > > > if the code is bug-free, the value doesn't matter. > > they should not be used by anyone. > > thus those NULLify should not be necessary. > > The value wouldn't matter if it were stored in a local variable, but it is not the case. Since the lifetime of this dangling pointer is extended, the value does matter. > And all it takes for a software to change from "Bug-free" to "Vulnerable" is one single commit. > And if this single commit naively operates on this dangling pointer, the system will crash. why doesn't a local variable matter? with your logic, i think the value left on stack might be unintentionally used by a buggy software. > > > if there is a bug (i guess there is :-) i prefer it fails loudly. > > that's why i generally don't like the "defensive" methods. > > NuttX is embedded in several types of products, from consumer electronics goods to industrial assets. For the majority of applications (if not all of them) the goal is to maximize the mean time between failures. > So, considering this, your preference for "failing loudly" and "fix bugs if any" does not seem much fit. i don't agree. running with a wrong state is often worse than a crash. it depends. > > > i pushed a counter-proposal to the defensive style. [219356c](https://github.com/apache/incubator-nuttx-apps/commit/219356ce7d1b8a26140c98d303d493c6f04b19e9) > > Good, this commit may be a proof that this code is not **currently** buggy. But that's not the issue I raised. > Now, here it is a simple unit test for making this potential bug evident: > https://godbolt.org/z/f4dzW8M44 the unit test is just broken. it doesn't seem to support your argument. (btw, the website looks cool.) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
