Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

negative base values #4938

Closed
kazuho opened this issue Sep 27, 2021 · 6 comments · Fixed by #4940
Closed

negative base values #4938

kazuho opened this issue Sep 27, 2021 · 6 comments · Fixed by #4940
Labels
-qpack editorial has-consensus

Comments

@kazuho
Copy link
Member

@kazuho kazuho commented Sep 27, 2021

Section 4.5.1.2 of QPACK states that Base is calculated as:

   if S == 0:
      Base = ReqInsertCount + DeltaBase
   else:
      Base = ReqInsertCount - DeltaBase - 1

As Base is defined as an index of a table, I tend to believe that it is assumed to be non-negative. Am I correct?

I am asking this because I cannot find any statement that states as such, even though section 7.4 states that decoders might refuse very large values.

I understand that it might be too late to consider changes to the draft, but would like to see if we have consensus that QPACK encoders are prohibited from generating an Encoded Field Section Prefix that causes Base to become negative.

@martinthomson
Copy link
Member

@martinthomson martinthomson commented Sep 27, 2021

We reject as invalid anything that underflows. We also (now) reject anything that produces a base >= 264. I don't see any other way to handle that situation.

@MikeBishop
Copy link
Contributor

@MikeBishop MikeBishop commented Sep 27, 2021

That might fall in the "reasonable clarification" bucket, but @afrind and @quicwg/chairs would need to weigh in on that. All integers in QPACK are intended to be positive, but this is (I think) the only instance in which one integer is subtracted from another, creating a potentially-negative result.

@kazuho
Copy link
Member Author

@kazuho kazuho commented Sep 28, 2021

Thank you for your comments. It's assuring to know that others think the same way; i.e. that Base should be positive.

@martinthomson

We also (now) reject anything that produces a base >= 264. I don't see any other way to handle that situation.

That sounds totally fine when the underlying transport is QUIC v1, because the encoder can send no more than ~262 instructions due to the size limit of a QUIC stream. Even if we ignore this v1-specific property, endpoints would have the opportunity to send GOAWAY before base exceeds 264.

@martinthomson martinthomson added -qpack editorial labels Sep 29, 2021
@LPardue
Copy link
Member

@LPardue LPardue commented Oct 4, 2021

Is there a concrete proposal here? Seeing a PR would let us understand the scope of change being proposed.

martinthomson added a commit that referenced this issue Oct 4, 2021
Happy to use MAY here, which should have the same net effect.

Closes #4938.
@bencebeky
Copy link
Contributor

@bencebeky bencebeky commented Oct 5, 2021

Kazuho, nice find! I agree with your intuition. In fact when I implemented QPACK decoding back in December 2018 I already assumed Base must not be negative and made our decoder close the connection (not just the stream) with QPACK_DECOMPRESSION_FAILED if it was negative, see https://github.com/google/quiche/blob/a6ef0a64fd597e53003c7846192790ed0e3642b5/quic/core/qpack/qpack_progressive_decoder.cc#L331. This implementation has been used by Google's servers and by Chrome ever since we support HTTP/3, and I have not seen reports of interop problems, so I assume not many encoders out there generate negative Base values.

I support adding clarification to the specs.

@LPardue LPardue added the has-consensus label Nov 22, 2021
@MikeBishop
Copy link
Contributor

@MikeBishop MikeBishop commented May 11, 2022

Fixed in #4940.

@MikeBishop MikeBishop closed this May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-qpack editorial has-consensus
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants