Re: [PATCH 2/2] http: update curl http/2 info matching for curl 8.3.0

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 15, 2023, 18:38 UTC
Message-ID: <ZQSkjiyrOac4DK8q@nand.local>
In-Reply-To: <20230915113443.GB3531587@coredump.intra.peff.net>

On Fri, Sep 15, 2023 at 07:34:43AM -0400, Jeff King wrote:

Show 16 quoted lines

> @@ -751,6 +753,18 @@ static int match_curl_h2_trace(const char *line, const char **out)
>  	    skip_iprefix(line, "h2 [", out))
>  		return 1;
>
> +	/*
> +	 * curl 8.3.0 uses:
> +	 *   [HTTP/2] [<stream-id>] [<header-name>: <header-val>]
> +	 * where <stream-id> is numeric.
> +	 */
> +	if (skip_iprefix(line, "[HTTP/2] [", &p)) {
> +		while (isdigit(*p))
> +			p++;
> +		if (skip_prefix(p, "] [", out))
> +			return 1;
> +	}
> +

This looks good, too, though I do have one question. The HTTP/2 specification in 5.1 says (among other things):

    Streams are identified with an unsigned 31-bit integer. Streams
    initiated by a client MUST use odd-numbered stream identifiers; those
    initiated by the server MUST use even-numbered stream identifiers. A
    stream identifier of zero (0x0) is used for connection control messages;
    the stream identifier of zero cannot be used to establish a new stream.

So the parsing you wrote here makes sense in that we consume digits between the pair of square brackets enclosing the stream identifier.

But I think we would happily eat a line like:

    [HTTP/2] [] [Secret: xyz]

even lacking a stream identifier. I think that's reasonably OK in practice, because we're being over-eager in redacting instead of the other way around. And we're unlikely to see such a line from curl anyway, so I don't think that it matters.

If you feel otherwise, though, I think something as simple as:

    if (skip_iprefix(line, "[HTTP/2] [", &p)) {
      if (!*p)
        return 0;
      while (isdigit(*p))
        p++;
      if (skip_prefix(p, "] [", out))
        return 1;
    }

would do the trick. I *think* that this would also work:

    if (skip_iprefix(line, "[HTTP/2] [", &p)) {
      do {
        p++;
      } while (isdigit(*p))
      if (skip_prefix(p, "] [", out))
        return 1;
    }

since we know that p is non-NULL, and if it's the end of the line, *p will be NUL and isdigit(*p) will return 0. But it's arguably less direct, and requires some extra reasoning, so I have a vague preference for the former.

But this may all be moot anyway, I don't feel strongly one way or the other.

Thanks, Taylor