Adding commandline option to override bans on certain port numbers through a ...

Adding commandline option to override bans on certain port numbers through a comma separated list of ports.

BUG=18307
TEST=url_request_unittest, use commandline flag allowed_ports=1,600. Navigate to http://www.google.com:1 or http://www.google.com:600. You should not get an ERR_UNSAFE_PORT, it will attempt to load the page.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=24883

[jar (doing other things), 2009-08-29 14:28:18]

http://codereview.chromium.org/176032/diff/7001/1006
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/176032/diff/7001/1006#newcode850
Line 850: URLRequestHttpJob::set_overridden_banned_ports(port_strings);
nit/interface design suggestion: 
IMO, all parsing and error checking should be done in one place.  Hence, I think
that the command line string, or n vector of ints (that are indeed checked as
valid ints) should be passed.

With regard to where the parsing etc. is done, browser_init is something that
grows indefinitely, and would be better if made shorter (and easier to read). 
Hence, with this much parsing and error checking, I'd rather it all be done in
set_overridden_banned_ports. (yeah... other code is doing a lot of parsing
here... but you can see this file is just getting too large).

I'd also prefer to avoid things kindred to a double negative in the method name.
 Hence perhaps the method name could be set_allowed_ports or
set_forced_allowed_ports.

http://codereview.chromium.org/176032/diff/7001/1009
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/176032/diff/7001/1009#newcode245
Line 245: extern const wchar_t kBannedPortsOverride[];
nit: I'd suggest a name which better avoids a double negative, such as
kEnabledPorts.

To make the confusion clearer:

Is this a replacement for the list of banned ports?  It overrides them, so do we
list all the banned ports here??

Or is this a list that reverses the decision to ban them, and allows the ports?

http://codereview.chromium.org/176032/diff/7001/1008
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/176032/diff/7001/1008#newcode28
Line 28: // Specifies a list of ports that should be accepted despite bans.
This comment makes it clear what the intent is... but as noted, the name is
confusing.  If you like the prose, you could call it: acceptable_ports.  I still
think allowed_ports is clearer.

http://codereview.chromium.org/176032/diff/7001/1008#newcode30
Line 30: const std::vector<std::wstring> ports) {
If you stick with this interface, you should pass in a const reference to
vector, rather than a const vector.

http://codereview.chromium.org/176032/diff/7001/1008#newcode132
Line 132: static std::vector<std::wstring> overridden_banned_ports_;
IMO, this should probably be a vector of ints, or probably even more useful, a
set of ints.

[Miranda Callahan, 2009-08-29 18:05:35]

http://codereview.chromium.org/176032/diff/7001/1008
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/176032/diff/7001/1008#newcode28
Line 28: // Specifies a list of ports that should be accepted despite bans.
On 2009/08/29 14:28:18, jar wrote:
> This comment makes it clear what the intent is... but as noted, the name is
> confusing.  If you like the prose, you could call it: acceptable_ports.  I
still
> think allowed_ports is clearer.

I don't know; if you call this "allowed_ports," that's not quite right, since it
implies that all other ports are banned, which -- I assume -- isn't correct. 
How about "banned_port_exceptions"?  I know it's still a double negative, but I
think it's more accurate than just saying that the ports are allowed, which
isn't quite what this list is about... What do you think?

http://codereview.chromium.org/176032/diff/7001/1008#newcode132
Line 132: static std::vector<std::wstring> overridden_banned_ports_;
+1 for set of ints: log(n) vs. linear search time, and no need to do the string
conversion with every port comparison.

[pierre.lafayette, 2009-08-29 18:24:56]

Updated. I've used allowed_ports for now, awaiting feedback on the naming
changes.

http://codereview.chromium.org/176032

[Miranda Callahan, 2009-08-29 20:10:53]

lgtm.

[jar (doing other things), 2009-08-29 22:24:40]

http://codereview.chromium.org/176032/diff/7008/7013
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/176032/diff/7008/7013#newcode601
Line 601: // Explicitly allow ports using a comma separated list of port
numbers.
IMO, the above comment is fine.  If you want to be more descriptive (based on
the comment in the review, you could say:

Explicitly allow additional ports using a comma separatel list of port numbers.

http://codereview.chromium.org/176032/diff/7008/7010
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/176032/diff/7008/7010#newcode73
Line 73: const std::wstring::value_type d = L',';
nit: avoid use of abbreviation |d| for |delimiter|.  Const variales should also
begin with k.

IMO, better yet would be to use the name kComma.

http://codereview.chromium.org/176032/diff/7008/7010#newcode75
Line 75: for (size_t i = 0; i <= size; ++i) {
I'm a little surprised that you would index the array with i==size.  I would
expect that to be a bad reference, as I don't think you have a null terminator
to count on.

I think this would be a subtle memory violation, and would have thrown an
exception if you had used at() method of indexing.

http://codereview.chromium.org/176032/diff/7008/7010#newcode85
Line 85: TrimWhitespace(tmp, TRIM_ALL, &trim_tmp);
I guess you're assuming the command line might have a quoted string, and hence
whitespace is possible??  IMO, this is an unnecessary complication.  It also
leads to confused parsing when faced with arguments such as:

"1,2,3 4 5,,,, ,   ,   8  9  10"
IMO, the above should be illegal.  I'm not clear on how you'd parse the above.

I'd suggest the command line consist of ONLY digits and commas, and parsing
would be much simpler.

In any case, please add a unit test with a number of clear examples for this
parsing logic.

Perhaps the original coding format, using split(), would be more effective.

http://codereview.chromium.org/176032/diff/7008/7011
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/176032/diff/7008/7011#newcode32
Line 32: static const std::set<int>& allowed_ports() { return allowed_ports_; }
IMO, the above names are great.  If here is additional concern about ambigiuity
(per email reviews), you could call this "explicitly_allowed_ports"

[Miranda Callahan, 2009-08-29 22:59:04]

http://codereview.chromium.org/176032/diff/7008/7011
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/176032/diff/7008/7011#newcode32
Line 32: static const std::set<int>& allowed_ports() { return allowed_ports_; }
On 2009/08/29 22:24:41, jar wrote:
> IMO, the above names are great.  If here is additional concern about
ambigiuity
> (per email reviews), you could call this "explicitly_allowed_ports"

I like this idea; I still think it's a tad misleading to just say
"allowed_ports", because there are ports that are allowed that are not on this
list -- and some future coder may stumble over the distinction.  The name jar
suggests also avoids the double-negative.

[pierre.lafayette, 2009-08-30 01:48:47]

Updated. Went with explicitly_allowed_ports.

http://codereview.chromium.org/176032/diff/7008/7010
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/176032/diff/7008/7010#newcode75
Line 75: for (size_t i = 0; i <= size; ++i) {
This algorithm is from the SplitString function. However, as you pointed out, I
missed a small subtlety. The indexing of the string should only be after the
condition that compares i to size in the if statement, that way the string is
never indexed at allowed_ports[size].

On 2009/08/29 22:24:41, jar wrote:
> I'm a little surprised that you would index the array with i==size.  I would
> expect that to be a bad reference, as I don't think you have a null terminator
> to count on.
> 
> I think this would be a subtle memory violation, and would have thrown an
> exception if you had used at() method of indexing.

[jar (doing other things), 2009-08-30 04:12:57]

Thanks for adding the unittest!!!

Some minor stuff below.

Thanks,

Jim

http://codereview.chromium.org/176032/diff/3028/3029
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/176032/diff/3028/3029#newcode835
Line 835: TrimString(temp, L",", &allowed_ports);
This is still a remnant part of the parsing (or lexing) that deletes leading and
trailing commas.  You should probably leave this work to the set function.  I
think your parsing handles such items anyway (assuming a zero length string gets
converted to a zero??).

http://codereview.chromium.org/176032/diff/3028/3029#newcode836
Line 836: URLRequestHttpJob::set_explicitly_allowed_ports(allowed_ports);
style nit: hacker style names such as "set_explicitly_allowed_ports" should only
be used when the code is extremely simple (generally an assignment).  This
should probably be SetExplicitlyAllowedPorts().

http://codereview.chromium.org/176032/diff/3028/3030
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/176032/diff/3028/3030#newcode65
Line 65: void URLRequestHttpJob::set_explicitly_allowed_ports(
style nite: As mentioned elsewhere, hacker-style method names should only be
used for trivial methods (that take almost no time, and will likely be inlined).
 This one probably deserves bouncy-caps such as SetExpliitlyAllowedPorts().

http://codereview.chromium.org/176032/diff/3028/3030#newcode82
Line 82: size_t len = i - last;
style nit: |len| should probably be |length|.   Sorry I didn't notice this last
time.

http://codereview.chromium.org/176032/diff/3028/3030#newcode85
Line 85: ports.insert(StringToInt(tmp));
Since substr() reportedly throws an exception if last >= size, it is probably
cleaner to explicitly avoid that posibility, and only do a substring if len > 0.
Safer might be:
...len = i - last;
if (len > 0)
  ports.insert(StringToInt(allowed_ports.substr(last,len)));

You could also consider overflows, but I think this will only cause a string
conversion to return 0... but it is not clear from the prototype.  Perhaps at
least a comment that the user is on-their-own if bogusly large port numbers are
speicified... or else you have to do the conversion using a method that warns
about an overflow.

[pierre.lafayette, 2009-08-30 08:28:44]

Updated.
http://codereview.chromium.org/176032

[jar (doing other things), 2009-08-30 14:30:58]

LGTM  (minor style nit below)

http://codereview.chromium.org/176032/diff/1027/7020
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/176032/diff/1027/7020#newcode64
Line 64: // static
style nit: probably add newline between line 63 and 64.

[pierre.lafayette, 2009-08-30 16:29:59]

You'll need to land this for me.
Thanks.

http://codereview.chromium.org/176032/diff/1027/7020
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/176032/diff/1027/7020#newcode64
Line 64: // static
On 2009/08/30 14:30:59, jar wrote:
> style nit: probably add newline between line 63 and 64.

Done.

[jar (doing other things), 2009-08-30 17:35:26]

I'm OOO... and mostly trying to help via email.  To do a landing, I need to
watch the tree for an extended period of time.... and so I'd rather do so
when I'm back (in 8 days).  (althougth it is vaguely possible I can do so
today).
How quickly do you need this landed?   If sooner, I may have to get someone
else to help out.

Thanks,

Jim

On Sun, Aug 30, 2009 at 9:29 AM, <pierre.lafayette@gmail.com> wrote:

> You'll need to land this for me.
> Thanks.
>
>
> http://codereview.chromium.org/176032/diff/1027/7020
> File net/url_request/url_request_http_job.cc (right):
>
> http://codereview.chromium.org/176032/diff/1027/7020#newcode64
> Line 64: // static
> On 2009/08/30 14:30:59, jar wrote:
>
>> style nit: probably add newline between line 63 and 64.
>>
>
> Done.
>
>
> http://codereview.chromium.org/176032
>

[Miranda Callahan, 2009-08-30 18:42:38]

I can land this later today or tomorrow -- no problem.


On 2009/08/30 17:35:26, jar wrote:
> I'm OOO... and mostly trying to help via email.  To do a landing, I need to
> watch the tree for an extended period of time.... and so I'd rather do so
> when I'm back (in 8 days).  (althougth it is vaguely possible I can do so
> today).
> How quickly do you need this landed?   If sooner, I may have to get someone
> else to help out.
> 
> Thanks,
> 
> Jim
> 
> On Sun, Aug 30, 2009 at 9:29 AM, <mailto:pierre.lafayette@gmail.com> wrote:
> 
> > You'll need to land this for me.
> > Thanks.
> >
> >
> > http://codereview.chromium.org/176032/diff/1027/7020
> > File net/url_request/url_request_http_job.cc (right):
> >
> > http://codereview.chromium.org/176032/diff/1027/7020#newcode64
> > Line 64: // static
> > On 2009/08/30 14:30:59, jar wrote:
> >
> >> style nit: probably add newline between line 63 and 64.
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.org/176032
> >
>

[Miranda Callahan, 2009-08-31 17:07:21]

Landed as http://src.chromium.org/viewvc/chrome?view=rev&revision=24883.  I had
to make some tweaks to get this past linux and mac bots, but nothing major.

On 2009/08/30 18:42:38, Miranda Callahan wrote:
> I can land this later today or tomorrow -- no problem.
> 
> 
> On 2009/08/30 17:35:26, jar wrote:
> > I'm OOO... and mostly trying to help via email.  To do a landing, I need to
> > watch the tree for an extended period of time.... and so I'd rather do so
> > when I'm back (in 8 days).  (althougth it is vaguely possible I can do so
> > today).
> > How quickly do you need this landed?   If sooner, I may have to get someone
> > else to help out.
> > 
> > Thanks,
> > 
> > Jim
> > 
> > On Sun, Aug 30, 2009 at 9:29 AM, <mailto:pierre.lafayette@gmail.com> wrote:
> > 
> > > You'll need to land this for me.
> > > Thanks.
> > >
> > >
> > > http://codereview.chromium.org/176032/diff/1027/7020
> > > File net/url_request/url_request_http_job.cc (right):
> > >
> > > http://codereview.chromium.org/176032/diff/1027/7020#newcode64
> > > Line 64: // static
> > > On 2009/08/30 14:30:59, jar wrote:
> > >
> > >> style nit: probably add newline between line 63 and 64.
> > >>
> > >
> > > Done.
> > >
> > >
> > > http://codereview.chromium.org/176032
> > >
> >

[pierre.lafayette, 2009-09-01 05:17:50]

On 2009/08/31 17:07:21, Miranda Callahan wrote:
> Landed as http://src.chromium.org/viewvc/chrome?view=rev&revision=24883.  I
had
> to make some tweaks to get this past linux and mac bots, but nothing major.

Thanks!

[darin (slow to review), 2009-09-01 06:49:23]

http://codereview.chromium.org/176032/diff/9017/8006
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/176032/diff/9017/8006#newcode66
Line 66: void URLRequestHttpJob::SetExplicitlyAllowedPorts(
it seems like we'd want to allow port overrides for FTP as well, and possibly
for other protocols in the future.  perhaps it would be better to move this
override stuff to net_util so that it can be shared by other protocols.

[pierre.lafayette, 2009-09-01 12:48:22]

No problem. I'll make a separate patch for the refactoring.

On 2009/09/01 06:49:23, darin wrote:
> http://codereview.chromium.org/176032/diff/9017/8006
> File net/url_request/url_request_http_job.cc (right):
> 
> http://codereview.chromium.org/176032/diff/9017/8006#newcode66
> Line 66: void URLRequestHttpJob::SetExplicitlyAllowedPorts(
> it seems like we'd want to allow port overrides for FTP as well, and possibly
> for other protocols in the future.  perhaps it would be better to move this
> override stuff to net_util so that it can be shared by other protocols.

[darin (slow to review), 2009-09-01 14:03:19]

Thanks!

On Tue, Sep 1, 2009 at 5:48 AM, <pierre.lafayette@gmail.com> wrote:

> No problem. I'll make a separate patch for the refactoring.
>
>
> On 2009/09/01 06:49:23, darin wrote:
>
>> http://codereview.chromium.org/176032/diff/9017/8006
>> File net/url_request/url_request_http_job.cc (right):
>>
>
>  http://codereview.chromium.org/176032/diff/9017/8006#newcode66
>> Line 66: void URLRequestHttpJob::SetExplicitlyAllowedPorts(
>> it seems like we'd want to allow port overrides for FTP as well, and
>>
> possibly
>
>> for other protocols in the future.  perhaps it would be better to move
>>
> this
>
>> override stuff to net_util so that it can be shared by other
>>
> protocols.
>
>
>
> http://codereview.chromium.org/176032
>