Re: [PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

["Michael Büsch" <m@bues.ch>]

[-- Attachment #1: Type: text/plain, Size: 2421 bytes --]

On Fri,  8 Mar 2024 02:29:27 +0300
Rand Deeb <rand.sec96@gmail.com> wrote:

> On Fri, Mar 8, 2024 at 12:39 AM Michael Büsch <m@bues.ch> wrote:
> 
> > The point is that leaving them in is defensive programming against future changes
> > or against possible misunderstandings of the situation.  
> 
> Dear Michael, I understand your point. It's essential to consider defensive
> programming principles to anticipate and mitigate potential issues in the 
> future. However, it's also crucial to strike a balance and not overburden 
> every function with excessive checks. It's about adopting a mindset of 
> anticipating potential problems while also maintaining code clarity and 
> efficiency.

Removing NULL checks is the opposite of maintainability and code clarity.
Efficiency doesn't matter here. (And besides that, NULL checks do not always mean less efficiency.)

> > A NULL pointer dereference is Undefined Behavior.
> > It can't get much worse in C.  
> 
> Again, If we adopt this approach, we'll find ourselves adding a null check 
> to every function we write, assuming that such changes may occur in the 
> future.

This would be a good thing.
Let the compiler remove redundant checks or let them stay there in the resulting
program, if the compiler can't fiure it out.
Checks are a good thing.

> > Your suggestion was about REMOVING a null pointer check.
> > Not about adding one.
> > I NAK-ed the REMOVAL of a null pointer check. Not the addition.  
> 
> My suggestion was to remove a (REDUNDANT) null pointer check, and not a 
> null pointer check, there is a big difference.

No. There is no difference.

> However, if the reviewer encounters this check, they 
> might mistakenly assume that 'dev' could indeed be NULL before the function
> call.

So? Nothing would happen.

> Conversely, if they read that 'dev' cannot be NULL, it could lead to 
> confusion, and perhaps they want the actual null check. Removing redundant 
> checks could mitigate confusion and minimize the risk of overlooking the 
> actual null check for example.

I fundamentally disagree.
Removing a NULL check _adds_ confusion.
NULL is "the billion mistake" of computing.
Please don't ever make it worse.
Thanks.

I will not ack a patch that reduces code quality.
Removing NULL checks almost always reduces the quality of the code.

-- 
Michael Büsch
https://bues.ch/

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]