From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-09-18 13:14:38
From: Andrzej Hunt <redacted>
parse_feature_value() does not update offset if the feature being
searched for does not specify a value. A loop that uses
parse_feature_value() to find a feature which was specified without a
value therefore might never exit (such loops will typically use
next_server_feature_value() as opposed to parse_feature_value() itself).
This usually isn't an issue: there's no point in using
next_server_feature_value() to search for repeated instances of the same
capability unless that capability typically specifies a value - but a
broken server could send a response that omits the value for a feature
even when we are expecting a value.
Therefore we add an offset update calculation for the no-value case,
which helps ensure that loops using next_server_feature_value() will
always terminate.
next_server_feature_value(), and the offset calculation, were first
added in 2.28 in:
2c6a403d96 (connect: add function to parse multiple v1 capability values, 2020-05-25)
Thanks to Peff for authoring the test.
Co-authored-by: Jeff King [off-list ref]
Signed-off-by: Jeff King <redacted>
Signed-off-by: Andrzej Hunt <redacted>
---
connect: also update offset for features without values
This is a small patch to avoid an infinite loop which can occur when a
broken server forgets to include a value when specifying symref in the
capabilities list.
Thanks to Peff for writing the test.
Note: I modified the test by adding and object-format=... to the
injected server response, because the oid that we're using is the
default hash (which will be e.g. sha256 for some CI jobs), but our
protocol handler assumes sha1 unless a different hash has been
explicitly specified. I'm open to alternative suggestions.
ATB,
Andrzej
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1091%2Fahunt%2Fconnectloop-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1091/ahunt/connectloop-v1
Pull-Request: https://github.com/git/git/pull/1091
connect.c | 2 ++
t/t5704-protocol-violations.sh | 13 +++++++++++++
2 files changed, 15 insertions(+)
From: Taylor Blau <hidden> Date: 2021-09-18 15:53:04
Hi Andrzej,
On Sat, Sep 18, 2021 at 01:14:32PM +0000, Andrzej Hunt via GitGitGadget wrote:
From: Andrzej Hunt <redacted>
Thanks for writing this patch. I have seen a copy of this on the
security list, but the modified version here looks good to me, too. I
left a few notes throughout.
Recapping our discussion on the security list, we decided that this
didn't merit an embargoed release because a misbehaving server can still
cause a client to hang if it simply printed half of its ref
advertisement. So this issue isn't new, but fixing this instance of it
is good nonetheless.
parse_feature_value() does not update offset if the feature being
searched for does not specify a value. A loop that uses
parse_feature_value() to find a feature which was specified without a
value therefore might never exit (such loops will typically use
next_server_feature_value() as opposed to parse_feature_value() itself).
This usually isn't an issue: there's no point in using
next_server_feature_value() to search for repeated instances of the same
capability unless that capability typically specifies a value - but a
broken server could send a response that omits the value for a feature
even when we are expecting a value.
It may be worth adding a little detail here. parse_feature_value takes
an offset, and uses it to seek past the point in features_list that
we've already seen. But if we get a value-less feature, then offset is
never updated, and we'll keep parsing the same thing over and over in a
loop.
(I know that you know all of that, but I think it is worth spelling out
a little more clearly in the patch message).
Therefore we add an offset update calculation for the no-value case,
which helps ensure that loops using next_server_feature_value() will
always terminate.
next_server_feature_value(), and the offset calculation, were first
added in 2.28 in:
2c6a403d96 (connect: add function to parse multiple v1 capability values, 2020-05-25)
This line wrapping is a little odd, but not a big deal.
quoted hunk
Thanks to Peff for authoring the test.
Co-authored-by: Jeff King [off-list ref]
Signed-off-by: Jeff King <redacted>
Signed-off-by: Andrzej Hunt <redacted>
---
connect: also update offset for features without values
This is a small patch to avoid an infinite loop which can occur when a
broken server forgets to include a value when specifying symref in the
capabilities list.
Thanks to Peff for writing the test.
Note: I modified the test by adding and object-format=... to the
injected server response, because the oid that we're using is the
default hash (which will be e.g. sha256 for some CI jobs), but our
protocol handler assumes sha1 unless a different hash has been
explicitly specified. I'm open to alternative suggestions.
ATB,
Andrzej
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1091%2Fahunt%2Fconnectloop-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1091/ahunt/connectloop-v1
Pull-Request: https://github.com/git/git/pull/1091
connect.c | 2 ++
t/t5704-protocol-violations.sh | 13 +++++++++++++
2 files changed, 15 insertions(+)
The critical piece :-). Since feature_list is a superset of found, this
is perfectly safe. It calculates first the offset of the found string
within feature_list, and then adds the length of the feature name.
I would have found this easier to read if it were spelled out as:
*offset = found - features_list + len;
which is the same thing but follows the order of how I spelled out this
expression in English. But the way you wrote it matches how
parse_feature_value() sets the offset when there is a value, so I think
it's worth being consistent with that.
@@ -32,4 +32,17 @@ test_expect_success 'extra delim packet in v2 fetch args' 'test_i18ngrep"expected flush after fetch arguments"err'+test_expect_success'bogus symref in v0 capabilities''+test_commitfoo&&+oid=$(gitrev-parseHEAD)&&+{+printf"%s HEAD\0symref object-format=%s\n""$oid""$GIT_DEFAULT_HASH"|+test-toolpkt-linepack-raw-stdin&&
I'm actually really happy with this modification to add the non-empty
object-format after the broken "symref" part, since it ensures that your
offset calculation is right (and that we can continue to parse features
with or without values after a value-less one).
From: brian m. carlson <hidden> Date: 2021-09-18 17:21:33
On 2021-09-18 at 13:14:32, Andrzej Hunt via GitGitGadget wrote:
From: Andrzej Hunt <redacted>
parse_feature_value() does not update offset if the feature being
searched for does not specify a value. A loop that uses
parse_feature_value() to find a feature which was specified without a
value therefore might never exit (such loops will typically use
next_server_feature_value() as opposed to parse_feature_value() itself).
This usually isn't an issue: there's no point in using
next_server_feature_value() to search for repeated instances of the same
capability unless that capability typically specifies a value - but a
broken server could send a response that omits the value for a feature
even when we are expecting a value.
Therefore we add an offset update calculation for the no-value case,
which helps ensure that loops using next_server_feature_value() will
always terminate.
next_server_feature_value(), and the offset calculation, were first
added in 2.28 in:
2c6a403d96 (connect: add function to parse multiple v1 capability values, 2020-05-25)
Thanks to Peff for authoring the test.
Thanks to both of you for the patch and test.
Note: I modified the test by adding and object-format=... to the
injected server response, because the oid that we're using is the
default hash (which will be e.g. sha256 for some CI jobs), but our
protocol handler assumes sha1 unless a different hash has been
explicitly specified. I'm open to alternative suggestions.
Yeah, this seems sensible. A few lines above, we compute the value
offset ("value") as "found + len", where "found" starts as
"feature_list", so this will be either the space following this value or
the NUL byte at the end of the string. That means that we'll make
progress next time because strstr will start searching from that point.
(I'm sure all of this is obvious to you, but I'm just mentioning it to
ensure that my understanding of the code is the same as everyone
else's.)
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
I'm actually really happy with this modification to add the non-empty
object-format after the broken "symref" part, since it ensures that your
offset calculation is right (and that we can continue to parse features
with or without values after a value-less one).
I don't think it quite does that, though. If I understand the parsing
code correctly, it walks through the list looking for entries for a
_particular_ capability. I.e., it will look for any "symref" entries,
advancing the offset counter. And then separately it will start again
looking for any object-format entries, with a brand-new offset counter
starting at 0.
So if you want to confirm that the parsing continues after the
unexpected entry, you'd want a second symref entry, and then to make
sure it was correctly parsed. Perhaps something like this:
I don't think it's hugely important (after all, this is something that
the server isn't supposed to send in the first place). But given that we
did make it work correctly (and the original on the security list
didn't), it's not too bad to test it on top.
Swapping out the "printf >expect" for a here-doc might make it a bit
more readable. I used printf because of the tab handling, but:
tab=$(printf "\t")
cat >expect <<-EOF
ref: ${dst}${tab}HEAD
${oid}${tab}HEAD
EOF
isn't too bad.
-Peff
I'm actually really happy with this modification to add the non-empty
object-format after the broken "symref" part, since it ensures that your
offset calculation is right (and that we can continue to parse features
with or without values after a value-less one).
I don't think it quite does that, though. If I understand the parsing
code correctly, it walks through the list looking for entries for a
_particular_ capability. I.e., it will look for any "symref" entries,
advancing the offset counter. And then separately it will start again
looking for any object-format entries, with a brand-new offset counter
starting at 0.
Ah; you're absolutely right. We call next_server_feature_value from
annotate_refs_with_symref_info() and server_supports_hash(), each of
which initializes their own offset from zero.
So if you want to confirm that the parsing continues after the
unexpected entry, you'd want a second symref entry, and then to make
sure it was correctly parsed. Perhaps something like this:
[...]
Yeah, I agree that would exercise it, and I also agree that it isn't
hugely important. But this patch does make an effort to handle that
case, so it's probably worth testing.
Thanks,
Taylor
From: Eric Sunshine <hidden> Date: 2021-09-19 01:04:05
On Sat, Sep 18, 2021 at 6:05 PM Jeff King [off-list ref] wrote:
Swapping out the "printf >expect" for a here-doc might make it a bit
more readable. I used printf because of the tab handling, but:
tab=$(printf "\t")
cat >expect <<-EOF
ref: ${dst}${tab}HEAD
${oid}${tab}HEAD
EOF
isn't too bad.
Or just use q_to_tab():
q_to_tab >expect <<-EOF
ref: ${dst}QHEAD
${oid}QHEAD
EOF
However, the typical use-case for q_to_tab() is when we need a leading
or trailing TAB character. When TAB is embedded within the line, we
often just use a literal TAB character; indeed, many tests in the
suite do exactly that, so that would be an even simpler option.
From: Jeff King <hidden> Date: 2021-09-19 02:20:05
On Sat, Sep 18, 2021 at 09:02:37PM -0400, Eric Sunshine wrote:
On Sat, Sep 18, 2021 at 6:05 PM Jeff King [off-list ref] wrote:
quoted
Swapping out the "printf >expect" for a here-doc might make it a bit
more readable. I used printf because of the tab handling, but:
tab=$(printf "\t")
cat >expect <<-EOF
ref: ${dst}${tab}HEAD
${oid}${tab}HEAD
EOF
isn't too bad.
Or just use q_to_tab():
q_to_tab >expect <<-EOF
ref: ${dst}QHEAD
${oid}QHEAD
EOF
However, the typical use-case for q_to_tab() is when we need a leading
or trailing TAB character.
Ah, yeah, I forgot we had that. I _thought_ we had a variable ($HT or
something) for this, but it looks like we only define and use it in a
few scripts.
I'm not sure using q_to_tab() is all that readable here, because it
blends into the HEAD token.
When TAB is embedded within the line, we
often just use a literal TAB character; indeed, many tests in the
suite do exactly that, so that would be an even simpler option.
Yeah, that'd probably be OK. I usually shy away from embedded tabs
because they can cause confusion in editors. But we have them already,
and this kind of expected output is not touched all that often.
-Peff
From: Eric Sunshine <hidden> Date: 2021-09-19 02:53:57
On Sat, Sep 18, 2021 at 10:20 PM Jeff King [off-list ref] wrote:
On Sat, Sep 18, 2021 at 09:02:37PM -0400, Eric Sunshine wrote:
quoted
Or just use q_to_tab():
Ah, yeah, I forgot we had that. I _thought_ we had a variable ($HT or
something) for this, but it looks like we only define and use it in a
few scripts.
Perhaps you were thinking about the LF or SQ variables defined by t/test-lib.sh?
From: Andrzej Hunt <hidden> Date: 2021-09-26 15:14:43
On 18/09/2021 17:53, Taylor Blau wrote:
quoted
parse_feature_value() does not update offset if the feature being
searched for does not specify a value. A loop that uses
parse_feature_value() to find a feature which was specified without a
value therefore might never exit (such loops will typically use
next_server_feature_value() as opposed to parse_feature_value() itself).
This usually isn't an issue: there's no point in using
next_server_feature_value() to search for repeated instances of the same
capability unless that capability typically specifies a value - but a
broken server could send a response that omits the value for a feature
even when we are expecting a value.
It may be worth adding a little detail here. parse_feature_value takes
an offset, and uses it to seek past the point in features_list that
we've already seen. But if we get a value-less feature, then offset is
never updated, and we'll keep parsing the same thing over and over in a
loop.
(I know that you know all of that, but I think it is worth spelling out
a little more clearly in the patch message).
Good point - I've tried to improve this for V2 (I've mostly just copied
your description verbatim).
quoted
Therefore we add an offset update calculation for the no-value case,
which helps ensure that loops using next_server_feature_value() will
always terminate.
quoted
next_server_feature_value(), and the offset calculation, were first
added in 2.28 in:
2c6a403d96 (connect: add function to parse multiple v1 capability values, 2020-05-25)
This line wrapping is a little odd, but not a big deal.
I'll fix this for V2 - I think I tried too hard to make this look nice,
but putting the reference inline does look better (and I've now realised
this seems to be the usual way of doing things here).
ATB,
Andrzej
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-09-26 15:58:40
From: Andrzej Hunt <redacted>
parse_feature_value() takes an offset, and uses it to seek past the
point in features_list that we've already seen. However if the feature
being searched for does not specify a value, the offset is not
updated. Therefore if we call parse_feature_value() in a loop on a
value-less feature, we'll keep on parsing the same feature over and over
again. This usually isn't an issue: there's no point in using
next_server_feature_value() to search for repeated instances of the same
capability unless that capability typically specifies a value - but a
broken server could send a response that omits the value for a feature
even when we are expecting a value.
Therefore we add an offset update calculation for the no-value case,
which helps ensure that loops using next_server_feature_value() will
always terminate.
next_server_feature_value(), and the offset calculation, were first
added in 2.28 in 2c6a403d96 (connect: add function to parse multiple
v1 capability values, 2020-05-25).
Thanks to Peff for authoring the test.
Co-authored-by: Jeff King [off-list ref]
Signed-off-by: Jeff King <redacted>
Signed-off-by: Andrzej Hunt <redacted>
---
connect: also update offset for features without values
V2 incorporates Peff's test and test stability improvements, and
attempts to improve the commit message.
ATB,
Andrzej
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1091%2Fahunt%2Fconnectloop-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1091/ahunt/connectloop-v2
Pull-Request: https://github.com/git/git/pull/1091
Range-diff vs v1:
1: dcbb05ddc4b ! 1: 908e4e6c4ed connect: also update offset for features without values
@@ Metadata
## Commit message ##
connect: also update offset for features without values
- parse_feature_value() does not update offset if the feature being
- searched for does not specify a value. A loop that uses
- parse_feature_value() to find a feature which was specified without a
- value therefore might never exit (such loops will typically use
- next_server_feature_value() as opposed to parse_feature_value() itself).
- This usually isn't an issue: there's no point in using
+ parse_feature_value() takes an offset, and uses it to seek past the
+ point in features_list that we've already seen. However if the feature
+ being searched for does not specify a value, the offset is not
+ updated. Therefore if we call parse_feature_value() in a loop on a
+ value-less feature, we'll keep on parsing the same feature over and over
+ again. This usually isn't an issue: there's no point in using
next_server_feature_value() to search for repeated instances of the same
capability unless that capability typically specifies a value - but a
broken server could send a response that omits the value for a feature
@@ Commit message
always terminate.
next_server_feature_value(), and the offset calculation, were first
- added in 2.28 in:
- 2c6a403d96 (connect: add function to parse multiple v1 capability values, 2020-05-25)
+ added in 2.28 in 2c6a403d96 (connect: add function to parse multiple
+ v1 capability values, 2020-05-25).
Thanks to Peff for authoring the test.
@@ t/t5704-protocol-violations.sh: test_expect_success 'extra delim packet in v2 fe
+test_expect_success 'bogus symref in v0 capabilities' '
+ test_commit foo &&
+ oid=$(git rev-parse HEAD) &&
++ dst=refs/heads/foo &&
+ {
-+ printf "%s HEAD\0symref object-format=%s\n" "$oid" "$GIT_DEFAULT_HASH" |
++ printf "%s HEAD\0symref object-format=%s symref=HEAD:%s\n" \
++ "$oid" "$GIT_DEFAULT_HASH" "$dst" |
+ test-tool pkt-line pack-raw-stdin &&
+ printf "0000"
+ } >input &&
-+ git ls-remote --upload-pack="cat input ;:" . >actual &&
-+ printf "%s\tHEAD\n" "$oid" >expect &&
++ git ls-remote --symref --upload-pack="cat input; read junk;:" . >actual &&
++ printf "ref: %s\tHEAD\n%s\tHEAD\n" "$dst" "$oid" >expect &&
+ test_cmp expect actual
+'
+
connect.c | 2 ++
t/t5704-protocol-violations.sh | 15 +++++++++++++++
2 files changed, 17 insertions(+)
From: Jeff King <hidden> Date: 2021-09-27 19:47:51
On Sun, Sep 26, 2021 at 03:58:33PM +0000, Andrzej Hunt via GitGitGadget wrote:
connect: also update offset for features without values
V2 incorporates Peff's test and test stability improvements, and
attempts to improve the commit message.