Re: [PATCH] connect: also update offset for features without values

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 2987 bytes --]

On 2021-09-18 at 13:14:32, Andrzej Hunt via GitGitGadget wrote:
> From: Andrzej Hunt <andrzej@ahunt.org>
> 
> 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.

This is a fine solution, I think.

>     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(+)
> 
> diff --git a/connect.c b/connect.c
> index aff13a270e6..eaf7d6d2618 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -557,6 +557,8 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i
>  			if (!*value || isspace(*value)) {
>  				if (lenp)
>  					*lenp = 0;
> +				if (offset)
> +					*offset = found + len - feature_list;

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]