git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
brian m. carlson <sandals@crustytoothpaste.net>
Date
Sep 18, 2021, 17:18 UTC
Message-ID
<YUYfVM8jMkPG85qT@camp.crustytoothpaste.net>
In-Reply-To
<pull.1091.git.git.1631970872884.gitgitgadget@gmail.com>
On 2021-09-18 at 13:14:32, Andrzej Hunt via GitGitGadget wrote:
Show 22 quoted lines
> 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.
Show 5 quoted lines
>     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.
Show 22 quoted lines
>     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
Previous: Andrzej HuntNext: Junio C Hamano
Message 10 of 17 in “connect: also update offset for features without values”
  1. connect: also update offset for features without valuesAndrzej Hunt via GitGitGadget, Sep 18, 2021
  2. Taylor BlauSep 18, 2021
  3. Jeff KingSep 18, 2021
  4. Taylor BlauSep 18, 2021
  5. Eric SunshineSep 19, 2021
  6. Jeff KingSep 19, 2021
  7. Eric SunshineSep 19, 2021
  8. Junio C HamanoSep 19, 2021
  9. Andrzej HuntSep 26, 2021
  10. brian m. carlsonSep 18, 2021
  11. Junio C HamanoSep 23, 2021
  12. Jeff KingSep 23, 2021
  13. Junio C HamanoSep 23, 2021
  14. Jeff KingSep 23, 2021
  15. Andrzej HuntSep 26, 2021
  16. connect: also update offset for features without valuesAndrzej Hunt via GitGitGadget, Sep 26, 2021
  17. Jeff KingSep 27, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.