{"thread":{"id":"59614","subject":"gpg-related crash with custom formatter (BUG: gpg-interface.c:915: invalid trust level requested -1)","startedAt":"2023-04-18T06:12:15Z","lastAt":"2023-04-24T16:22:43Z","messageCount":9,"participants":["Rolf Eike Beer","Jeff King","Jaydeep Das","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"475589","messageId":"5926995.lOV4Wx5bFT@devpool47.emlix.com","threadId":"59614","inReplyTo":null,"subject":"gpg-related crash with custom formatter (BUG: gpg-interface.c:915: invalid trust level requested -1)","fromName":"Rolf Eike Beer","fromEmail":"eb@emlix.com","sentAt":"2023-04-18T06:12:03Z","receivedAt":"2023-04-18T06:12:15Z","isPatch":false,"sender":{"key":"eb@emlix.com","avatar":null},"body":"I use this one:\n\n[format]\n        pretty = %C(yellow)commit %H%C(auto)%d%Creset%nAuthor: %an <%ae> \n%C(yellow)% GK %GS %C(auto)[%GT% G?]%Creset%nDate:   %ad%n%n%w(0,4,4)%s%n%w(0,\n4,4)%+b\n\nWhen I now run \"git log\" in a repository that contains commits signed by \npeople not in my keyring (e.g. the Gentoo git) I get this backtrace:\n\nBUG: gpg-interface.c:915: invalid trust level requested -1\n\nProgram received signal SIGABRT, Aborted.\n0x00007ffff7d69d7c in __pthread_kill_implementation () from /lib64/libc.so.6\nMissing separate debuginfos, use: zypper install libpcre2-8-0-\ndebuginfo-10.42-3.6.x86_64 libz1-x86-64-v3-debuginfo-1.2.13-3.4.x86_64\n(gdb) bt\n#0  0x00007ffff7d69d7c in __pthread_kill_implementation () from /lib64/libc.so.6\n#1  0x00007ffff7d18356 in raise () from /lib64/libc.so.6\n#2  0x00007ffff7d00897 in abort () from /lib64/libc.so.6\n#3  0x00005555558041fe in BUG_vfl (params=0x7fffffffb9e0, fmt=0x555555895608 \n\"invalid trust level requested %d\", line=<optimized out>, file=<optimized out>) \nat /usr/src/debug/git-2.40.0/usage.c:313\n#4  BUG_fl (file=<optimized out>, line=<optimized out>, fmt=0x555555895608 \n\"invalid trust level requested %d\") at /usr/src/debug/git-2.40.0/usage.c:330\n#5  0x000055555576a816 in gpg_trust_level_to_str (level=<optimized out>) at /\nusr/src/debug/git-2.40.0/gpg-interface.c:915\n#6  format_commit_one (sb=0x7fffffffbf40, placeholder=0x555555936b65 \"GT% G?]\n%Creset%nDate:   %ad%n%n%w(0,4,4)%s%n%w(0,4,4)%+b\", context=0x7fffffffbde0) at /\nusr/src/debug/git-2.40.0/pretty.c:1617\n#7  0x000055555576ab1e in format_commit_item (sb=0x7fffffffbf40, \nplaceholder=0x555555936b65 \"GT% G?]%Creset%nDate:   %ad%n%n%w(0,4,4)%s%n%w(0,\n4,4)%+b\", context=0x7fffffffbde0) at /usr/src/debug/git-2.40.0/pretty.c:1844\n#8  0x00005555557cfc44 in strbuf_expand (sb=0x7fffffffbf40, format=0x555555936b65 \n\"GT% G?]%Creset%nDate:   %ad%n%n%w(0,4,4)%s%n%w(0,4,4)%+b\", fn=0x55555576a8d0 \n<format_commit_item>, context=0x7fffffffbde0) at /usr/src/debug/git-2.40.0/\nstrbuf.c:429\n#9  0x000055555576af0e in repo_format_commit_message (r=0x555555928540 \n<the_repo>, commit=0x55555594e2d0, format=<optimized out>, sb=0x7fffffffbf40, \npretty_ctx=<optimized out>) at /usr/src/debug/git-2.40.0/pretty.c:1910\n#10 0x000055555571ad10 in show_log (opt=opt@entry=0x7fffffffc570) at /usr/src/\ndebug/git-2.40.0/log-tree.c:781\n#11 0x000055555571bd17 in log_tree_commit (opt=0x7fffffffc570, commit=<optimized \nout>) at /usr/src/debug/git-2.40.0/log-tree.c:1117\n#12 0x00005555555e9638 in cmd_log_walk_no_free (rev=0x7fffffffc570) at builtin/\nlog.c:508\n#13 0x00005555555e9d5a in cmd_log_walk (rev=0x7fffffffc570) at builtin/log.c:549\n#14 cmd_log (argc=1, argv=0x7fffffffd7c0, prefix=0x0) at builtin/log.c:883\n#15 0x00005555555782bb in run_builtin (argv=0x7fffffffd7c0, argc=1, \np=0x5555558fa000 <commands.lto_priv+1440>) at /usr/src/debug/git-2.40.0/git.c:\n445\n#16 handle_builtin (argc=1, argv=0x7fffffffd7c0) at /usr/src/debug/git-2.40.0/\ngit.c:699\n#17 0x00005555555787e7 in run_argv (argcp=0x7fffffffd50c, argv=0x7fffffffd530) at /\nusr/src/debug/git-2.40.0/git.c:763\n#18 0x000055555557464b in cmd_main (argv=<optimized out>, argc=<optimized \nout>) at /usr/src/debug/git-2.40.0/git.c:898\n#19 main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/\ngit-2.40.0/common-main.c:57\n\nThis is not absolutely new, it is broken for a while but I was too lazy to \nreport (sorry about that). It has worked in the past, i.e. when I created that \nformatter (IIRC at least 2 years ago).\n\nSystem is an openSUSE Tumbleweed installation on amd64.\n\nGreetings,\n\nEike\n-- \nRolf Eike Beer, emlix GmbH, http://www.emlix.com\nFon +49 551 30664-0, Fax +49 551 30664-11\nGothaer Platz 3, 37083 Göttingen, Germany\nSitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160\nGeschäftsführung: Heike Jordan, Dr. Uwe Kracke – Ust-IdNr.: DE 205 198 055\n\nemlix - smart embedded open source\n"},{"id":"475591","messageId":"20230418064846.GA1414@coredump.intra.peff.net","threadId":"59614","inReplyTo":"5926995.lOV4Wx5bFT@devpool47.emlix.com","subject":"Re: gpg-related crash with custom formatter (BUG: gpg-interface.c:915: invalid trust level requested -1)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-18T06:48:46Z","receivedAt":"2023-04-18T06:49:17Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 18, 2023 at 08:12:03AM +0200, Rolf Eike Beer wrote:\n\n> When I now run \"git log\" in a repository that contains commits signed by \n> people not in my keyring (e.g. the Gentoo git) I get this backtrace:\n> \n> BUG: gpg-interface.c:915: invalid trust level requested -1\n\nThanks for giving an example repo. After cloning:\n \n  https://anongit.gentoo.org/git/repo/gentoo.git\n\nI can reproduce just by running \"git log -1 --format=%GT\". Bisecting\nturns up 803978da49 (gpg-interface: add function for converting trust\nlevel to string, 2022-07-11), which is not too surprising.\n\nBefore that we returned an empty string. I don't know if the fix is a\nsimple as:\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex aceeb08336..edb0da1bda 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -934,7 +934,10 @@ const char *gpg_trust_level_to_str(enum signature_trust_level level)\n {\n \tstruct sigcheck_gpg_trust_level *trust;\n \n-\tif (level < 0 || level >= ARRAY_SIZE(sigcheck_gpg_trust_level))\n+\tif (level < 0)\n+\t\treturn \"\";\n+\n+\tif (level >= ARRAY_SIZE(sigcheck_gpg_trust_level))\n \t\tBUG(\"invalid trust level requested %d\", level);\n \n \ttrust = &sigcheck_gpg_trust_level[level];\n\nwhich restores the original behavior, or if the original was papering\nover another bug (e.g., should this be \"undefined\"?). Certainly the\nempty string matches other placeholders like %GS for this case (since we\nobviously don't know anything about the signer).\n\n+cc folks who worked on 803978da49.\n\n-Peff\n"},{"id":"475626","messageId":"CACaPSotwDfDXk=kR7LntF46NN-cM-zx22T83m1sjYEDLbnSxNQ@mail.gmail.com","threadId":"59614","inReplyTo":"20230418064846.GA1414@coredump.intra.peff.net","subject":"Re: gpg-related crash with custom formatter (BUG: gpg-interface.c:915: invalid trust level requested -1)","fromName":"Jaydeep Das","fromEmail":"jaydeepjd.8914@gmail.com","sentAt":"2023-04-18T15:16:16Z","receivedAt":"2023-04-18T15:16:30Z","isPatch":false,"sender":{"key":"jaydeepjd.8914@gmail.com","avatar":"https://avatars.githubusercontent.com/u/64089730?v=4"},"body":"Thanks for the report.\n\nThe patch did not preserve the exact behaviour of\nthe previous code. Rather than calling BUG() whenever a trust level is out\nof the sigcheck_gpg_trust_level[] array, we can simply return an empty string\n\nif (level < 0 || level >= ARRAY_SIZE(sigcheck_gpg_trust_level))\n        return \"\";\n\nIt will replicate the exact behaviour as the previous code. But as\nJeff pointed out,\nShould this really be the defined behavior?\n\nLet me know what you think. I will make the necessary changes.\n\nThanks,\nJaydeep.\n\n\nOn Tue, Apr 18, 2023 at 12:18 PM Jeff King <peff@peff.net> wrote:\n>\n> On Tue, Apr 18, 2023 at 08:12:03AM +0200, Rolf Eike Beer wrote:\n>\n> > When I now run \"git log\" in a repository that contains commits signed by\n> > people not in my keyring (e.g. the Gentoo git) I get this backtrace:\n> >\n> > BUG: gpg-interface.c:915: invalid trust level requested -1\n>\n> Thanks for giving an example repo. After cloning:\n>\n>   https://anongit.gentoo.org/git/repo/gentoo.git\n>\n> I can reproduce just by running \"git log -1 --format=%GT\". Bisecting\n> turns up 803978da49 (gpg-interface: add function for converting trust\n> level to string, 2022-07-11), which is not too surprising.\n>\n> Before that we returned an empty string. I don't know if the fix is a\n> simple as:\n>\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index aceeb08336..edb0da1bda 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -934,7 +934,10 @@ const char *gpg_trust_level_to_str(enum signature_trust_level level)\n>  {\n>         struct sigcheck_gpg_trust_level *trust;\n>\n> -       if (level < 0 || level >= ARRAY_SIZE(sigcheck_gpg_trust_level))\n> +       if (level < 0)\n> +               return \"\";\n> +\n> +       if (level >= ARRAY_SIZE(sigcheck_gpg_trust_level))\n>                 BUG(\"invalid trust level requested %d\", level);\n>\n>         trust = &sigcheck_gpg_trust_level[level];\n>\n> which restores the original behavior, or if the original was papering\n> over another bug (e.g., should this be \"undefined\"?). Certainly the\n> empty string matches other placeholders like %GS for this case (since we\n> obviously don't know anything about the signer).\n>\n> +cc folks who worked on 803978da49.\n>\n> -Peff\n"},{"id":"475629","messageId":"xmqq354xf9m6.fsf@gitster.g","threadId":"59614","inReplyTo":"5926995.lOV4Wx5bFT@devpool47.emlix.com","subject":"Re: gpg-related crash with custom formatter (BUG: gpg-interface.c:915: invalid trust level requested -1)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-18T16:17:21Z","receivedAt":"2023-04-18T16:17:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rolf Eike Beer <eb@emlix.com> writes:\n\n> I use this one:\n>\n> [format]\n>         pretty = %C(yellow)commit %H%C(auto)%d%Creset%nAuthor: %an <%ae> \n> %C(yellow)% GK %GS %C(auto)[%GT% G?]%Creset%nDate:   %ad%n%n%w(0,4,4)%s%n%w(0,\n> 4,4)%+b\n>\n> When I now run \"git log\" in a repository that contains commits signed by \n> people not in my keyring (e.g. the Gentoo git) I get this backtrace:\n\nThanks for a clearly described report.  GPG reports something like\n\n    [GNUPG:] NEWSIG\n    [GNUPG:] ERRSIG B0B5E88696AFE6CB 1 8 00 1681831898 9 E1F036B1FEE7221FC778ECEFB0B5E88696AFE6CB\n    [GNUPG:] NO_PUBKEY B0B5E88696AFE6CB\n\nbut parse_gpg_output() that is responsible for setting the\ntrust_level member of sigc structure never responds to this report\nbecause none among NEWSIG, ERRSIG, and NO_PUBKEY begins with\n\"TRUST_\" that triggers a call to parse_gpg_trust_level() to set the\nmember.\n\nThe caller of parse_gpg_output() initializes the member to -1 and\nthat is left intact.  Of course, it is not one of the values that\ngpg_trust_level_to_str() knows about.\n\nThe absolute minimum fix is to initialize the member to TRUST_NEVER\nwhich is one of the values gpg_trust_level_to_str() knows about.  It\nseems that SSH based signature verification codepath uses the same\napproach.\n\n\n gpg-interface.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git c/gpg-interface.c w/gpg-interface.c\nindex aceeb08336..2044e00205 100644\n--- c/gpg-interface.c\n+++ w/gpg-interface.c\n@@ -650,7 +650,7 @@ int check_signature(struct signature_check *sigc,\n \tgpg_interface_lazy_init();\n \n \tsigc->result = 'N';\n-\tsigc->trust_level = -1;\n+\tsigc->trust_level = TRUST_NEVER;\n \n \tfmt = get_format_by_sig(signature);\n \tif (!fmt)\n"},{"id":"475630","messageId":"xmqqy1mpduq3.fsf@gitster.g","threadId":"59614","inReplyTo":"20230418064846.GA1414@coredump.intra.peff.net","subject":"Re: gpg-related crash with custom formatter (BUG: gpg-interface.c:915: invalid trust level requested -1)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-18T16:24:20Z","receivedAt":"2023-04-18T16:24:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> which restores the original behavior, or if the original was papering\n> over another bug (e.g., should this be \"undefined\"?). Certainly the\n> empty string matches other placeholders like %GS for this case (since we\n> obviously don't know anything about the signer).\n\nHeh, I shouldn't have wasted my cycles in \"git log\" but in my\nnewsreader ;-)\n\nLooking at the original before the gpg_trust_level_to_str() function\nwas introduced, the switch statement looks like it is missing the\nusual \"default: BUG()\" for unhandled enum.  My version made it mimic\nwhat ssh side seems to do, but I tend to prefer your empty string\nthat differentiates between \"we never saw any trust level\" and \"the\nsystem says this key should never be trusted\".\n\nThanks.\n"},{"id":"475677","messageId":"20230419012957.GA503941@coredump.intra.peff.net","threadId":"59614","inReplyTo":"xmqqy1mpduq3.fsf@gitster.g","subject":"[PATCH] gpg-interface: set trust level of missing key to \"undefined\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-19T01:29:57Z","receivedAt":"2023-04-19T01:30:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 18, 2023 at 09:24:20AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > which restores the original behavior, or if the original was papering\n> > over another bug (e.g., should this be \"undefined\"?). Certainly the\n> > empty string matches other placeholders like %GS for this case (since we\n> > obviously don't know anything about the signer).\n> \n> Heh, I shouldn't have wasted my cycles in \"git log\" but in my\n> newsreader ;-)\n> \n> Looking at the original before the gpg_trust_level_to_str() function\n> was introduced, the switch statement looks like it is missing the\n> usual \"default: BUG()\" for unhandled enum.  My version made it mimic\n> what ssh side seems to do, but I tend to prefer your empty string\n> that differentiates between \"we never saw any trust level\" and \"the\n> system says this key should never be trusted\".\n\nActually it gets weirder even, as I think we're violating the C standard\na bit here. ;)\n\nHere's the patch that I came up with, though it does not distinguish\nbetween \"we did not see any trust level\" and \"gpg told us the trust\nlevel was undefined\". I think that's OK. That level is still below\nTRUST_NEVER. But if we really want to distinguish we can introduce a new\nvalue for the enum.\n\n-- >8 --\nSubject: gpg-interface: set trust level of missing key to \"undefined\"\n\nIn check_signature(), we initialize the trust_level field to \"-1\", with\nthe idea that if gpg does not return a trust level at all (if there is\nno signature, or if the signature is made by an unknown key), we'll\nuse that value. But this has two problems:\n\n  1. Since the field is an enum, it's up to the compiler to decide what\n     underlying storage to use, and it only has to fit the values we've\n     declared. So we may not be able to store \"-1\" at all. And indeed,\n     on my system (linux with gcc), the resulting enum is an unsigned\n     32-bit value, and -1 becomes 4294967295.\n\n     The difference may seem academic (and you even get \"-1\" if you pass\n     it to printf(\"%d\")), but it means that code like this:\n\n       status |= sigc->trust_level < configured_min_trust_level;\n\n     does not necessarily behave as expected. This turns out not to be a\n     bug in practice, though, because we keep the \"-1\" only when gpg did\n     not report a signature from a known key, in which case the line\n     above:\n\n       status |= sigc->result != 'G';\n\n     would always set status to non-zero anyway. So only a 'G' signature\n     with no parsed trust level would cause a problem, which doesn't\n     seem likely to trigger (outside of unexpected gpg behavior).\n\n  2. When using the \"%GT\" format placeholder, we pass the value to\n     gpg_trust_level_to_str(), which complains that the value is out of\n     range with a BUG(). This behavior was introduced by 803978da49\n     (gpg-interface: add function for converting trust level to string,\n     2022-07-11). Before that, we just did a switch() on the enum, and\n     anything that wasn't matched would end up as the empty string.\n\n     Curiously, solving this by naively doing:\n\n       if (level < 0)\n               return \"\";\n\n     in that function isn't sufficient. Because of (1) above, the\n     compiler can (and does in my case) actually remove that conditional\n     as dead code!\n\nWe can solve both by representing this state as an enum value. We could\ndo this by adding a new \"unknown\" value. But this really seems to match\nthe existing \"undefined\" level well. GPG describes this as \"Not enough\ninformation for calculation\".\n\nWe have tests in t7510 that trigger this case (verifying a signature\nfrom a key that we don't have, and then checking various %G\nplaceholders), but they didn't notice the BUG() because we didn't look\nat %GT for that case! Let's make sure we check all %G placeholders for\neach case in the formatting tests.\n\nThe interesting ones here are \"show unknown signature with custom\nformat\" and \"show lack of signature with custom format\", both of which\nwould BUG() before, and now turn %GT into \"undefined\". Prior to\n803978da49 they would have turned it into the empty string, but I think\nsaying \"undefined\" consistently is a reasonable outcome, and probably\nmakes life easier for anyone parsing the output (and any such parser had\nto be ready to see \"undefined\" already).\n\nThe other modified tests produce the same output before and after this\npatch, but now we're consistently checking both %G? and %GT in all of\nthem.\n\nSigned-off-by: Jeff King <peff@peff.net>\nReported-by: Rolf Eike Beer <eb@emlix.com>\n---\n gpg-interface.c          |  2 +-\n t/t7510-signed-commit.sh | 21 ++++++++++++++-------\n 2 files changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex aceeb08336..f3ac5acdd9 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -650,7 +650,7 @@ int check_signature(struct signature_check *sigc,\n \tgpg_interface_lazy_init();\n \n \tsigc->result = 'N';\n-\tsigc->trust_level = -1;\n+\tsigc->trust_level = TRUST_UNDEFINED;\n \n \tfmt = get_format_by_sig(signature);\n \tif (!fmt)\ndiff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\nindex 48f86cb367..ccbc416402 100755\n--- a/t/t7510-signed-commit.sh\n+++ b/t/t7510-signed-commit.sh\n@@ -221,84 +221,91 @@ test_expect_success GPG 'amending already signed commit' '\n test_expect_success GPG 'show good signature with custom format' '\n \tcat >expect <<-\\EOF &&\n \tG\n+\tultimate\n \t13B6F51ECDDE430D\n \tC O Mitter <committer@example.com>\n \t73D758744BE721698EC54E8713B6F51ECDDE430D\n \t73D758744BE721698EC54E8713B6F51ECDDE430D\n \tEOF\n-\tgit log -1 --format=\"%G?%n%GK%n%GS%n%GF%n%GP\" sixth-signed >actual &&\n+\tgit log -1 --format=\"%G?%n%GT%n%GK%n%GS%n%GF%n%GP\" sixth-signed >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success GPG 'show bad signature with custom format' '\n \tcat >expect <<-\\EOF &&\n \tB\n+\tundefined\n \t13B6F51ECDDE430D\n \tC O Mitter <committer@example.com>\n \n \n \tEOF\n-\tgit log -1 --format=\"%G?%n%GK%n%GS%n%GF%n%GP\" $(cat forged1.commit) >actual &&\n+\tgit log -1 --format=\"%G?%n%GT%n%GK%n%GS%n%GF%n%GP\" $(cat forged1.commit) >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success GPG 'show untrusted signature with custom format' '\n \tcat >expect <<-\\EOF &&\n \tU\n+\tundefined\n \t65A0EEA02E30CAD7\n \tEris Discordia <discord@example.net>\n \tF8364A59E07FFE9F4D63005A65A0EEA02E30CAD7\n \tD4BE22311AD3131E5EDA29A461092E85B7227189\n \tEOF\n-\tgit log -1 --format=\"%G?%n%GK%n%GS%n%GF%n%GP\" eighth-signed-alt >actual &&\n+\tgit log -1 --format=\"%G?%n%GT%n%GK%n%GS%n%GF%n%GP\" eighth-signed-alt >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success GPG 'show untrusted signature with undefined trust level' '\n \tcat >expect <<-\\EOF &&\n+\tU\n \tundefined\n \t65A0EEA02E30CAD7\n \tEris Discordia <discord@example.net>\n \tF8364A59E07FFE9F4D63005A65A0EEA02E30CAD7\n \tD4BE22311AD3131E5EDA29A461092E85B7227189\n \tEOF\n-\tgit log -1 --format=\"%GT%n%GK%n%GS%n%GF%n%GP\" eighth-signed-alt >actual &&\n+\tgit log -1 --format=\"%G?%n%GT%n%GK%n%GS%n%GF%n%GP\" eighth-signed-alt >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success GPG 'show untrusted signature with ultimate trust level' '\n \tcat >expect <<-\\EOF &&\n+\tG\n \tultimate\n \t13B6F51ECDDE430D\n \tC O Mitter <committer@example.com>\n \t73D758744BE721698EC54E8713B6F51ECDDE430D\n \t73D758744BE721698EC54E8713B6F51ECDDE430D\n \tEOF\n-\tgit log -1 --format=\"%GT%n%GK%n%GS%n%GF%n%GP\" sixth-signed >actual &&\n+\tgit log -1 --format=\"%G?%n%GT%n%GK%n%GS%n%GF%n%GP\" sixth-signed >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success GPG 'show unknown signature with custom format' '\n \tcat >expect <<-\\EOF &&\n \tE\n+\tundefined\n \t65A0EEA02E30CAD7\n \n \n \n \tEOF\n-\tGNUPGHOME=\"$GNUPGHOME_NOT_USED\" git log -1 --format=\"%G?%n%GK%n%GS%n%GF%n%GP\" eighth-signed-alt >actual &&\n+\tGNUPGHOME=\"$GNUPGHOME_NOT_USED\" git log -1 --format=\"%G?%n%GT%n%GK%n%GS%n%GF%n%GP\" eighth-signed-alt >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success GPG 'show lack of signature with custom format' '\n \tcat >expect <<-\\EOF &&\n \tN\n+\tundefined\n \n \n \n \n \tEOF\n-\tgit log -1 --format=\"%G?%n%GK%n%GS%n%GF%n%GP\" seventh-unsigned >actual &&\n+\tgit log -1 --format=\"%G?%n%GT%n%GK%n%GS%n%GF%n%GP\" seventh-unsigned >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.40.0.579.g88b9f0bf51\n\n"},{"id":"475699","messageId":"xmqqy1mnanz8.fsf@gitster.g","threadId":"59614","inReplyTo":"20230419012957.GA503941@coredump.intra.peff.net","subject":"Re: [PATCH] gpg-interface: set trust level of missing key to \"undefined\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-19T15:30:35Z","receivedAt":"2023-04-19T15:30:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Here's the patch that I came up with, though it does not distinguish\n> between \"we did not see any trust level\" and \"gpg told us the trust\n> level was undefined\". I think that's OK. That level is still below\n> TRUST_NEVER. But if we really want to distinguish we can introduce a new\n> value for the enum.\n\nGood.\n\nIn my zeroth draft, I added to the enum a new TRUST_FAILED = -1 to\nbe used for the initialization assignment and get stringified in the\ngpg_trust_level_to_str() function, which gave us the distinction and\nmade sure the enum is signed.  But in the end, I decided it was not\nworth risking upsetting the end-user scripts that assumed the\ncurrent set of levels with a new \"level\" that is not known to them.\n\nInitializing to undefined like this patch is with much less damage\nto the codebase, and existing end-user scripts are probably prepared\nto react to \"undefined\" already and treat it as even less trustworthy\nthan the \"never\" ones.\n\nWill queue.  Thanks.\n\n> -- >8 --\n> Subject: gpg-interface: set trust level of missing key to \"undefined\"\n>\n> In check_signature(), we initialize the trust_level field to \"-1\", with\n> the idea that if gpg does not return a trust level at all (if there is\n> no signature, or if the signature is made by an unknown key), we'll\n> use that value. But this has two problems:\n>\n>   1. Since the field is an enum, it's up to the compiler to decide what\n>      underlying storage to use, and it only has to fit the values we've\n>      declared. So we may not be able to store \"-1\" at all. And indeed,\n>      on my system (linux with gcc), the resulting enum is an unsigned\n>      32-bit value, and -1 becomes 4294967295.\n>\n>      The difference may seem academic (and you even get \"-1\" if you pass\n>      it to printf(\"%d\")), but it means that code like this:\n>\n>        status |= sigc->trust_level < configured_min_trust_level;\n>\n>      does not necessarily behave as expected. This turns out not to be a\n>      bug in practice, though, because we keep the \"-1\" only when gpg did\n>      not report a signature from a known key, in which case the line\n>      above:\n>\n>        status |= sigc->result != 'G';\n>\n>      would always set status to non-zero anyway. So only a 'G' signature\n>      with no parsed trust level would cause a problem, which doesn't\n>      seem likely to trigger (outside of unexpected gpg behavior).\n>\n>   2. When using the \"%GT\" format placeholder, we pass the value to\n>      gpg_trust_level_to_str(), which complains that the value is out of\n>      range with a BUG(). This behavior was introduced by 803978da49\n>      (gpg-interface: add function for converting trust level to string,\n>      2022-07-11). Before that, we just did a switch() on the enum, and\n>      anything that wasn't matched would end up as the empty string.\n>\n>      Curiously, solving this by naively doing:\n>\n>        if (level < 0)\n>                return \"\";\n>\n>      in that function isn't sufficient. Because of (1) above, the\n>      compiler can (and does in my case) actually remove that conditional\n>      as dead code!\n>\n> We can solve both by representing this state as an enum value. We could\n> do this by adding a new \"unknown\" value. But this really seems to match\n> the existing \"undefined\" level well. GPG describes this as \"Not enough\n> information for calculation\".\n>\n> We have tests in t7510 that trigger this case (verifying a signature\n> from a key that we don't have, and then checking various %G\n> placeholders), but they didn't notice the BUG() because we didn't look\n> at %GT for that case! Let's make sure we check all %G placeholders for\n> each case in the formatting tests.\n>\n> The interesting ones here are \"show unknown signature with custom\n> format\" and \"show lack of signature with custom format\", both of which\n> would BUG() before, and now turn %GT into \"undefined\". Prior to\n> 803978da49 they would have turned it into the empty string, but I think\n> saying \"undefined\" consistently is a reasonable outcome, and probably\n> makes life easier for anyone parsing the output (and any such parser had\n> to be ready to see \"undefined\" already).\n>\n> The other modified tests produce the same output before and after this\n> patch, but now we're consistently checking both %G? and %GT in all of\n> them.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> Reported-by: Rolf Eike Beer <eb@emlix.com>\n> ---\n>  gpg-interface.c          |  2 +-\n>  t/t7510-signed-commit.sh | 21 ++++++++++++++-------\n>  2 files changed, 15 insertions(+), 8 deletions(-)\n>\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index aceeb08336..f3ac5acdd9 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -650,7 +650,7 @@ int check_signature(struct signature_check *sigc,\n>  \tgpg_interface_lazy_init();\n>  \n>  \tsigc->result = 'N';\n> -\tsigc->trust_level = -1;\n> +\tsigc->trust_level = TRUST_UNDEFINED;\n>  \n>  \tfmt = get_format_by_sig(signature);\n>  \tif (!fmt)\n> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\n> index 48f86cb367..ccbc416402 100755\n> --- a/t/t7510-signed-commit.sh\n> +++ b/t/t7510-signed-commit.sh\n> @@ -221,84 +221,91 @@ test_expect_success GPG 'amending already signed commit' '\n>  test_expect_success GPG 'show good signature with custom format' '\n>  \tcat >expect <<-\\EOF &&\n>  \tG\n> +\tultimate\n>  \t13B6F51ECDDE430D\n>  \tC O Mitter <committer@example.com>\n>  \t73D758744BE721698EC54E8713B6F51ECDDE430D\n>  \t73D758744BE721698EC54E8713B6F51ECDDE430D\n>  \tEOF\n> -\tgit log -1 --format=\"%G?%n%GK%n%GS%n%GF%n%GP\" sixth-signed >actual &&\n> +\tgit log -1 --format=\"%G?%n%GT%n%GK%n%GS%n%GF%n%GP\" sixth-signed >actual &&\n>  \ttest_cmp expect actual\n>  '\n>  \n>  test_expect_success GPG 'show bad signature with custom format' '\n>  \tcat >expect <<-\\EOF &&\n>  \tB\n> +\tundefined\n>  \t13B6F51ECDDE430D\n>  \tC O Mitter <committer@example.com>\n>  \n>  \n>  \tEOF\n> -\tgit log -1 --format=\"%G?%n%GK%n%GS%n%GF%n%GP\" $(cat forged1.commit) >actual &&\n> +\tgit log -1 --format=\"%G?%n%GT%n%GK%n%GS%n%GF%n%GP\" $(cat forged1.commit) >actual &&\n>  \ttest_cmp expect actual\n>  '\n>  \n>  test_expect_success GPG 'show untrusted signature with custom format' '\n>  \tcat >expect <<-\\EOF &&\n>  \tU\n> +\tundefined\n>  \t65A0EEA02E30CAD7\n>  \tEris Discordia <discord@example.net>\n>  \tF8364A59E07FFE9F4D63005A65A0EEA02E30CAD7\n>  \tD4BE22311AD3131E5EDA29A461092E85B7227189\n>  \tEOF\n> -\tgit log -1 --format=\"%G?%n%GK%n%GS%n%GF%n%GP\" eighth-signed-alt >actual &&\n> +\tgit log -1 --format=\"%G?%n%GT%n%GK%n%GS%n%GF%n%GP\" eighth-signed-alt >actual &&\n>  \ttest_cmp expect actual\n>  '\n>  \n>  test_expect_success GPG 'show untrusted signature with undefined trust level' '\n>  \tcat >expect <<-\\EOF &&\n> +\tU\n>  \tundefined\n>  \t65A0EEA02E30CAD7\n>  \tEris Discordia <discord@example.net>\n>  \tF8364A59E07FFE9F4D63005A65A0EEA02E30CAD7\n>  \tD4BE22311AD3131E5EDA29A461092E85B7227189\n>  \tEOF\n> -\tgit log -1 --format=\"%GT%n%GK%n%GS%n%GF%n%GP\" eighth-signed-alt >actual &&\n> +\tgit log -1 --format=\"%G?%n%GT%n%GK%n%GS%n%GF%n%GP\" eighth-signed-alt >actual &&\n>  \ttest_cmp expect actual\n>  '\n>  \n>  test_expect_success GPG 'show untrusted signature with ultimate trust level' '\n>  \tcat >expect <<-\\EOF &&\n> +\tG\n>  \tultimate\n>  \t13B6F51ECDDE430D\n>  \tC O Mitter <committer@example.com>\n>  \t73D758744BE721698EC54E8713B6F51ECDDE430D\n>  \t73D758744BE721698EC54E8713B6F51ECDDE430D\n>  \tEOF\n> -\tgit log -1 --format=\"%GT%n%GK%n%GS%n%GF%n%GP\" sixth-signed >actual &&\n> +\tgit log -1 --format=\"%G?%n%GT%n%GK%n%GS%n%GF%n%GP\" sixth-signed >actual &&\n>  \ttest_cmp expect actual\n>  '\n>  \n>  test_expect_success GPG 'show unknown signature with custom format' '\n>  \tcat >expect <<-\\EOF &&\n>  \tE\n> +\tundefined\n>  \t65A0EEA02E30CAD7\n>  \n>  \n>  \n>  \tEOF\n> -\tGNUPGHOME=\"$GNUPGHOME_NOT_USED\" git log -1 --format=\"%G?%n%GK%n%GS%n%GF%n%GP\" eighth-signed-alt >actual &&\n> +\tGNUPGHOME=\"$GNUPGHOME_NOT_USED\" git log -1 --format=\"%G?%n%GT%n%GK%n%GS%n%GF%n%GP\" eighth-signed-alt >actual &&\n>  \ttest_cmp expect actual\n>  '\n>  \n>  test_expect_success GPG 'show lack of signature with custom format' '\n>  \tcat >expect <<-\\EOF &&\n>  \tN\n> +\tundefined\n>  \n>  \n>  \n>  \n>  \tEOF\n> -\tgit log -1 --format=\"%G?%n%GK%n%GS%n%GF%n%GP\" seventh-unsigned >actual &&\n> +\tgit log -1 --format=\"%G?%n%GT%n%GK%n%GS%n%GF%n%GP\" seventh-unsigned >actual &&\n>  \ttest_cmp expect actual\n>  '\n"},{"id":"475828","messageId":"20230422104758.GA2969939@coredump.intra.peff.net","threadId":"59614","inReplyTo":"xmqqy1mnanz8.fsf@gitster.g","subject":"Re: [PATCH] gpg-interface: set trust level of missing key to \"undefined\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-22T10:47:58Z","receivedAt":"2023-04-22T10:48:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 19, 2023 at 08:30:35AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Here's the patch that I came up with, though it does not distinguish\n> > between \"we did not see any trust level\" and \"gpg told us the trust\n> > level was undefined\". I think that's OK. That level is still below\n> > TRUST_NEVER. But if we really want to distinguish we can introduce a new\n> > value for the enum.\n> \n> Good.\n> \n> In my zeroth draft, I added to the enum a new TRUST_FAILED = -1 to\n> be used for the initialization assignment and get stringified in the\n> gpg_trust_level_to_str() function, which gave us the distinction and\n> made sure the enum is signed.  But in the end, I decided it was not\n> worth risking upsetting the end-user scripts that assumed the\n> current set of levels with a new \"level\" that is not known to them.\n> \n> Initializing to undefined like this patch is with much less damage\n> to the codebase, and existing end-user scripts are probably prepared\n> to react to \"undefined\" already and treat it as even less trustworthy\n> than the \"never\" ones.\n\nOne thing that I wondered about for using UNDEFINED is that we do this:\n\n  static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED;\n\nwhich is then later compared with:\n\n  status |= sigc->result != 'G';\n  status |= sigc->trust_level < configured_min_trust_level;\n\nSo before my patch the uninitialized state is (supposedly) less than the\nmin level, and after they are the same. For the reasons I gave in the\ncommit message, I think that less-than comparison was already broken.\nAnd likewise, for the reasons I gave, it hopefully never matters since\nthe result would never be 'G' in that case.\n\nSo I think it's fine, but I definitely had to stare at it for a while.\nThis all comes from 54887b4689 (gpg-interface: add minTrustLevel as a\nconfiguration option, 2019-12-27), which does discuss some of the\nimplications, but I think my patch is in line with the logic there.\n\n-Peff\n"},{"id":"475930","messageId":"xmqq4jp5gshf.fsf@gitster.g","threadId":"59614","inReplyTo":"20230422104758.GA2969939@coredump.intra.peff.net","subject":"Re: [PATCH] gpg-interface: set trust level of missing key to \"undefined\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-24T16:22:36Z","receivedAt":"2023-04-24T16:22:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So before my patch the uninitialized state is (supposedly) less than the\n> min level, and after they are the same. For the reasons I gave in the\n> commit message, I think that less-than comparison was already broken.\n> And likewise, for the reasons I gave, it hopefully never matters since\n> the result would never be 'G' in that case.\n\nYes * 2.\n\n> So I think it's fine, but I definitely had to stare at it for a while.\n> This all comes from 54887b4689 (gpg-interface: add minTrustLevel as a\n> configuration option, 2019-12-27), which does discuss some of the\n> implications, but I think my patch is in line with the logic there.\n\nYes.\n\nThanks.\n"}]}