* gpg-related crash with custom formatter (BUG: gpg-interface.c:915: invalid trust level requested -1)
@ 2023-04-18 6:12 Rolf Eike Beer
2023-04-18 6:48 ` Jeff King
2023-04-18 16:17 ` gpg-related crash with custom formatter (BUG: gpg-interface.c:915: invalid trust level requested -1) Junio C Hamano
0 siblings, 2 replies; 9+ messages in thread
From: Rolf Eike Beer @ 2023-04-18 6:12 UTC (permalink / raw
To: git
[-- Attachment #1: Type: text/plain, Size: 3957 bytes --]
I use this one:
[format]
pretty = %C(yellow)commit %H%C(auto)%d%Creset%nAuthor: %an <%ae>
%C(yellow)% GK %GS %C(auto)[%GT% G?]%Creset%nDate: %ad%n%n%w(0,4,4)%s%n%w(0,
4,4)%+b
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
Program received signal SIGABRT, Aborted.
0x00007ffff7d69d7c in __pthread_kill_implementation () from /lib64/libc.so.6
Missing separate debuginfos, use: zypper install libpcre2-8-0-
debuginfo-10.42-3.6.x86_64 libz1-x86-64-v3-debuginfo-1.2.13-3.4.x86_64
(gdb) bt
#0 0x00007ffff7d69d7c in __pthread_kill_implementation () from /lib64/libc.so.6
#1 0x00007ffff7d18356 in raise () from /lib64/libc.so.6
#2 0x00007ffff7d00897 in abort () from /lib64/libc.so.6
#3 0x00005555558041fe in BUG_vfl (params=0x7fffffffb9e0, fmt=0x555555895608
"invalid trust level requested %d", line=<optimized out>, file=<optimized out>)
at /usr/src/debug/git-2.40.0/usage.c:313
#4 BUG_fl (file=<optimized out>, line=<optimized out>, fmt=0x555555895608
"invalid trust level requested %d") at /usr/src/debug/git-2.40.0/usage.c:330
#5 0x000055555576a816 in gpg_trust_level_to_str (level=<optimized out>) at /
usr/src/debug/git-2.40.0/gpg-interface.c:915
#6 format_commit_one (sb=0x7fffffffbf40, placeholder=0x555555936b65 "GT% G?]
%Creset%nDate: %ad%n%n%w(0,4,4)%s%n%w(0,4,4)%+b", context=0x7fffffffbde0) at /
usr/src/debug/git-2.40.0/pretty.c:1617
#7 0x000055555576ab1e in format_commit_item (sb=0x7fffffffbf40,
placeholder=0x555555936b65 "GT% G?]%Creset%nDate: %ad%n%n%w(0,4,4)%s%n%w(0,
4,4)%+b", context=0x7fffffffbde0) at /usr/src/debug/git-2.40.0/pretty.c:1844
#8 0x00005555557cfc44 in strbuf_expand (sb=0x7fffffffbf40, format=0x555555936b65
"GT% G?]%Creset%nDate: %ad%n%n%w(0,4,4)%s%n%w(0,4,4)%+b", fn=0x55555576a8d0
<format_commit_item>, context=0x7fffffffbde0) at /usr/src/debug/git-2.40.0/
strbuf.c:429
#9 0x000055555576af0e in repo_format_commit_message (r=0x555555928540
<the_repo>, commit=0x55555594e2d0, format=<optimized out>, sb=0x7fffffffbf40,
pretty_ctx=<optimized out>) at /usr/src/debug/git-2.40.0/pretty.c:1910
#10 0x000055555571ad10 in show_log (opt=opt@entry=0x7fffffffc570) at /usr/src/
debug/git-2.40.0/log-tree.c:781
#11 0x000055555571bd17 in log_tree_commit (opt=0x7fffffffc570, commit=<optimized
out>) at /usr/src/debug/git-2.40.0/log-tree.c:1117
#12 0x00005555555e9638 in cmd_log_walk_no_free (rev=0x7fffffffc570) at builtin/
log.c:508
#13 0x00005555555e9d5a in cmd_log_walk (rev=0x7fffffffc570) at builtin/log.c:549
#14 cmd_log (argc=1, argv=0x7fffffffd7c0, prefix=0x0) at builtin/log.c:883
#15 0x00005555555782bb in run_builtin (argv=0x7fffffffd7c0, argc=1,
p=0x5555558fa000 <commands.lto_priv+1440>) at /usr/src/debug/git-2.40.0/git.c:
445
#16 handle_builtin (argc=1, argv=0x7fffffffd7c0) at /usr/src/debug/git-2.40.0/
git.c:699
#17 0x00005555555787e7 in run_argv (argcp=0x7fffffffd50c, argv=0x7fffffffd530) at /
usr/src/debug/git-2.40.0/git.c:763
#18 0x000055555557464b in cmd_main (argv=<optimized out>, argc=<optimized
out>) at /usr/src/debug/git-2.40.0/git.c:898
#19 main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/
git-2.40.0/common-main.c:57
This is not absolutely new, it is broken for a while but I was too lazy to
report (sorry about that). It has worked in the past, i.e. when I created that
formatter (IIRC at least 2 years ago).
System is an openSUSE Tumbleweed installation on amd64.
Greetings,
Eike
--
Rolf Eike Beer, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11
Gothaer Platz 3, 37083 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführung: Heike Jordan, Dr. Uwe Kracke – Ust-IdNr.: DE 205 198 055
emlix - smart embedded open source
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 313 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: gpg-related crash with custom formatter (BUG: gpg-interface.c:915: invalid trust level requested -1)
2023-04-18 6:12 gpg-related crash with custom formatter (BUG: gpg-interface.c:915: invalid trust level requested -1) Rolf Eike Beer
@ 2023-04-18 6:48 ` Jeff King
2023-04-18 15:16 ` Jaydeep Das
2023-04-18 16:24 ` Junio C Hamano
2023-04-18 16:17 ` gpg-related crash with custom formatter (BUG: gpg-interface.c:915: invalid trust level requested -1) Junio C Hamano
1 sibling, 2 replies; 9+ messages in thread
From: Jeff King @ 2023-04-18 6:48 UTC (permalink / raw
To: Rolf Eike Beer; +Cc: git, Jaydeep P Das, Hariom Verma, Christian Couder
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: gpg-related crash with custom formatter (BUG: gpg-interface.c:915: invalid trust level requested -1)
2023-04-18 6:48 ` Jeff King
@ 2023-04-18 15:16 ` Jaydeep Das
2023-04-18 16:24 ` Junio C Hamano
1 sibling, 0 replies; 9+ messages in thread
From: Jaydeep Das @ 2023-04-18 15:16 UTC (permalink / raw
To: Jeff King
Cc: Rolf Eike Beer, git, Hariom Verma, Christian Couder,
Junio C Hamano
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:
>
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: gpg-related crash with custom formatter (BUG: gpg-interface.c:915: invalid trust level requested -1)
2023-04-18 6:12 gpg-related crash with custom formatter (BUG: gpg-interface.c:915: invalid trust level requested -1) Rolf Eike Beer
2023-04-18 6:48 ` Jeff King
@ 2023-04-18 16:17 ` Junio C Hamano
1 sibling, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2023-04-18 16:17 UTC (permalink / raw
To: Rolf Eike Beer; +Cc: git
Rolf Eike Beer <eb@emlix.com> writes:
> I use this one:
>
> [format]
> pretty = %C(yellow)commit %H%C(auto)%d%Creset%nAuthor: %an <%ae>
> %C(yellow)% GK %GS %C(auto)[%GT% G?]%Creset%nDate: %ad%n%n%w(0,4,4)%s%n%w(0,
> 4,4)%+b
>
> 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:
Thanks for a clearly described report. GPG reports something like
[GNUPG:] NEWSIG
[GNUPG:] ERRSIG B0B5E88696AFE6CB 1 8 00 1681831898 9 E1F036B1FEE7221FC778ECEFB0B5E88696AFE6CB
[GNUPG:] NO_PUBKEY B0B5E88696AFE6CB
but parse_gpg_output() that is responsible for setting the
trust_level member of sigc structure never responds to this report
because none among NEWSIG, ERRSIG, and NO_PUBKEY begins with
"TRUST_" that triggers a call to parse_gpg_trust_level() to set the
member.
The caller of parse_gpg_output() initializes the member to -1 and
that is left intact. Of course, it is not one of the values that
gpg_trust_level_to_str() knows about.
The absolute minimum fix is to initialize the member to TRUST_NEVER
which is one of the values gpg_trust_level_to_str() knows about. It
seems that SSH based signature verification codepath uses the same
approach.
gpg-interface.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git c/gpg-interface.c w/gpg-interface.c
index aceeb08336..2044e00205 100644
--- c/gpg-interface.c
+++ w/gpg-interface.c
@@ -650,7 +650,7 @@ int check_signature(struct signature_check *sigc,
gpg_interface_lazy_init();
sigc->result = 'N';
- sigc->trust_level = -1;
+ sigc->trust_level = TRUST_NEVER;
fmt = get_format_by_sig(signature);
if (!fmt)
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: gpg-related crash with custom formatter (BUG: gpg-interface.c:915: invalid trust level requested -1)
2023-04-18 6:48 ` Jeff King
2023-04-18 15:16 ` Jaydeep Das
@ 2023-04-18 16:24 ` Junio C Hamano
2023-04-19 1:29 ` [PATCH] gpg-interface: set trust level of missing key to "undefined" Jeff King
1 sibling, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2023-04-18 16:24 UTC (permalink / raw
To: Jeff King
Cc: Rolf Eike Beer, git, Jaydeep P Das, Hariom Verma,
Christian Couder
Jeff King <peff@peff.net> writes:
> 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).
Heh, I shouldn't have wasted my cycles in "git log" but in my
newsreader ;-)
Looking at the original before the gpg_trust_level_to_str() function
was introduced, the switch statement looks like it is missing the
usual "default: BUG()" for unhandled enum. My version made it mimic
what ssh side seems to do, but I tend to prefer your empty string
that differentiates between "we never saw any trust level" and "the
system says this key should never be trusted".
Thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] gpg-interface: set trust level of missing key to "undefined"
2023-04-18 16:24 ` Junio C Hamano
@ 2023-04-19 1:29 ` Jeff King
2023-04-19 15:30 ` Junio C Hamano
0 siblings, 1 reply; 9+ messages in thread
From: Jeff King @ 2023-04-19 1:29 UTC (permalink / raw
To: Junio C Hamano
Cc: Rolf Eike Beer, git, Jaydeep P Das, Hariom Verma,
Christian Couder
On Tue, Apr 18, 2023 at 09:24:20AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > 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).
>
> Heh, I shouldn't have wasted my cycles in "git log" but in my
> newsreader ;-)
>
> Looking at the original before the gpg_trust_level_to_str() function
> was introduced, the switch statement looks like it is missing the
> usual "default: BUG()" for unhandled enum. My version made it mimic
> what ssh side seems to do, but I tend to prefer your empty string
> that differentiates between "we never saw any trust level" and "the
> system says this key should never be trusted".
Actually it gets weirder even, as I think we're violating the C standard
a bit here. ;)
Here's the patch that I came up with, though it does not distinguish
between "we did not see any trust level" and "gpg told us the trust
level was undefined". I think that's OK. That level is still below
TRUST_NEVER. But if we really want to distinguish we can introduce a new
value for the enum.
-- >8 --
Subject: gpg-interface: set trust level of missing key to "undefined"
In check_signature(), we initialize the trust_level field to "-1", with
the idea that if gpg does not return a trust level at all (if there is
no signature, or if the signature is made by an unknown key), we'll
use that value. But this has two problems:
1. Since the field is an enum, it's up to the compiler to decide what
underlying storage to use, and it only has to fit the values we've
declared. So we may not be able to store "-1" at all. And indeed,
on my system (linux with gcc), the resulting enum is an unsigned
32-bit value, and -1 becomes 4294967295.
The difference may seem academic (and you even get "-1" if you pass
it to printf("%d")), but it means that code like this:
status |= sigc->trust_level < configured_min_trust_level;
does not necessarily behave as expected. This turns out not to be a
bug in practice, though, because we keep the "-1" only when gpg did
not report a signature from a known key, in which case the line
above:
status |= sigc->result != 'G';
would always set status to non-zero anyway. So only a 'G' signature
with no parsed trust level would cause a problem, which doesn't
seem likely to trigger (outside of unexpected gpg behavior).
2. When using the "%GT" format placeholder, we pass the value to
gpg_trust_level_to_str(), which complains that the value is out of
range with a BUG(). This behavior was introduced by 803978da49
(gpg-interface: add function for converting trust level to string,
2022-07-11). Before that, we just did a switch() on the enum, and
anything that wasn't matched would end up as the empty string.
Curiously, solving this by naively doing:
if (level < 0)
return "";
in that function isn't sufficient. Because of (1) above, the
compiler can (and does in my case) actually remove that conditional
as dead code!
We can solve both by representing this state as an enum value. We could
do this by adding a new "unknown" value. But this really seems to match
the existing "undefined" level well. GPG describes this as "Not enough
information for calculation".
We have tests in t7510 that trigger this case (verifying a signature
from a key that we don't have, and then checking various %G
placeholders), but they didn't notice the BUG() because we didn't look
at %GT for that case! Let's make sure we check all %G placeholders for
each case in the formatting tests.
The interesting ones here are "show unknown signature with custom
format" and "show lack of signature with custom format", both of which
would BUG() before, and now turn %GT into "undefined". Prior to
803978da49 they would have turned it into the empty string, but I think
saying "undefined" consistently is a reasonable outcome, and probably
makes life easier for anyone parsing the output (and any such parser had
to be ready to see "undefined" already).
The other modified tests produce the same output before and after this
patch, but now we're consistently checking both %G? and %GT in all of
them.
Signed-off-by: Jeff King <peff@peff.net>
Reported-by: Rolf Eike Beer <eb@emlix.com>
---
gpg-interface.c | 2 +-
t/t7510-signed-commit.sh | 21 ++++++++++++++-------
2 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index aceeb08336..f3ac5acdd9 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -650,7 +650,7 @@ int check_signature(struct signature_check *sigc,
gpg_interface_lazy_init();
sigc->result = 'N';
- sigc->trust_level = -1;
+ sigc->trust_level = TRUST_UNDEFINED;
fmt = get_format_by_sig(signature);
if (!fmt)
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 48f86cb367..ccbc416402 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -221,84 +221,91 @@ test_expect_success GPG 'amending already signed commit' '
test_expect_success GPG 'show good signature with custom format' '
cat >expect <<-\EOF &&
G
+ ultimate
13B6F51ECDDE430D
C O Mitter <committer@example.com>
73D758744BE721698EC54E8713B6F51ECDDE430D
73D758744BE721698EC54E8713B6F51ECDDE430D
EOF
- git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
+ git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
test_cmp expect actual
'
test_expect_success GPG 'show bad signature with custom format' '
cat >expect <<-\EOF &&
B
+ undefined
13B6F51ECDDE430D
C O Mitter <committer@example.com>
EOF
- git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" $(cat forged1.commit) >actual &&
+ git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" $(cat forged1.commit) >actual &&
test_cmp expect actual
'
test_expect_success GPG 'show untrusted signature with custom format' '
cat >expect <<-\EOF &&
U
+ undefined
65A0EEA02E30CAD7
Eris Discordia <discord@example.net>
F8364A59E07FFE9F4D63005A65A0EEA02E30CAD7
D4BE22311AD3131E5EDA29A461092E85B7227189
EOF
- git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
+ git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
test_cmp expect actual
'
test_expect_success GPG 'show untrusted signature with undefined trust level' '
cat >expect <<-\EOF &&
+ U
undefined
65A0EEA02E30CAD7
Eris Discordia <discord@example.net>
F8364A59E07FFE9F4D63005A65A0EEA02E30CAD7
D4BE22311AD3131E5EDA29A461092E85B7227189
EOF
- git log -1 --format="%GT%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
+ git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
test_cmp expect actual
'
test_expect_success GPG 'show untrusted signature with ultimate trust level' '
cat >expect <<-\EOF &&
+ G
ultimate
13B6F51ECDDE430D
C O Mitter <committer@example.com>
73D758744BE721698EC54E8713B6F51ECDDE430D
73D758744BE721698EC54E8713B6F51ECDDE430D
EOF
- git log -1 --format="%GT%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
+ git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
test_cmp expect actual
'
test_expect_success GPG 'show unknown signature with custom format' '
cat >expect <<-\EOF &&
E
+ undefined
65A0EEA02E30CAD7
EOF
- GNUPGHOME="$GNUPGHOME_NOT_USED" git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
+ GNUPGHOME="$GNUPGHOME_NOT_USED" git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
test_cmp expect actual
'
test_expect_success GPG 'show lack of signature with custom format' '
cat >expect <<-\EOF &&
N
+ undefined
EOF
- git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" seventh-unsigned >actual &&
+ git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" seventh-unsigned >actual &&
test_cmp expect actual
'
--
2.40.0.579.g88b9f0bf51
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] gpg-interface: set trust level of missing key to "undefined"
2023-04-19 1:29 ` [PATCH] gpg-interface: set trust level of missing key to "undefined" Jeff King
@ 2023-04-19 15:30 ` Junio C Hamano
2023-04-22 10:47 ` Jeff King
0 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2023-04-19 15:30 UTC (permalink / raw
To: Jeff King
Cc: Rolf Eike Beer, git, Jaydeep P Das, Hariom Verma,
Christian Couder
Jeff King <peff@peff.net> writes:
> Here's the patch that I came up with, though it does not distinguish
> between "we did not see any trust level" and "gpg told us the trust
> level was undefined". I think that's OK. That level is still below
> TRUST_NEVER. But if we really want to distinguish we can introduce a new
> value for the enum.
Good.
In my zeroth draft, I added to the enum a new TRUST_FAILED = -1 to
be used for the initialization assignment and get stringified in the
gpg_trust_level_to_str() function, which gave us the distinction and
made sure the enum is signed. But in the end, I decided it was not
worth risking upsetting the end-user scripts that assumed the
current set of levels with a new "level" that is not known to them.
Initializing to undefined like this patch is with much less damage
to the codebase, and existing end-user scripts are probably prepared
to react to "undefined" already and treat it as even less trustworthy
than the "never" ones.
Will queue. Thanks.
> -- >8 --
> Subject: gpg-interface: set trust level of missing key to "undefined"
>
> In check_signature(), we initialize the trust_level field to "-1", with
> the idea that if gpg does not return a trust level at all (if there is
> no signature, or if the signature is made by an unknown key), we'll
> use that value. But this has two problems:
>
> 1. Since the field is an enum, it's up to the compiler to decide what
> underlying storage to use, and it only has to fit the values we've
> declared. So we may not be able to store "-1" at all. And indeed,
> on my system (linux with gcc), the resulting enum is an unsigned
> 32-bit value, and -1 becomes 4294967295.
>
> The difference may seem academic (and you even get "-1" if you pass
> it to printf("%d")), but it means that code like this:
>
> status |= sigc->trust_level < configured_min_trust_level;
>
> does not necessarily behave as expected. This turns out not to be a
> bug in practice, though, because we keep the "-1" only when gpg did
> not report a signature from a known key, in which case the line
> above:
>
> status |= sigc->result != 'G';
>
> would always set status to non-zero anyway. So only a 'G' signature
> with no parsed trust level would cause a problem, which doesn't
> seem likely to trigger (outside of unexpected gpg behavior).
>
> 2. When using the "%GT" format placeholder, we pass the value to
> gpg_trust_level_to_str(), which complains that the value is out of
> range with a BUG(). This behavior was introduced by 803978da49
> (gpg-interface: add function for converting trust level to string,
> 2022-07-11). Before that, we just did a switch() on the enum, and
> anything that wasn't matched would end up as the empty string.
>
> Curiously, solving this by naively doing:
>
> if (level < 0)
> return "";
>
> in that function isn't sufficient. Because of (1) above, the
> compiler can (and does in my case) actually remove that conditional
> as dead code!
>
> We can solve both by representing this state as an enum value. We could
> do this by adding a new "unknown" value. But this really seems to match
> the existing "undefined" level well. GPG describes this as "Not enough
> information for calculation".
>
> We have tests in t7510 that trigger this case (verifying a signature
> from a key that we don't have, and then checking various %G
> placeholders), but they didn't notice the BUG() because we didn't look
> at %GT for that case! Let's make sure we check all %G placeholders for
> each case in the formatting tests.
>
> The interesting ones here are "show unknown signature with custom
> format" and "show lack of signature with custom format", both of which
> would BUG() before, and now turn %GT into "undefined". Prior to
> 803978da49 they would have turned it into the empty string, but I think
> saying "undefined" consistently is a reasonable outcome, and probably
> makes life easier for anyone parsing the output (and any such parser had
> to be ready to see "undefined" already).
>
> The other modified tests produce the same output before and after this
> patch, but now we're consistently checking both %G? and %GT in all of
> them.
>
> Signed-off-by: Jeff King <peff@peff.net>
> Reported-by: Rolf Eike Beer <eb@emlix.com>
> ---
> gpg-interface.c | 2 +-
> t/t7510-signed-commit.sh | 21 ++++++++++++++-------
> 2 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index aceeb08336..f3ac5acdd9 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -650,7 +650,7 @@ int check_signature(struct signature_check *sigc,
> gpg_interface_lazy_init();
>
> sigc->result = 'N';
> - sigc->trust_level = -1;
> + sigc->trust_level = TRUST_UNDEFINED;
>
> fmt = get_format_by_sig(signature);
> if (!fmt)
> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> index 48f86cb367..ccbc416402 100755
> --- a/t/t7510-signed-commit.sh
> +++ b/t/t7510-signed-commit.sh
> @@ -221,84 +221,91 @@ test_expect_success GPG 'amending already signed commit' '
> test_expect_success GPG 'show good signature with custom format' '
> cat >expect <<-\EOF &&
> G
> + ultimate
> 13B6F51ECDDE430D
> C O Mitter <committer@example.com>
> 73D758744BE721698EC54E8713B6F51ECDDE430D
> 73D758744BE721698EC54E8713B6F51ECDDE430D
> EOF
> - git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
> + git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
> test_cmp expect actual
> '
>
> test_expect_success GPG 'show bad signature with custom format' '
> cat >expect <<-\EOF &&
> B
> + undefined
> 13B6F51ECDDE430D
> C O Mitter <committer@example.com>
>
>
> EOF
> - git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" $(cat forged1.commit) >actual &&
> + git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" $(cat forged1.commit) >actual &&
> test_cmp expect actual
> '
>
> test_expect_success GPG 'show untrusted signature with custom format' '
> cat >expect <<-\EOF &&
> U
> + undefined
> 65A0EEA02E30CAD7
> Eris Discordia <discord@example.net>
> F8364A59E07FFE9F4D63005A65A0EEA02E30CAD7
> D4BE22311AD3131E5EDA29A461092E85B7227189
> EOF
> - git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
> + git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
> test_cmp expect actual
> '
>
> test_expect_success GPG 'show untrusted signature with undefined trust level' '
> cat >expect <<-\EOF &&
> + U
> undefined
> 65A0EEA02E30CAD7
> Eris Discordia <discord@example.net>
> F8364A59E07FFE9F4D63005A65A0EEA02E30CAD7
> D4BE22311AD3131E5EDA29A461092E85B7227189
> EOF
> - git log -1 --format="%GT%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
> + git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
> test_cmp expect actual
> '
>
> test_expect_success GPG 'show untrusted signature with ultimate trust level' '
> cat >expect <<-\EOF &&
> + G
> ultimate
> 13B6F51ECDDE430D
> C O Mitter <committer@example.com>
> 73D758744BE721698EC54E8713B6F51ECDDE430D
> 73D758744BE721698EC54E8713B6F51ECDDE430D
> EOF
> - git log -1 --format="%GT%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
> + git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
> test_cmp expect actual
> '
>
> test_expect_success GPG 'show unknown signature with custom format' '
> cat >expect <<-\EOF &&
> E
> + undefined
> 65A0EEA02E30CAD7
>
>
>
> EOF
> - GNUPGHOME="$GNUPGHOME_NOT_USED" git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
> + GNUPGHOME="$GNUPGHOME_NOT_USED" git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
> test_cmp expect actual
> '
>
> test_expect_success GPG 'show lack of signature with custom format' '
> cat >expect <<-\EOF &&
> N
> + undefined
>
>
>
>
> EOF
> - git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" seventh-unsigned >actual &&
> + git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" seventh-unsigned >actual &&
> test_cmp expect actual
> '
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gpg-interface: set trust level of missing key to "undefined"
2023-04-19 15:30 ` Junio C Hamano
@ 2023-04-22 10:47 ` Jeff King
2023-04-24 16:22 ` Junio C Hamano
0 siblings, 1 reply; 9+ messages in thread
From: Jeff King @ 2023-04-22 10:47 UTC (permalink / raw
To: Junio C Hamano
Cc: Rolf Eike Beer, git, Jaydeep P Das, Hariom Verma,
Christian Couder
On Wed, Apr 19, 2023 at 08:30:35AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Here's the patch that I came up with, though it does not distinguish
> > between "we did not see any trust level" and "gpg told us the trust
> > level was undefined". I think that's OK. That level is still below
> > TRUST_NEVER. But if we really want to distinguish we can introduce a new
> > value for the enum.
>
> Good.
>
> In my zeroth draft, I added to the enum a new TRUST_FAILED = -1 to
> be used for the initialization assignment and get stringified in the
> gpg_trust_level_to_str() function, which gave us the distinction and
> made sure the enum is signed. But in the end, I decided it was not
> worth risking upsetting the end-user scripts that assumed the
> current set of levels with a new "level" that is not known to them.
>
> Initializing to undefined like this patch is with much less damage
> to the codebase, and existing end-user scripts are probably prepared
> to react to "undefined" already and treat it as even less trustworthy
> than the "never" ones.
One thing that I wondered about for using UNDEFINED is that we do this:
static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED;
which is then later compared with:
status |= sigc->result != 'G';
status |= sigc->trust_level < configured_min_trust_level;
So before my patch the uninitialized state is (supposedly) less than the
min level, and after they are the same. For the reasons I gave in the
commit message, I think that less-than comparison was already broken.
And likewise, for the reasons I gave, it hopefully never matters since
the result would never be 'G' in that case.
So I think it's fine, but I definitely had to stare at it for a while.
This all comes from 54887b4689 (gpg-interface: add minTrustLevel as a
configuration option, 2019-12-27), which does discuss some of the
implications, but I think my patch is in line with the logic there.
-Peff
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gpg-interface: set trust level of missing key to "undefined"
2023-04-22 10:47 ` Jeff King
@ 2023-04-24 16:22 ` Junio C Hamano
0 siblings, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2023-04-24 16:22 UTC (permalink / raw
To: Jeff King
Cc: Rolf Eike Beer, git, Jaydeep P Das, Hariom Verma,
Christian Couder
Jeff King <peff@peff.net> writes:
> So before my patch the uninitialized state is (supposedly) less than the
> min level, and after they are the same. For the reasons I gave in the
> commit message, I think that less-than comparison was already broken.
> And likewise, for the reasons I gave, it hopefully never matters since
> the result would never be 'G' in that case.
Yes * 2.
> So I think it's fine, but I definitely had to stare at it for a while.
> This all comes from 54887b4689 (gpg-interface: add minTrustLevel as a
> configuration option, 2019-12-27), which does discuss some of the
> implications, but I think my patch is in line with the logic there.
Yes.
Thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-04-24 16:22 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-18 6:12 gpg-related crash with custom formatter (BUG: gpg-interface.c:915: invalid trust level requested -1) Rolf Eike Beer
2023-04-18 6:48 ` Jeff King
2023-04-18 15:16 ` Jaydeep Das
2023-04-18 16:24 ` Junio C Hamano
2023-04-19 1:29 ` [PATCH] gpg-interface: set trust level of missing key to "undefined" Jeff King
2023-04-19 15:30 ` Junio C Hamano
2023-04-22 10:47 ` Jeff King
2023-04-24 16:22 ` Junio C Hamano
2023-04-18 16:17 ` gpg-related crash with custom formatter (BUG: gpg-interface.c:915: invalid trust level requested -1) Junio C Hamano
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).