Open a firewall hole when a TCP server or UDP socket is bound.

chromeos/network/firewall_hole.cc

+// Copyright 2015 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 "chromeos/network/firewall_hole.h"
+
+#include <fcntl.h>
+#include <unistd.h>
+
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/threading/worker_pool.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/permission_broker_client.h"
+#include "dbus/file_descriptor.h"
+
+namespace chromeos {
+
+namespace {
+
+void CreateValidLifeline(dbus::FileDescriptor* lifeline_local,
+                         dbus::FileDescriptor* lifeline_remote) {
+  int lifeline[2] = {-1, -1};
+  if (pipe2(lifeline, O_CLOEXEC) < 0) {
+    PLOG(ERROR) << "Failed to create a lifeline pipe";
+    return;
+  }
+
+  lifeline_local->PutValue(lifeline[0]);
+  lifeline_local->CheckValidity();
+
+  lifeline_remote->PutValue(lifeline[1]);
+  lifeline_remote->CheckValidity();
+}
+
+void OnPortReleased(FirewallHole::PortType type,
+                    uint16 port,
+                    const std::string& interface,
+                    dbus::FileDescriptor* lifeline_fd,
+                    bool success) {
+  const char* port_type;

[pneubeck (no reviews), 2015/03/03 22:24:51]

please explicitly initialize to null, or probably better move this to a helper
PortTypeToString that you can call directly in the LOG(...) <<.

Currently this code is executed even if success==true

[Reilly Grant (use Gerrit), 2015/03/04 01:35:50]

Done.

+  switch (type) {
+    case FirewallHole::TCP:
+      port_type = "TCP";
+      break;
+    case FirewallHole::UDP:
+      port_type = "UDP";
+      break;
+  }
+
+  if (!success) {
+    LOG(WARNING) << "Failed to release firewall hold for " << port_type
+                 << " port " << port << " on " << interface << ".";
+  }
+
+  base::WorkerPool::PostTask(
+      FROM_HERE,
+      base::Bind(&base::DeletePointer<dbus::FileDescriptor>, lifeline_fd),
+      false);
+}
+}
+
+// static
+void FirewallHole::Open(PortType type,
+                        uint16 port,
+                        const std::string& interface,
+                        const OpenCallback& callback) {
+  dbus::FileDescriptor* lifeline_local = new dbus::FileDescriptor();
+  dbus::FileDescriptor* lifeline_remote = new dbus::FileDescriptor();
+  FirewallHole* hole = new FirewallHole(type, port, interface, lifeline_local);
+  base::WorkerPool::PostTaskAndReply(
+      FROM_HERE,
+      base::Bind(&CreateValidLifeline, lifeline_local, lifeline_remote),
+      base::Bind(&FirewallHole::OnLifelineCreated, base::Unretained(hole),
+                 base::Owned(lifeline_remote), callback),
+      false);
+}
+
+FirewallHole::~FirewallHole() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (opened_) {
+    PermissionBrokerClient* client =
+        DBusThreadManager::Get()->GetPermissionBrokerClient();
+    if (client) {

[pneubeck (no reviews), 2015/03/03 22:24:51]

the 'return' below is a bit hidden.
would be clearer with
  DCHECK(client) << ...;
  ...
  return;

[Reilly Grant (use Gerrit), 2015/03/07 02:36:03]

Done.

+      switch (type_) {
+        case TCP:
+          client->ReleaseTcpPort(port_, interface_,
+                                 base::Bind(OnPortReleased, type_, port_,
+                                            interface_, lifeline_fd_));
+          break;
+        case UDP:
+          client->ReleaseUdpPort(port_, interface_,
+                                 base::Bind(OnPortReleased, type_, port_,
+                                            interface_, lifeline_fd_));
+          break;
+      }
+      return;
+    } else {
+      NOTREACHED() << "Could not get permission broker client.";
+    }
+  }
+
+  base::WorkerPool::PostTask(
+      FROM_HERE,
+      base::Bind(&base::DeletePointer<dbus::FileDescriptor>, lifeline_fd_),
+      false);
+}
+
+FirewallHole::FirewallHole(PortType type,
+                           uint16 port,
+                           const std::string& interface,
+                           dbus::FileDescriptor* lifeline_fd)
+    : type_(type),
+      port_(port),
+      interface_(interface),
+      opened_(false),

[pneubeck (no reviews), 2015/03/03 22:24:51]

optional:
consider using the new c++11 "Non-Static Class Member Initializers" instead in
the header, see http://chromium-cpp.appspot.com/.

[Reilly Grant (use Gerrit), 2015/03/04 01:35:50]

Done.

+      lifeline_fd_(lifeline_fd) {
+}
+
+void FirewallHole::OnLifelineCreated(dbus::FileDescriptor* lifeline_remote,
+                                     const OpenCallback& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  PermissionBrokerClient* client =
+      DBusThreadManager::Get()->GetPermissionBrokerClient();

[pneubeck (no reviews), 2015/03/03 22:24:51]

please don't rely on the singleton and pass in the client explicitly.
That way you can mock the client in a unit test and you don't have to check
!client in here.

[Reilly Grant (use Gerrit), 2015/03/04 01:35:50]

Done.

[pneubeck (no reviews), 2015/03/08 03:35:01]

Hm. Not really done but I also see the issue with the lifetime if you pass the
client explicitly and that you need some global instance that tells you whether
the client is still existent or not.

Please add at least a unit test that exercises the positive/successful flow of
creating the firewall hole.
You'll probably see then what the issues with DBusThreadManager are. (You'll
end-up initializing all Stub DBusClients, e.g. Bluetooth)

+  if (!client) {
+    NOTREACHED() << "Could not get permission broker client.";
+    callback.Run(nullptr);
+    delete this;
+    return;
+  }
+
+  switch (type_) {
+    case TCP:
+      client->RequestTcpPortAccess(
+          port_, interface_, *lifeline_remote,
+          base::Bind(&FirewallHole::OnPortAccessGranted, base::Unretained(this),

[pneubeck (no reviews), 2015/03/03 22:24:51]

are you assuming here that |this| is outliving |client| ?
that seems not to be the case looking at the NOTREACHEDs in line 126 and 99.

[Reilly Grant (use Gerrit), 2015/03/07 02:36:03]

I believe this is addressed in the latest patchset.

+                     callback));
+      break;
+    case UDP:
+      client->RequestUdpPortAccess(
+          port_, interface_, *lifeline_remote,
+          base::Bind(&FirewallHole::OnPortAccessGranted, base::Unretained(this),
+                     callback));
+      break;
+  }
+}
+
+void FirewallHole::OnPortAccessGranted(
+    const FirewallHole::OpenCallback& callback,
+    bool success) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (success) {
+    opened_ = true;
+    callback.Run(make_scoped_ptr(this));
+  } else {
+    callback.Run(nullptr);
+    delete this;

[pneubeck (no reviews), 2015/03/03 22:24:50]

delete this is... dangerous.

E.g. if client->RequestUdpPortAccess calls its callback synchronously (which
isn't unrealistic in certain error cases), you have an invalid this pointer on
the call stack after this pointer.
You would have to document that at several places.

Better not rely on a |delete this| at all.

[Reilly Grant (use Gerrit), 2015/03/04 01:35:49]

Done.

+  }
+}
+
+}  // namespace chromeos