Avoid adding invalid headers in AddHeaderFromString

Avoid adding invalid headers in AddHeaderFromString

In net::HttpRequestHeaders::AddHeaderFromString(), verify that the
header name and value are valid. If they are not, don't add them (and
log a fatal message on debug builds).

BUG=634225
Committed: https://crrev.com/0753a6f4ffdb5bcd1ff9c4b977f9052878112f42
Cr-Commit-Position: refs/heads/master@{#411556}

[Adam Rice, 2016-08-09 10:18:19]

The CQ bit was checked by ricea@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 10:18:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Adam Rice, 2016-08-09 11:10:15]

ricea@chromium.org changed reviewers:
+ mmenke@chromium.org

[Adam Rice, 2016-08-09 11:10:16]

+mmenke We won't want this if we're going to eliminate AddHeaderFromString()
altogether. But it looks to me like the calls in
net/url_request/test_url_fetcher_factory.cc and
net/url_request/url_fetcher_core.cc will be hard to eliminate.

[commit-bot: I haz the power, 2016-08-09 11:13:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 11:13:16]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-08-09 16:54:47]

https://codereview.chromium.org/2225933004/diff/1/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/2225933004/diff/1/net/http/http_util.cc#newco...
net/http/http_util.cc:427: }  // namespace
The more common pattern in net/ is to use a single anonymous namespace at the
top of the file.

https://codereview.chromium.org/2225933004/diff/1/net/http/http_util.cc#newco...
net/http/http_util.cc:461: if (string.size() == 0)
!string.empty() is preferred, as it doesn't require computing the length of the
string.

https://codereview.chromium.org/2225933004/diff/1/net/http/http_util.h
File net/http/http_util.h (right):

https://codereview.chromium.org/2225933004/diff/1/net/http/http_util.h#newcod...
net/http/http_util.h:120: return IsToken(base::StringPiece(&*begin, end -
begin));
Is this guaranteed to work?

[Adam Rice, 2016-08-10 03:18:19]

The CQ bit was checked by ricea@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 03:18:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Adam Rice, 2016-08-10 03:19:08]

https://codereview.chromium.org/2225933004/diff/1/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/2225933004/diff/1/net/http/http_util.cc#newco...
net/http/http_util.cc:427: }  // namespace
On 2016/08/09 16:54:47, mmenke wrote:
> The more common pattern in net/ is to use a single anonymous namespace at the
> top of the file.

Done. I can move the rest of the anonymous namespaces in this file in a separate
CL if you'd like.

https://codereview.chromium.org/2225933004/diff/1/net/http/http_util.cc#newco...
net/http/http_util.cc:461: if (string.size() == 0)
On 2016/08/09 16:54:47, mmenke wrote:
> !string.empty() is preferred, as it doesn't require computing the length of
the
> string.

I can't believe I did that. Fixed.

https://codereview.chromium.org/2225933004/diff/1/net/http/http_util.h
File net/http/http_util.h (right):

https://codereview.chromium.org/2225933004/diff/1/net/http/http_util.h#newcod...
net/http/http_util.h:120: return IsToken(base::StringPiece(&*begin, end -
begin));
On 2016/08/09 16:54:47, mmenke wrote:
> Is this guaranteed to work?

Interesting question. I had to go look at the C++ standard. For the "is the type
const char *" question, the most conclusive reference I can find is in 21.4.1:

The char-like objects in a basic_string object shall be stored contiguously.
That is, for any basic_string
object s, the identity &*(s.begin() + n) == &*s.begin() + n shall hold for all
values of n such that 0
<= n < s.size().

This at least strongly implies that the type of *s.begin() is char and not some
proxy type.

I think if |begin| is off-the-end then *begin is undefined behaviour, so I have
added a check for that.

[mmenke, 2016-08-10 03:30:55]

https://codereview.chromium.org/2225933004/diff/1/net/http/http_util.h
File net/http/http_util.h (right):

https://codereview.chromium.org/2225933004/diff/1/net/http/http_util.h#newcod...
net/http/http_util.h:120: return IsToken(base::StringPiece(&*begin, end -
begin));
On 2016/08/10 03:19:07, Adam Rice wrote:
> On 2016/08/09 16:54:47, mmenke wrote:
> > Is this guaranteed to work?
> 
> Interesting question. I had to go look at the C++ standard. For the "is the
type
> const char *" question, the most conclusive reference I can find is in 21.4.1:
> 
> The char-like objects in a basic_string object shall be stored contiguously.
> That is, for any basic_string
> object s, the identity &*(s.begin() + n) == &*s.begin() + n shall hold for all
> values of n such that 0
> <= n < s.size().
> 
> This at least strongly implies that the type of *s.begin() is char and not
some
> proxy type.
> 
> I think if |begin| is off-the-end then *begin is undefined behaviour, so I
have
> added a check for that.

And std::string::const_iterators are required to return const char&'s?  This
just seems very weird and obscure.

Looks like this method is only called in 3 places.  How hard would it be to just
get rid of this override?

[commit-bot: I haz the power, 2016-08-10 04:30:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 04:30:15]

Dry run: This issue passed the CQ dry run.

[Adam Rice, 2016-08-10 05:22:59]

https://codereview.chromium.org/2225933004/diff/1/net/http/http_util.h
File net/http/http_util.h (right):

https://codereview.chromium.org/2225933004/diff/1/net/http/http_util.h#newcod...
net/http/http_util.h:120: return IsToken(base::StringPiece(&*begin, end -
begin));
On 2016/08/10 03:30:55, mmenke wrote:
> And std::string::const_iterators are required to return const char&'s?  This
> just seems very weird and obscure.

To be fair, it is widely used: https://cs.chromium.org/search/?q=%22%26*%22

> Looks like this method is only called in 3 places.  How hard would it be to
just
> get rid of this override?

Take a look at Patch Set 4. I'm not sure it's better.

[Adam Rice, 2016-08-10 05:36:42]

The CQ bit was checked by ricea@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 05:37:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 06:56:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 06:56:06]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-08-10 20:15:19]

LGTM, moduloe comment.  Not going to ask you to convert anything else over to
StringPieces, though longer term, think that's where we should go

https://codereview.chromium.org/2225933004/diff/60001/net/http/http_content_d...
File net/http/http_content_disposition.cc (right):

https://codereview.chromium.org/2225933004/diff/60001/net/http/http_content_d...
net/http/http_content_disposition.cc:352: HttpUtil::TrimLWS(&type_begin,
&type_end);
There's a base::StringPiece version of this, too.  Can go back beyond that
without running into HttpContentDisposition::Parse, unfortuantely, which has
multiple callsites, and goes a bit beyond the scope of this CL.

[Adam Rice, 2016-08-12 04:19:46]

The CQ bit was checked by ricea@chromium.org

[Adam Rice, 2016-08-12 04:19:46]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2225933004/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-08-12 04:20:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 05:21:14]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-12 05:22:43]

Description was changed from

==========
Avoid adding invalid headers in AddHeaderFromString

In net::HttpRequestHeaders::AddHeaderFromString(), verify that the
header name and value are valid. If they are not, don't add them (and
log a fatal message on debug builds).

BUG=634225
==========

to

==========
Avoid adding invalid headers in AddHeaderFromString

In net::HttpRequestHeaders::AddHeaderFromString(), verify that the
header name and value are valid. If they are not, don't add them (and
log a fatal message on debug builds).

BUG=634225

Committed: https://crrev.com/0753a6f4ffdb5bcd1ff9c4b977f9052878112f42
Cr-Commit-Position: refs/heads/master@{#411556}
==========

[commit-bot: I haz the power, 2016-08-12 05:22:44]

Patchset 7 (id:??) landed as
https://crrev.com/0753a6f4ffdb5bcd1ff9c4b977f9052878112f42
Cr-Commit-Position: refs/heads/master@{#411556}

[Adam Rice, 2016-08-12 05:48:33]

https://codereview.chromium.org/2225933004/diff/60001/net/http/http_content_d...
File net/http/http_content_disposition.cc (right):

https://codereview.chromium.org/2225933004/diff/60001/net/http/http_content_d...
net/http/http_content_disposition.cc:352: HttpUtil::TrimLWS(&type_begin,
&type_end);
On 2016/08/10 20:15:19, mmenke wrote:
> There's a base::StringPiece version of this, too.  Can go back beyond that
> without running into HttpContentDisposition::Parse, unfortuantely, which has
> multiple callsites, and goes a bit beyond the scope of this CL.

Done.

HttpUtil::HeadersIterator can be cleaned up similarly, but it would be cleanest
to change HttpResponseHeaders at the same time, and this CL has seen enough
scope creep.