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

[Rand Deeb]

On Fri, Mar 8, 2024 at 8:11 AM Michael Büsch <m@xxxxxxx> wrote:
>
> On Fri,  8 Mar 2024 02:29:27 +0300
> Rand Deeb <rand.sec96@xxxxxxxxx> wrote:
>
> > On Fri, Mar 8, 2024 at 12:39 AM Michael Büsch <m@xxxxxxx> 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.)

I respect your opinion, but it seems we are stuck in a while(1) loop
without a break. Again, I don't agree with this. Adding a redundant null
check goes against code clarity instead of enhancing it. Just because the
condition checks for null does not justify its presence if it's redundant.
I could insert this check every two lines.
You advocate for this approach based on the potential occurrence in the
future. However, this is one of the reasons why there are reviewers like
yourself are responsible and authorized to approve or reject patches and
verify their integrity before acceptance.

> > > 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.

Our discussion isn't about what the compiler will do; I know that. The
discussion revolves around the code itself. Alright, let's add a null
check for the 'env' parameter as well. Perhaps we could even automate this
process and add null checks for each function in the file.

> > > 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.

Yes there is !
The check is literally redundant. Whether we keep it or remove it, the
outcome remains the same. In this case maybe it's not a big deal, but
adopting this approach could lead to an accumulation of redundant
statements throughout the codebase

> > 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.

add completely unnecessary confusion?

> I will not ack a patch that reduces code quality.
> Removing NULL checks almost always reduces the quality of the code.
Even at this point there is a misunderstanding. The whole discussion is not
about the ack because you've already given it to the first version.
The discussion is because i'm interested in knowing different points of
views.
I think this discussion took more than its time.
This represents your personal point of view, with which I disagree.
Thank you again, and you can do whatever you want, continue with the first
or third version of the patch.