Re: [PATCH v5 4/5] merge/pull Check for untrusted good GPG signatures

On Sun, Mar 31, 2013 at 02:27:20PM +0200, Sebastian Götte wrote:

On 03/31/2013 02:16 PM, Thomas Rast wrote:

quoted

"Sebastian Götte" [off-list ref] wrote:

quoted

expecting success: 
test_must_fail git merge --ff-only --verify-signatures side-untrusted
2>mergeerror &&
       test_i18ngrep "has a good, untrusted GPG signature" mergeerror

==1430== Conditional jump or move depends on uninitialised value(s)
==1430==    at 0x4C26B5C: strchrnul (mc_replace_strmem.c:711)
==1430==    by 0x47B90B: check_commit_signature (commit.c:1057)
==1430==    by 0x444212: cmd_merge (merge.c:1245)
==1430==    by 0x4050E6: handle_internal_command (git.c:281)
==1430==    by 0x40530C: main (git.c:489)

Though I also can't see the problem. strchrnul gets passed a char* that
is just fine.

Usually that means that the string *contents* are uninitialized,
usually because you scanned past the end of the string...

I checked for that, everything looks fine to me. The pointer should point to a valid, 0-terminated string.

It looks like the "found" pointer has wandered off the end of the
string.  In the test case here, the gpg_status is:

-- >8 --
[GNUPG:] SIG_ID rzX3GbdzQyxB4Jdm1uD0CzL4B4Y 2013-03-31 1364735152
[GNUPG:] GOODSIG 61092E85B7227189 Eris Discordia [off-list ref]
[GNUPG:] VALIDSIG D4BE22311AD3131E5EDA29A461092E85B7227189 2013-03-31
1364735152 0 4 0 1 2 00 D4BE22311AD3131E5EDA29A461092E85B7227189
[GNUPG:] TRUST_UNDEFINED
-- 8< --

But the parse_signature_lines code assumes that after reading a
signature it can fill in the key from the next 16 bytes and then look
for a newline after that.  In this case it clearly needs to only read
the signature if it's a GOODSIG or BADSIG line.

Wrapping a "signature_check[i].result != 'U'" condition around the lines
that extract the key and advance the "found" pointer after doing so
fixes this for me.


John