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

From: Jeff King <peff@peff.net>
Date: Sep 16, 2023, 05:32 UTC
Message-ID: <20230916053201.GB13092@coredump.intra.peff.net>
In-Reply-To: <ZQSkjiyrOac4DK8q@nand.local>

On Fri, Sep 15, 2023 at 02:38:06PM -0400, Taylor Blau wrote:

Show 11 quoted lines

> 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.

Yes, though I'm less concerned with what the standard says than with what curl's code does (and it uses %d).

Show 8 quoted lines

> 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.

Yes, you're correct that we'd allow an empty stream identifier. I'm content to leave it in the name of simplicity.

Show 10 quoted lines

> 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;
>     }

Yes, that would work, but...

Show 14 quoted lines

> 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.

Your do-while is too eager, I think. It advances the first "p" before we've looked at it, so:

  - we'd match "[HTTP/2] [x1] [foo]", allowing one byte of non-digit
    cruft

  - if the string is "[HTTP/2] [", then "p" is at the NUL after the
    skip_iprefix call, and p++ walks us off the end of the array.

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

My inclination is to leave it. I was actually tempted to just allow _anything_ in the brackets if only because it makes the code even simpler, but the "skip past digits" seemed like a reasonable middle ground.

-Peff