From a86f67b46edec7cd2789ceaaee395a8ecca5c5b0 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 1 Aug 2025 13:08:04 +0000 Subject: [PATCH 1/4] Work around parse errors of a bogus git.git commit The C Git source code repository itself contains a commit with bogus `gpgsig` lines: 5f549aa2f783 (pretty: %G[?GS] placeholders, 2011-10-21). Basically, this commit's headers contain a GPG signature that _should_ have been a multi-line header, but instead every line of that signature was added individually as a `gpgsig` header. Crucially, this includes a header line that starts with `gpgsig` followed by a space and that's already the end of the line. Gitoxide's commit header parser was not prepared for that. Even worse: the error message in that instance was simply empty. Let's make the parser more robust by accepting such commit headers, and add a test to verify that the commit in question _can_ be parsed by Gitoxide. Signed-off-by: Johannes Schindelin --- gix-object/src/commit/decode.rs | 2 +- .../commit/bogus-gpgsig-lines-in-git.git.txt | 31 +++++++++++++++++++ gix-object/tests/object/commit/from_bytes.rs | 17 ++++++++++ gix-object/tests/object/commit/mod.rs | 2 ++ 4 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 gix-object/tests/fixtures/commit/bogus-gpgsig-lines-in-git.git.txt diff --git a/gix-object/src/commit/decode.rs b/gix-object/src/commit/decode.rs index 4d8bc402724..c62f4a30daf 100644 --- a/gix-object/src/commit/decode.rs +++ b/gix-object/src/commit/decode.rs @@ -51,7 +51,7 @@ pub fn commit<'a, E: ParserError<&'a [u8]> + AddContext<&'a [u8], StrContext>>( alt(( parse::any_header_field_multi_line.map(|(k, o)| (k.as_bstr(), Cow::Owned(o))), |i: &mut _| { - parse::any_header_field(i, take_till(1.., NL)) + parse::any_header_field(i, take_till(0.., NL)) .map(|(k, o)| (k.as_bstr(), Cow::Borrowed(o.as_bstr()))) }, )), diff --git a/gix-object/tests/fixtures/commit/bogus-gpgsig-lines-in-git.git.txt b/gix-object/tests/fixtures/commit/bogus-gpgsig-lines-in-git.git.txt new file mode 100644 index 00000000000..948e5e44a79 --- /dev/null +++ b/gix-object/tests/fixtures/commit/bogus-gpgsig-lines-in-git.git.txt @@ -0,0 +1,31 @@ +tree db079155794727ac821adfba2eb68b330cc0c120 +parent 33a11a20eb7610771268e34211509cbbdee76b1e +author Junio C Hamano 1319256362 -0700 +committer Junio C Hamano 1319259176 -0700 +gpgsig -----BEGIN PGP SIGNATURE----- +gpgsig Version: GnuPG v1.4.10 (GNU/Linux) +gpgsig +gpgsig iQIcBAABAgAGBQJOokwoAAoJELC16IaWr+bL0WoP/2QKYkWpEyXF608m2L/cB9Qx +gpgsig /N0oBjyL1guIjPX9B3Wxq80dnLLEPnpnO39aiQIXFoJS0L6KEurqK6uDPHy3/ULa +gpgsig QsAug2HeCLsDnIFbzFSpSIMv/bP/72FDb/idMBT99xTcQj5UJEUgj7AAtx0vnKvQ +gpgsig pQIXtPu5GBUdhl3SiGgiJFRyp4r5EgV5I40GBwx/ty9cPEIN7ukJ3CR9+KC8eIGx +gpgsig Az7qngi3dhTn7g+3Z8XX5OYFDMSt9xn1gxqWXOMNlG0mxCvpFe59kwciugp26KVp +gpgsig n+yJ0UOdoxyZX8pdqXQjvklmoo7e55aaxtbHe7KSD56ebL7h7vHhkGWORU1dOp+h +gpgsig Iv5dQItkKSR8afB7FmRjo8+B/2g0wZDKRTGhzm7d1gooO5gpXvuvm4GRl5Io+IEj +gpgsig c7Li3EYmXADWUZWJtbDKDgKGKIOmWv72Qrz52iaESrhZ909HiXfn/jhHBuDRmLtQ +gpgsig /4v3T4O25uhdZ4p/PjHQn/ZroCmDyMwmnrtw/tt5fSNrl4qGcYg8Jj/1ynfF1AtS +gpgsig dM2LR65sOwXzSsqAbQjyRFYMLSWhHd/h8BcpZHDXmNBkZJVPm4zvD3ZVaAo6rtZD +gpgsig WJ9YXWXtPhuf09OgYBzcBlamTrk9ByH+NCIdrFkqfhNF1YI5dArSZytIXJhpPI1e +gpgsig TrmQAZf0BiI5J6PYN0AI +gpgsig =Qg/+ +gpgsig -----END PGP SIGNATURE----- + +pretty: %G[?GS] placeholders + +Add new placeholders related to the GPG signature on signed commits. + + - %GG to show the raw verification message from GPG; + - %G? to show either "G" for Good, "B" for Bad; + - %GS to show the name of the signer. + +Signed-off-by: Junio C Hamano diff --git a/gix-object/tests/object/commit/from_bytes.rs b/gix-object/tests/object/commit/from_bytes.rs index 7f9fea67abd..21e4275f02e 100644 --- a/gix-object/tests/object/commit/from_bytes.rs +++ b/gix-object/tests/object/commit/from_bytes.rs @@ -342,3 +342,20 @@ fn newline_right_after_signature_multiline_header() -> crate::Result { assert!(commit.message.starts_with(b"Rollup")); Ok(()) } + +#[test] +fn bogus_multi_gpgsig_header() -> crate::Result { + let fixture = fixture_name("commit", "bogus-gpgsig-lines-in-git.git.txt"); + let commit = CommitRef::from_bytes(&fixture)?; + let pgp_sig = crate::commit::BEGIN_PGP_SIGNATURE.as_bstr(); + assert_eq!(commit.extra_headers[0].1.as_ref(), pgp_sig); + assert_eq!(commit.extra_headers().pgp_signature(), Some(pgp_sig)); + assert_eq!( + commit.extra_headers().find(gix_object::commit::SIGNATURE_FIELD_NAME), + Some(pgp_sig) + ); + assert_eq!(commit.extra_headers().find_pos("gpgsig"), Some(0)); + assert_eq!(commit.extra_headers().find_pos("something else"), None); + assert!(commit.message.starts_with(b"pretty: %G[?GS] placeholders")); + Ok(()) +} diff --git a/gix-object/tests/object/commit/mod.rs b/gix-object/tests/object/commit/mod.rs index abc63a191ba..cf50683db41 100644 --- a/gix-object/tests/object/commit/mod.rs +++ b/gix-object/tests/object/commit/mod.rs @@ -149,6 +149,8 @@ iyBBl69jASy41Ug/BlFJbw4+ItkShpXwkJKuBBV/JExChmvbxYWaS7QnyYC9UO0= "; +const BEGIN_PGP_SIGNATURE: &[u8] = b"-----BEGIN PGP SIGNATURE-----"; + mod method { use gix_object::CommitRef; use pretty_assertions::assert_eq; From a0660fe3b4227b206dc6df099f55ec5bb41edce0 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 3 Aug 2025 10:52:45 +0000 Subject: [PATCH 2/4] Allow empty-valued commit headers in more places I just fixed a problem where a `gpgsig ` header line in a historical commit in the git.git repository itself was mistakenly causing a parse error. The culprit was, of course, that the header value is empty, and the existing code expected the value _not_ to be empty. Handle similar issues in other locations, too. Signed-off-by: Johannes Schindelin --- gix-object/src/commit/decode.rs | 2 +- gix-object/src/commit/ref_iter.rs | 4 ++-- gix-object/src/parse.rs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/gix-object/src/commit/decode.rs b/gix-object/src/commit/decode.rs index c62f4a30daf..a3efcc0596d 100644 --- a/gix-object/src/commit/decode.rs +++ b/gix-object/src/commit/decode.rs @@ -44,7 +44,7 @@ pub fn commit<'a, E: ParserError<&'a [u8]> + AddContext<&'a [u8], StrContext>>( .context(StrContext::Expected("author ".into())), (|i: &mut _| parse::header_field(i, b"committer", parse::signature)) .context(StrContext::Expected("committer ".into())), - opt(|i: &mut _| parse::header_field(i, b"encoding", take_till(1.., NL))) + opt(|i: &mut _| parse::header_field(i, b"encoding", take_till(0.., NL))) .context(StrContext::Expected("encoding ".into())), repeat( 0.., diff --git a/gix-object/src/commit/ref_iter.rs b/gix-object/src/commit/ref_iter.rs index d9d59f7498e..df953c1da1e 100644 --- a/gix-object/src/commit/ref_iter.rs +++ b/gix-object/src/commit/ref_iter.rs @@ -214,7 +214,7 @@ impl<'a> CommitRefIter<'a> { } } Encoding => { - let encoding = opt(|i: &mut _| parse::header_field(i, b"encoding", take_till(1.., NL))) + let encoding = opt(|i: &mut _| parse::header_field(i, b"encoding", take_till(0.., NL))) .context(StrContext::Expected("encoding ".into())) .parse_next(input)?; *state = State::ExtraHeaders; @@ -227,7 +227,7 @@ impl<'a> CommitRefIter<'a> { let extra_header = opt(alt(( |i: &mut _| parse::any_header_field_multi_line(i).map(|(k, o)| (k.as_bstr(), Cow::Owned(o))), |i: &mut _| { - parse::any_header_field(i, take_till(1.., NL)) + parse::any_header_field(i, take_till(0.., NL)) .map(|(k, o)| (k.as_bstr(), Cow::Borrowed(o.as_bstr()))) }, ))) diff --git a/gix-object/src/parse.rs b/gix-object/src/parse.rs index 7d90e2c81b9..bbe63abd1df 100644 --- a/gix-object/src/parse.rs +++ b/gix-object/src/parse.rs @@ -18,7 +18,7 @@ pub(crate) fn any_header_field_multi_line<'a, E: ParserError<&'a [u8]> + AddCont ( terminated(take_till(1.., SPACE_OR_NL), SPACE), ( - take_till(1.., NL), + take_till(0.., NL), NL, repeat(1.., terminated((SPACE, take_until(0.., NL)), NL)).map(|()| ()), ) From 8ad43f13012c7c8b05887d9f3d32f011d0c46274 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 3 Aug 2025 15:15:05 +0200 Subject: [PATCH 3/4] refactor - make the test more specific to what it's testing - fix a bug in the way multi-line headers are written so the object can be serialised. --- gix-object/src/encode.rs | 4 +++- gix-object/tests/object/commit/from_bytes.rs | 23 +++++++++++++------- gix-object/tests/object/commit/mod.rs | 2 -- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/gix-object/src/encode.rs b/gix-object/src/encode.rs index 2da523c078d..e2ce0addf69 100644 --- a/gix-object/src/encode.rs +++ b/gix-object/src/encode.rs @@ -38,7 +38,9 @@ pub(crate) fn header_field_multi_line(name: &[u8], value: &[u8], out: &mut dyn i let mut lines = value.as_bstr().lines_with_terminator(); out.write_all(name)?; out.write_all(SPACE)?; - out.write_all(lines.next().ok_or(Error::EmptyValue)?)?; + if let Some(line) = lines.next() { + out.write_all(line)?; + } for line in lines { out.write_all(SPACE)?; out.write_all(line)?; diff --git a/gix-object/tests/object/commit/from_bytes.rs b/gix-object/tests/object/commit/from_bytes.rs index 21e4275f02e..b89213394da 100644 --- a/gix-object/tests/object/commit/from_bytes.rs +++ b/gix-object/tests/object/commit/from_bytes.rs @@ -1,10 +1,10 @@ use gix_actor::SignatureRef; -use gix_object::{bstr::ByteSlice, commit::message::body::TrailerRef, CommitRef}; +use gix_object::{bstr::ByteSlice, commit::message::body::TrailerRef, CommitRef, WriteTo}; use smallvec::SmallVec; use crate::{ commit::{LONG_MESSAGE, MERGE_TAG, SIGNATURE}, - fixture_name, linus_signature, signature, + fixture_name, hex_to_id, linus_signature, signature, }; #[test] @@ -347,15 +347,22 @@ fn newline_right_after_signature_multiline_header() -> crate::Result { fn bogus_multi_gpgsig_header() -> crate::Result { let fixture = fixture_name("commit", "bogus-gpgsig-lines-in-git.git.txt"); let commit = CommitRef::from_bytes(&fixture)?; - let pgp_sig = crate::commit::BEGIN_PGP_SIGNATURE.as_bstr(); - assert_eq!(commit.extra_headers[0].1.as_ref(), pgp_sig); + let pgp_sig = b"-----BEGIN PGP SIGNATURE-----".as_bstr(); assert_eq!(commit.extra_headers().pgp_signature(), Some(pgp_sig)); assert_eq!( - commit.extra_headers().find(gix_object::commit::SIGNATURE_FIELD_NAME), - Some(pgp_sig) + commit.extra_headers().find_all("gpgsig").count(), + 17, + "Each signature header line is prefixed with `gpgsig` here, so we parse it as extra header" ); - assert_eq!(commit.extra_headers().find_pos("gpgsig"), Some(0)); - assert_eq!(commit.extra_headers().find_pos("something else"), None); assert!(commit.message.starts_with(b"pretty: %G[?GS] placeholders")); + + let mut buf = Vec::::new(); + commit.write_to(&mut buf)?; + let actual = gix_object::compute_hash(gix_hash::Kind::Sha1, gix_object::Kind::Commit, &buf)?; + assert_eq!( + actual, + hex_to_id("5f549aa2f78314ac37bbd436c8f80aea4c752e07"), + "round-tripping works despite the strangeness" + ); Ok(()) } diff --git a/gix-object/tests/object/commit/mod.rs b/gix-object/tests/object/commit/mod.rs index cf50683db41..abc63a191ba 100644 --- a/gix-object/tests/object/commit/mod.rs +++ b/gix-object/tests/object/commit/mod.rs @@ -149,8 +149,6 @@ iyBBl69jASy41Ug/BlFJbw4+ItkShpXwkJKuBBV/JExChmvbxYWaS7QnyYC9UO0= "; -const BEGIN_PGP_SIGNATURE: &[u8] = b"-----BEGIN PGP SIGNATURE-----"; - mod method { use gix_object::CommitRef; use pretty_assertions::assert_eq; From 191067ea5781f1f92b4ef3bc710832cb9391be4f Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 3 Aug 2025 15:21:31 +0200 Subject: [PATCH 4/4] remove unused struct. --- gitoxide-core/src/index/information.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/gitoxide-core/src/index/information.rs b/gitoxide-core/src/index/information.rs index d9fe591f505..64a63e87496 100644 --- a/gitoxide-core/src/index/information.rs +++ b/gitoxide-core/src/index/information.rs @@ -33,9 +33,6 @@ mod serde_only { } } } - - #[derive(serde::Serialize, serde::Deserialize)] - pub struct NodeId {} } }