Received-SPF: pass (google.com: domain of linux-wireless+bounces-4485-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240307223849.13d5b58b@barney> <20240307232927.171197-1-rand.sec96@gmail.com>
 <20240308060943.2410ef2e@barney>
In-Reply-To: <20240308060943.2410ef2e@barney>
From: Rand Deeb <rand.sec96@gmail.com>
Date: Fri, 8 Mar 2024 14:36:56 +0300
Message-ID: <CAN8dotkp7tQ6znKM5=1iM08hLvjLK-VbQm9LuqcGBf0e3ok=kg@mail.gmail.com>
Subject: Re: [PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent
To: =?UTF-8?Q?Michael_B=C3=BCsch?= <m@bues.ch>
Cc: deeb.rand@confident.ru, jonas.gorski@gmail.com, khoroshilov@ispras.ru, 
	kvalo@kernel.org, linux-kernel@vger.kernel.org, 
	linux-wireless@vger.kernel.org, lvc-project@linuxtesting.org, 
	voskresenski.stanislav@confident.ru, james.dutton@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Mar 8, 2024 at 8:11=E2=80=AFAM Michael B=C3=BCsch <m@bues.ch> wrote=
:
>
> On Fri,  8 Mar 2024 02:29:27 +0300
> Rand Deeb <rand.sec96@gmail.com> wrote:
>
> > On Fri, Mar 8, 2024 at 12:39=E2=80=AFAM Michael B=C3=BCsch <m@bues.ch> =
wrote:
> >
> > > The point is that leaving them in is defensive programming against fu=
ture changes
> > > or against possible misunderstandings of the situation.
> >
> > Dear Michael, I understand your point. It's essential to consider defen=
sive
> > programming principles to anticipate and mitigate potential issues in t=
he
> > future. However, it's also crucial to strike a balance and not overburd=
en
> > 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 alw=
ays 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 ch=
eck
> > 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 re=
sulting
> 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 func=
tion
> > 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.