New issue
Advanced search Search tips

Issue 596582 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 6
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 596523



Sign in to add a comment

URLRequestJob::NotifyHeadersComplete() permits a content-length header that contains a leading +

Project Member Reported by eroman@chromium.org, Mar 21 2016

Issue description

By virtue of using base::StringToInt64() and not checking for a leading plus.

Moreover, it does not check for failure from StringToInt64() return value.
 

Comment 1 by eroman@chromium.org, Mar 21 2016

Components: Internals>Network>HTTP

Comment 2 by mmenke@chromium.org, Mar 21 2016

eroman:  I'm assuming you're not planning on taking these on?  Thinking of adding a bug to reference all of these, and tossing it into a fixit (Which I'm actually planning on holding next quarter).

Comment 3 by eroman@chromium.org, Mar 21 2016

I believe that is covered by the blocking bug (issue 596523).

If you agree with https://bugs.chromium.org/p/chromium/issues/detail?id=596523#c7 then I can handle these. If not we can kick it to the fixit bucket.

Comment 4 by mmenke@chromium.org, Mar 21 2016

Oops...  Used to the old trackers double messages on linking bug dependencies, and didn't see the second message for that.

I agree with issue 596523.

Comment 5 by eroman@chromium.org, Mar 23 2016

Owner: eroman@chromium.org
Status: Assigned (was: Untriaged)
Components: Internals>Network
Components: -Internals>Network>HTTP
Owner: ----
Status: Untriaged (was: Assigned)
Owner: mmenke@chromium.org
Status: Assigned (was: Untriaged)
I'll poke at this - look at other browsers first, then make the change.
Actually...we have like 25 places that set content.length, and a grand total of 2 that actually use it.  Maybe we can just remove it?  The download system does use it, I guess (Which matters for blobs and data URLs, at least), but I'll see how things look without it.
URLFetcher uses it in download progress events - I think we may be able to manage without it in SimpleURLLoader, though, so going to put off any removal until then.  Have a patch to fix the + issue, at least, which I will send out for review, later today.
Project Member

Comment 12 by bugdroid1@chromium.org, Aug 6

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ff4871b346b7188f232c4c346d58a4d5a4c07c93

commit ff4871b346b7188f232c4c346d58a4d5a4c07c93
Author: Matt Menke <mmenke@chromium.org>
Date: Mon Aug 06 19:33:22 2018

URLRequestJob allowed leading +'s when getting expected content size

Not only are leading +'s not allowed in the Content-Length according to
spec, our normal Content-Length parsing code doesn't allow them either.
This CL switches to using the HttpResponseHeaders Content-Length parser,
to match HttpStreamParser's behavior.

Unclear if this makes any sense for H2/QUIC, or if we should be doing
something else entirely.

Bug:  596582 
Change-Id: I0520ade19f7eaa50cb9f4aab60338a19e97a1bd4
Reviewed-on: https://chromium-review.googlesource.com/1161250
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580950}
[modify] https://crrev.com/ff4871b346b7188f232c4c346d58a4d5a4c07c93/net/url_request/url_request_job.cc
[modify] https://crrev.com/ff4871b346b7188f232c4c346d58a4d5a4c07c93/net/url_request/url_request_job_unittest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment