quicwg / base-drafts

Internet-Drafts that make up the base QUIC specification
https://quicwg.org
1.62k stars 204 forks source link

negative base values #4938

Closed kazuho closed 2 years ago

kazuho commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

LPardue commented 2 years ago

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

bencebeky commented 2 years ago

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.

MikeBishop commented 2 years ago

Fixed in #4940.