On Fri, Sep 15, 2023 at 07:34:43AM -0400, Jeff King wrote:
> @@ -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
