From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH 1/2] notes: clean up confusing NULL checks in init_notes()
Date: Mon, 24 Apr 2023 17:57:19 -0400 [thread overview]
Message-ID: <20230424215719.GA3998354@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqqttx5duki.fsf@gitster.g>
On Mon, Apr 24, 2023 at 11:05:49AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Coverity complains that we check whether "notes_ref" is NULL, but it was
> > already implied to be non-NULL earlier in the function. And this is
> > true; since b9342b3fd63 (refs: add array of ref namespaces, 2022-08-05),
> > we call xstrdup(notes_ref) unconditionally, which would segfault if it
> > was NULL.
> >
> > But that commit is actually doing the right thing. Even if NULL is
> > passed into the function, we'll use default_notes_ref() as a fallback,
> > which will never return NULL (it tries a few options, but its last
> > resort is a string literal). Ironically, the "!notes_ref" check was
> > added by the same commit that added the fallback: 709f79b0894 (Notes
> > API: init_notes(): Initialize the notes tree from the given notes ref,
> > 2010-02-13). So this check never did anything.
>
> I am impressed(?) that Coverity can complain at the "_or_null" part
> in xstrdup_or_null().
No, my human brain added that part while I was looking at the function.
Coverity is definitely clever enough to realize that the NULL check in
xstrdup_or_null() is not needed here (it's a static inline, but I think
Coverity can even look between translation units). But complaining about
it would yield lots of false positives. It's redundant in this instance,
but not in other callers of the function.
So it would have to realize: we called xstrdup_or_null(), but there is
another function xstrdup() which is exactly the same but without the
NULL check. And I think that is asking too much of even a very clever
static analyzer. :)
-Peff
next prev parent reply other threads:[~2023-04-24 21:57 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-22 13:54 [PATCH 0/2] coverity null-check fixes Jeff King
2023-04-22 13:55 ` [PATCH 1/2] notes: clean up confusing NULL checks in init_notes() Jeff King
2023-04-24 18:05 ` Junio C Hamano
2023-04-24 21:57 ` Jeff King [this message]
2023-04-22 13:56 ` [PATCH 2/2] fetch_bundle_uri(): drop pointless NULL check Jeff King
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=20230424215719.GA3998354@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).