Re: gpg-related crash with custom formatter (BUG: gpg-interface.c:915: invalid trust level requested -1)

From: Jaydeep Das <jaydeepjd.8914@gmail.com>
Date: Apr 18, 2023, 15:16 UTC
Message-ID: <CACaPSotwDfDXk=kR7LntF46NN-cM-zx22T83m1sjYEDLbnSxNQ@mail.gmail.com>
In-Reply-To: <20230418064846.GA1414@coredump.intra.peff.net>

Thanks for the report.

The patch did not preserve the exact behaviour of the previous code. Rather than calling BUG() whenever a trust level is out of the sigcheck_gpg_trust_level[] array, we can simply return an empty string

if (level < 0 || level >= ARRAY_SIZE(sigcheck_gpg_trust_level))
        return "";

It will replicate the exact behaviour as the previous code. But as Jeff pointed out, Should this really be the defined behavior?

Let me know what you think. I will make the necessary changes.

Thanks, Jaydeep.

On Tue, Apr 18, 2023 at 12:18 PM Jeff King <peff@peff.net> wrote:

Show 44 quoted lines

>
> On Tue, Apr 18, 2023 at 08:12:03AM +0200, Rolf Eike Beer wrote:
>
> > When I now run "git log" in a repository that contains commits signed by
> > people not in my keyring (e.g. the Gentoo git) I get this backtrace:
> >
> > BUG: gpg-interface.c:915: invalid trust level requested -1
>
> Thanks for giving an example repo. After cloning:
>
>   https://anongit.gentoo.org/git/repo/gentoo.git
>
> I can reproduce just by running "git log -1 --format=%GT". Bisecting
> turns up 803978da49 (gpg-interface: add function for converting trust
> level to string, 2022-07-11), which is not too surprising.
>
> Before that we returned an empty string. I don't know if the fix is a
> simple as:
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index aceeb08336..edb0da1bda 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -934,7 +934,10 @@ const char *gpg_trust_level_to_str(enum signature_trust_level level)
>  {
>         struct sigcheck_gpg_trust_level *trust;
>
> -       if (level < 0 || level >= ARRAY_SIZE(sigcheck_gpg_trust_level))
> +       if (level < 0)
> +               return "";
> +
> +       if (level >= ARRAY_SIZE(sigcheck_gpg_trust_level))
>                 BUG("invalid trust level requested %d", level);
>
>         trust = &sigcheck_gpg_trust_level[level];
>
> which restores the original behavior, or if the original was papering
> over another bug (e.g., should this be "undefined"?). Certainly the
> empty string matches other placeholders like %GS for this case (since we
> obviously don't know anything about the signer).
>
> +cc folks who worked on 803978da49.
>
> -Peff