From: Rand Deeb <rand.sec96@gmail.com>
To: 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, rand.sec96@gmail.com,
voskresenski.stanislav@confident.ru, james.dutton@gmail.com
Subject: Re: [PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent
Date: Fri, 8 Mar 2024 02:29:27 +0300 [thread overview]
Message-ID: <20240307232927.171197-1-rand.sec96@gmail.com> (raw)
In-Reply-To: <20240307223849.13d5b58b@barney>
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.
> > I understand and respect your point of view as software engineer but it's a
> > matter of design problems which is not our case here.
>
> No, it very well is.
I'm talking about your phrase "Not having these checks is a big part of why
security sucks in today's software."
I think it's a matter of design problem, when you don't have a good design
of course you'll need to add so many checks everywhere.
Let me explain my point of view by example,
// Good design
CHECK(x){
if x != null && x is a number
return true;
else return false;
}
MULTIPLY(a, b){
return a*b;
}
SUM(a, b){
return a+b;
}
....
MAIN(){
// input a, b
CHECK(a);
CHECK(b);
// now do the operations
SUM(a, b)
MULTIPLY(a, b)
}
// Bad design
SUM(x, y){
if x != null && x is a number
return x+y;
}
MULTIPLY(x, y){
if x != null && x is a number
return x*y;
}
...
> 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.
> 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.
Would you please check the link in the previous comment about a similar
situation got accepted and applied.
> Absolutely wrong.
> Not having a NULL check complicates reviews.
> Reviewers will have to prove that pointers cannot be NULL, if there is no check.
> Removing this check would improve nothing.
With all due respect, I respectfully disagree with you on this point. In
your prior comment, you stated, "it is clearly documented that the pointer
can never be NULL" However, if the reviewer encounters this check, they
might mistakenly assume that 'dev' could indeed be NULL before the function
call. 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.
--
Best Regards
Rand Deeb
next prev parent reply other threads:[~2024-03-07 23:29 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-06 12:30 [PATCH v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent Rand Deeb
2024-03-06 15:54 ` Jeff Johnson
2024-03-06 19:51 ` Jonas Gorski
2024-03-07 13:41 ` Rand Deeb
2024-03-07 18:24 ` Michael Büsch
2024-03-07 21:19 ` Rand Deeb
2024-03-07 21:38 ` Michael Büsch
2024-03-07 22:02 ` James Dutton
2024-03-08 4:50 ` Michael Büsch
2024-03-07 23:29 ` Rand Deeb [this message]
2024-03-08 1:04 ` James Dutton
2024-03-08 12:11 ` Rand Deeb
2024-03-08 5:09 ` Michael Büsch
2024-03-08 11:36 ` Rand Deeb
2024-03-08 15:44 ` Michael Büsch
2024-03-12 15:31 ` [v3] ssb: Fix potential NULL pointer dereference in ssb_device_uevent() Kalle Valo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240307232927.171197-1-rand.sec96@gmail.com \
--to=rand.sec96@gmail.com \
--cc=deeb.rand@confident.ru \
--cc=james.dutton@gmail.com \
--cc=jonas.gorski@gmail.com \
--cc=khoroshilov@ispras.ru \
--cc=kvalo@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=lvc-project@linuxtesting.org \
--cc=m@bues.ch \
--cc=voskresenski.stanislav@confident.ru \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).