No longer preconnect to unsafe ports.

net/http/http_stream_factory_impl_job.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/http/http_stream_factory_impl_job.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "base/string_util.h"
@@ skipping 467 matching lines @@
   return result;
 }
 
 int HttpStreamFactoryImpl::Job::DoLoop(int result) {
   DCHECK_NE(next_state_, STATE_NONE);
   int rv = result;
   do {
     State state = next_state_;
     next_state_ = STATE_NONE;
     switch (state) {
+      case STATE_START:
+        DCHECK_EQ(OK, rv);
+        rv = DoStart();
+        break;
       case STATE_RESOLVE_PROXY:
         DCHECK_EQ(OK, rv);
         rv = DoResolveProxy();
         break;
       case STATE_RESOLVE_PROXY_COMPLETE:
         rv = DoResolveProxyComplete(rv);
         break;
       case STATE_WAIT_FOR_JOB:
         DCHECK_EQ(OK, rv);
         rv = DoWaitForJob();
@@ skipping 29 matching lines @@
         NOTREACHED() << "bad state";
         rv = ERR_FAILED;
         break;
     }
   } while (rv != ERR_IO_PENDING && next_state_ != STATE_NONE);
   return rv;
 }
 
 int HttpStreamFactoryImpl::Job::StartInternal() {
   CHECK_EQ(STATE_NONE, next_state_);
-
-  origin_ = HostPortPair(request_info_.url.HostNoBrackets(),
-                         request_info_.url.EffectiveIntPort());
-  origin_url_ = HttpStreamFactory::ApplyHostMappingRules(
-      request_info_.url, &origin_);
-
+  next_state_ = STATE_START;
   net_log_.BeginEvent(NetLog::TYPE_HTTP_STREAM_JOB,
                       HttpStreamJobParameters::Create(request_info_.url,
                                                       origin_url_));

[wtc, 2012/01/14 01:34:46]

In the original code, we have already initialized origin_url_
at this point.  In the new code, origin_url_ is initialized
in DoStart().  It seems that we should also move this
net_log_.BeginEvent statement (which uses origin_url_) to
DoStart().  Do you agree?

[mmenke, 2012/01/14 02:19:21]

Definitely a bug.  Thanks for spotting this.

-  next_state_ = STATE_RESOLVE_PROXY;
   int rv = RunLoop(OK);
   DCHECK_EQ(ERR_IO_PENDING, rv);
   return rv;
 }
 
+int HttpStreamFactoryImpl::Job::DoStart() {

[willchan no longer on Chromium, 2011/12/12 18:24:16]

Almost all the rest of our states have DoFoo() and DoFooComplete(). Can we just
put this code into StartInternal()?

[mmenke, 2011/12/12 18:43:21]

Currently it's expected to always return asynchronously.  Changing that would
require modifying all call sites.

Alternatively, we could separate off the failure code in RunLoop off to another
function instead, and call that directly, or make DoResolveProxy take in an
error code as input instead, which seems a little weird.

I'm not a huge fan of either of those options, which is why I added a new state.

I think the best of those options is the DoResolveProxy() option, but passing it
an error unrelated to resolving the proxy seems a little weird.  If you prefer
one of the alternative approaches, just tell me which.

[willchan no longer on Chromium, 2011/12/12 19:04:58]

Oh I see. I think I'd like to modify all call sites. There appear to only be 2
of them. Does that seem reasonable?

[mmenke, 2011/12/12 19:10:03]

Sure, will do.

[mmenke, 2011/12/13 16:35:00]

Making this able to fail synchronously is enough of a pain that I'm just going
to put fixing this on hold for a while.  Because of the whole |alternate_job|
thing, I'll have to modify RequestStream's job/request creation function a fair
bit to prevent re-entrancy, and maybe change a couple class invariants.

Jobs require requests to be created, Jobs are only removed from requests on
failure or when one succeeds, which may result in a callback to the delegate if
there are no other jobs, so synchronous failing requires a lot of funkiness or a
significant modification of the request/job interface.

[mmenke, 2011/12/13 16:44:32]

Or I could just add an OnStartComplete function that just sets the the next
state to NONE or RESOLVE_PROXY, based on the error code.

+  // Don't connect to restricted ports.
+  int port = request_info_.url.EffectiveIntPort();
+  if (!IsPortAllowedByDefault(port) && !IsPortAllowedByOverride(port))
+    return ERR_UNSAFE_PORT;

[wtc, 2012/01/14 01:34:46]

Before DoResolveProxyComplete() returns an error code, it
calls dependent_job_->Resume(this) if there is a dependent
job.  See lines 596-600 below.

I think we should do the same here.

[mmenke, 2012/01/14 02:19:21]

I agree.  There's no fall through to automatically call it on error.

+
+  origin_ = HostPortPair(request_info_.url.HostNoBrackets(), port);
+  origin_url_ = HttpStreamFactory::ApplyHostMappingRules(
+      request_info_.url, &origin_);
+
+  next_state_ = STATE_RESOLVE_PROXY;
+  return OK;
+}
+
 int HttpStreamFactoryImpl::Job::DoResolveProxy() {
   DCHECK(!pac_request_);
 
   next_state_ = STATE_RESOLVE_PROXY_COMPLETE;
 
   if (request_info_.load_flags & LOAD_BYPASS_PROXY) {
     proxy_info_.UseDirect();
     return OK;
   }
 
@@ skipping 14 matching lines @@
 
     if (proxy_info_.is_empty()) {
       // No proxies/direct to choose from. This happens when we don't support
       // any of the proxies in the returned list.
       result = ERR_NO_SUPPORTED_PROXIES;
     }
   }
 
   if (result != OK) {
     if (dependent_job_)
       dependent_job_->Resume(this);

[wtc, 2012/01/14 01:34:46]

Should we set dependent_job_ to NULL after calling
dependent_job_->Resume(this)?  We do this elsewhere
in this file.  (Search for "dependent_job_->Resume".)

By the way, I find dependent_job_ very confusing.  (It
comes from the original code by willchan.)  I suggest
renaming it waiting_job_ (the job that is waiting for |this|).

[mmenke, 2012/01/14 02:19:21]

It shouldn't really be needed, since if we fail, the Request will end up
deleting |this|.  Since the other job can't fail (Or succeed) synchronously, we
also don't have to worry about what it will do, once started.

That having been said, it does seem like something we should do, just to protect
against future changes in behavior.

I've also renamed the job and updated the description in the header.

     return result;
   }
 
   if (blocking_job_)
     next_state_ = STATE_WAIT_FOR_JOB;
   else
     next_state_ = STATE_INIT_CONNECTION;
   return OK;
 }
 
@@ skipping 595 matching lines @@
     return false;
   }
   if (request_info_.method != "GET" && request_info_.method != "HEAD") {
     return false;
   }
   return stream_factory_->http_pipelined_host_pool_.IsHostEligibleForPipelining(
       origin_);
 }
 
 }  // namespace net