On Fri, Sep 15, 2023 at 02:38:06PM -0400, Taylor Blau wrote:
> 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).
> 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.
> 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...
> 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
