Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2974613002:
Allow large response headers to buffer in HpackDecoder. (Closed)
|
Created:
3 years, 5 months ago by dahollings Modified:
3 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, bnc+watch_chromium.org, net-reviews_chromium.org, Ryan Hamilton, Biren Roy Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow large response headers to buffer in HpackDecoder. We were able to verify manually that building with this change fixes the issue with large response headers in Chrome for HTTP/2. Fix for QUIC will need further work which I have already started on server side (tracked under 161129433).
We settled on 512KB as the HPACK decoder buffer size limit, see comments for further discussion.
Review-Url: https://codereview.chromium.org/2974613002
Cr-Commit-Position: refs/heads/master@{#485738}
Committed: https://chromium.googlesource.com/chromium/src/+/bc9201c783c6e9c8609545e355f5eff5184cdbf4
Patch Set 1 #Patch Set 2 : 64MB --> 512KB #
Total comments: 1
Patch Set 3 : naught but a nit #
Messages
Total messages: 49 (29 generated)
The CQ bit was checked by dahollings@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dahollings@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dahollings@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Allow large response headers to buffer in HpackDecoder. ========== to ========== Allow large response headers to buffer in HpackDecoder. We were able to verify manually that building with this change fixes the issue with large response headers in Chrome for HTTP/2. Fix for QUIC will need further work which I have already started on server side (tracked under 161129433). ==========
Description was changed from ========== Allow large response headers to buffer in HpackDecoder. We were able to verify manually that building with this change fixes the issue with large response headers in Chrome for HTTP/2. Fix for QUIC will need further work which I have already started on server side (tracked under 161129433). ========== to ========== Allow large response headers to buffer in HpackDecoder. We were able to verify manually that building with this change fixes the issue with large response headers in Chrome for HTTP/2. Fix for QUIC will need further work which I have already started on server side (tracked under 161129433). I used 64MB as the HPACK decoder buffer size limit since that ends up being what is used for response headers on server, but feel free to suggest a different value. ==========
dahollings@chromium.org changed reviewers: + bnc@chromium.org
bnc@chromium.org changed reviewers: + rch@chromium.org
Looping in Ryan for a second opinion. I feel like 64 MB is way too large. Chrome/Cronet runs on many different platforms, and we would not want to make it use up so much memory on every connection. The current limit is 32 kB for this reason. Do we have any information about the distribution of response header sizes?
Looping in Ryan for a second opinion. I feel like 64 MB is way too large. Chrome/Cronet runs on many different platforms, and we would not want to make it use up so much memory on every connection. The current limit is 32 kB for this reason. Do we have any information about the distribution of response header sizes?
On 2017/07/10 12:56:44, Bence wrote: > Looping in Ryan for a second opinion. > > I feel like 64 MB is way too large. I wasn't sure, so I just followed GFE's value. I will defer to your and Ryan's judgment. More discussion below. > Chrome/Cronet runs on many different > platforms, and we would not want to make it use up so much memory on every > connection. This is the max size, but it would only use this much memory as needed. > The current limit is 32 kB for this reason. There's at least a few different limits in play here (and even more in the GFE...), and I doubt they are all working in total rational harmony. Attempted summary: Decoder limit on hpack fragments (defaults to 32KB, bumped to 64MB for GFE HTTP/2 response headers) https://cs.chromium.org/chromium/src/net/spdy/core/hpack/hpack_decoder3.cc?l=... Max string size (single field key/value) in hpack decoder (defaultly set to be same as above) https://cs.chromium.org/chromium/src/net/http2/hpack/decoder/hpack_whole_entr... SETTINGS_MAX_HEADER_LIST_SIZE, which GFE HTTP/2 sends and enforces at 32MB for response headers. It doesn't seem like Chrome HTTP/2 sends that SETTING, https://cs.chromium.org/chromium/src/net/spdy/core/spdy_protocol.h?type=cs&l=149 Chrome HTTP/2 does seem to have an implicit max header list size of 256KB. https://cs.chromium.org/chromium/src/net/spdy/chromium/header_coalescer.cc?l=... In the GFE, the decision was made to set the HPACK limits to be 2x the advertised/enforced header list size limit. One of the benefits of this is that you are protected against outrageous bugs/attacks without having to even decode (= DECOMPRESSION_ERROR), but you still send a sane HEADERS_TOO_LARGE error for headers that are within one factor over the limit. What do you think of doing something similar in Chrome, i.e. you/Ryan/me/everyone come up with a value for MAX_HEADER_LIST_SIZE that makes sense for Chrome, we advertise and enforce it, and set these HPACK limits to 2x the value we come up with? :) > Do we have any information about the distribution of response header sizes? Not that I am aware of, but that would be interesting. HTTP/2 and QUIC are not widely used as clients at the GFE yet to my knowledge, so it wouldn't show up in our usual error monitoring.
On 2017/07/10 12:56:44, Bence wrote: > Looping in Ryan for a second opinion. > > I feel like 64 MB is way too large. Chrome/Cronet runs on many different > platforms, and we would not want to make it use up so much memory on every > connection. The current limit is 32 kB for this reason. > > Do we have any information about the distribution of response header sizes? Perhaps we could land an UMA histogram for this? I agree that 64MB seems quite huge. That being said, it sounds like dahollings found a 256KB limit in a different place, so perhaps this 64MB limit doesn't actually come into play? It'd be great to make sure QUIC and HTTP/2. As it happens, in QUIC, I recently increased our limit to 256KB https://codereview.chromium.org/2909653004
On 2017/07/10 18:37:42, rch (use chromium not google) wrote: > On 2017/07/10 12:56:44, Bence wrote: > > Looping in Ryan for a second opinion. > > > > I feel like 64 MB is way too large. Chrome/Cronet runs on many different > > platforms, and we would not want to make it use up so much memory on every > > connection. The current limit is 32 kB for this reason. > > > > Do we have any information about the distribution of response header sizes? > > Perhaps we could land an UMA histogram for this? > > I agree that 64MB seems quite huge. That being said, it sounds like dahollings > found a 256KB limit in a different place, so perhaps this 64MB limit doesn't > actually come into play? > > It'd be great to make sure QUIC and HTTP/2. As it happens, in QUIC, I recently > increased our limit to 256KB https://codereview.chromium.org/2909653004 I got a chance to chat more with bnc@ about this earler. What do you two think of sending SETTINGS_MAX_HEADER_LIST_SIZE with 256KB (which is already enforced), and setting HPACK layer to reject fragments/strings above 512KB? That seems to me to strike a good balance on parity with existing policy/code, giving peers an opportunity to detect HEADERS_TOO_LARGE, and still protecting us from HPACK-decoding junk.
On 2017/07/10 19:01:46, dahollings wrote: > On 2017/07/10 18:37:42, rch (use chromium not google) wrote: > > On 2017/07/10 12:56:44, Bence wrote: > > > Looping in Ryan for a second opinion. > > > > > > I feel like 64 MB is way too large. Chrome/Cronet runs on many different > > > platforms, and we would not want to make it use up so much memory on every > > > connection. The current limit is 32 kB for this reason. > > > > > > Do we have any information about the distribution of response header sizes? > > > > Perhaps we could land an UMA histogram for this? > > > > I agree that 64MB seems quite huge. That being said, it sounds like dahollings > > found a 256KB limit in a different place, so perhaps this 64MB limit doesn't > > actually come into play? > > > > It'd be great to make sure QUIC and HTTP/2. As it happens, in QUIC, I recently > > increased our limit to 256KB https://codereview.chromium.org/2909653004 > > I got a chance to chat more with bnc@ about this earler. What do you two think > of sending SETTINGS_MAX_HEADER_LIST_SIZE with 256KB (which is already enforced), > and setting HPACK layer to reject fragments/strings above 512KB? That seems to > me to strike a good balance on parity with existing policy/code, giving peers an > opportunity to detect HEADERS_TOO_LARGE, and still protecting us from > HPACK-decoding junk. SGTM!
Newest patchset contains the HPACK change for Chrome HTTP/2. The SETTINGS change and QUIC will be separate. PTAL.
LGTM with nit. Thank you. https://codereview.chromium.org/2974613002/diff/20001/net/spdy/chromium/spdy_... File net/spdy/chromium/spdy_session.cc (right): https://codereview.chromium.org/2974613002/diff/20001/net/spdy/chromium/spdy_... net/spdy/chromium/spdy_session.cc:899: kMaxHeaderListSize); Nit: include header_coalescer.h for this.
The CQ bit was checked by dahollings@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org Link to the patchset: https://codereview.chromium.org/2974613002/#ps40001 (title: "naught but a nit")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dahollings@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
Description was changed from ========== Allow large response headers to buffer in HpackDecoder. We were able to verify manually that building with this change fixes the issue with large response headers in Chrome for HTTP/2. Fix for QUIC will need further work which I have already started on server side (tracked under 161129433). I used 64MB as the HPACK decoder buffer size limit since that ends up being what is used for response headers on server, but feel free to suggest a different value. ========== to ========== Allow large response headers to buffer in HpackDecoder. We were able to verify manually that building with this change fixes the issue with large response headers in Chrome for HTTP/2. Fix for QUIC will need further work which I have already started on server side (tracked under 161129433). We settled on 512KB as the HPACK decoder buffer size limit, see comments for further discussion. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dahollings@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by dahollings@chromium.org
The CQ bit was checked by dahollings@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by dahollings@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1499812870698330,
"parent_rev": "428a5f8309bbb20f55f4734ba31680d55498a40f", "commit_rev":
"bc9201c783c6e9c8609545e355f5eff5184cdbf4"}
Message was sent while issue was closed.
Description was changed from ========== Allow large response headers to buffer in HpackDecoder. We were able to verify manually that building with this change fixes the issue with large response headers in Chrome for HTTP/2. Fix for QUIC will need further work which I have already started on server side (tracked under 161129433). We settled on 512KB as the HPACK decoder buffer size limit, see comments for further discussion. ========== to ========== Allow large response headers to buffer in HpackDecoder. We were able to verify manually that building with this change fixes the issue with large response headers in Chrome for HTTP/2. Fix for QUIC will need further work which I have already started on server side (tracked under 161129433). We settled on 512KB as the HPACK decoder buffer size limit, see comments for further discussion. Review-Url: https://codereview.chromium.org/2974613002 Cr-Commit-Position: refs/heads/master@{#485738} Committed: https://chromium.googlesource.com/chromium/src/+/bc9201c783c6e9c8609545e355f5... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/bc9201c783c6e9c8609545e355f5... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
