New issue
Advanced search Search tips

Issue 621841 link

Starred by 3 users
Status: Verified
Owner:
hashimoto@chromium.org
Closed: Sep 2016
Cc:
jorgelo@chromium.org
steve...@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Remove dbus::FileDescriptor

Project Member Reported by hashimoto@chromium.org, Jun 21 2016 
dbus::FileDescriptor has an awkward API which requires all users to call dbus::FileDescriptor::CheckValidity() on an IO-allowed thread.

As stated in https://codereview.chromium.org/9700072#msg5, IIUC this check is not needed as renderer processes don't use D-Bus at all.

We should remove dbus::FileDescriptor by replacing it with base::ScopedFD.

Comment 1 by hashimoto@chromium.org, Jun 21 2016 

Even if we are to reject directory FDs, it should be done by dbus::Bus::Send and its relatives before sending messages with dbus_connection_send.

Comment 2 by hashimoto@chromium.org, Aug 22 2016

Status: Started (was: Assigned)

Comment 3 by satorux@chromium.org, Aug 22 2016

Cc: jorgelo@chromium.org
IIRC, what jorgelo@ suggested was that Chrome should not receive/send directory FDs through D-Bus, just like Chrome IPC prohibits passing of directory FDs. If that's the case, I think we need to keep the CheckValidity() logic. jorgelo@, what do you think?

Comment 4 by hashimoto@chromium.org, Aug 22 2016 

In the review mentioned above, jorgelo@ said "The worry here is that a renderer might get a dir fd. If that cannot happen, then there might not be a
need for this."
Because that cannot happen (i.e. renderer processes don't use D-Bus) there is no need to have this logic.

Comment 5 by jorgelo@chromium.org, Aug 22 2016 

Wow, that's an old CL. I think the comment still stands though: we need to prevent directory file descriptors from getting to the renderers. Now, hashimoto@ correctly points out that renderers don't use D-Bus -- and they can't, because that would allow them to talk to a bunch of other system daemons, which would defeat the purpose of the Chrome sandbox as much as leaking a directory fd would.

So, there's no need to do the directory fd check in D-Bus for this specific case. I would argue that since Chrome's D-Bus wrappers can be used in other contexts (they're used by Chrome OS system daemons for example), there might be a reason to check whether passed fd's are directories or not -- a system daemon might also be chrooted. But specifically for the concerns of Chrome renderers, directory fd checks are not needed.

Comment 6 by satorux@chromium.org, Aug 23 2016 

jorgelo@, thank you for the comment.
hashimoto@, let's get rid of the logic! I'll have a look at the patch.
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 24 2016 

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

commit 89822579003fc4f5128c58f9bbd1a22b347ea229
Author: hashimoto <hashimoto@chromium.org>
Date: Wed Aug 24 16:51:38 2016

No dbus::FileDescriptor in DebugDaemonClient

This is the first step to replace dbus::FileDescriptor with base::ScopedFD.

Add a new variant of dbus::MessageWriter::AppendFileDescriptor and dbus::MessageReader::PopFileDescriptor.
Use the new variant in DebugDaemonClient.

BUG= 621841 

Review-Url: https://codereview.chromium.org/2081153002
Cr-Commit-Position: refs/heads/master@{#414093}

[modify] https://crrev.com/89822579003fc4f5128c58f9bbd1a22b347ea229/chrome/browser/chromeos/system_logs/debug_log_writer.cc
[modify] https://crrev.com/89822579003fc4f5128c58f9bbd1a22b347ea229/chrome/browser/extensions/api/log_private/log_private_apitest_chromeos.cc
[modify] https://crrev.com/89822579003fc4f5128c58f9bbd1a22b347ea229/chrome/browser/metrics/perf/perf_output.cc
[modify] https://crrev.com/89822579003fc4f5128c58f9bbd1a22b347ea229/chrome/browser/metrics/perf/perf_output.h
[modify] https://crrev.com/89822579003fc4f5128c58f9bbd1a22b347ea229/chromeos/dbus/debug_daemon_client.cc
[modify] https://crrev.com/89822579003fc4f5128c58f9bbd1a22b347ea229/chromeos/dbus/debug_daemon_client.h
[modify] https://crrev.com/89822579003fc4f5128c58f9bbd1a22b347ea229/chromeos/dbus/fake_debug_daemon_client.cc
[modify] https://crrev.com/89822579003fc4f5128c58f9bbd1a22b347ea229/chromeos/dbus/fake_debug_daemon_client.h
[modify] https://crrev.com/89822579003fc4f5128c58f9bbd1a22b347ea229/chromeos/dbus/lorgnette_manager_client.cc
[modify] https://crrev.com/89822579003fc4f5128c58f9bbd1a22b347ea229/chromeos/dbus/pipe_reader.cc
[modify] https://crrev.com/89822579003fc4f5128c58f9bbd1a22b347ea229/chromeos/dbus/pipe_reader.h
[modify] https://crrev.com/89822579003fc4f5128c58f9bbd1a22b347ea229/dbus/message.cc
[modify] https://crrev.com/89822579003fc4f5128c58f9bbd1a22b347ea229/dbus/message.h
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 1 2016 

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

commit 1568856ab34e9581e2a7e0cf86116d3c23d0640e
Author: hashimoto <hashimoto@chromium.org>
Date: Thu Sep 01 02:07:10 2016

chromeos: Remove dbus::FileDescriptor from PermissionBrokerClient

Replace dbus::FileDescriptor with base::ScopedFD

BUG= 621841 

Review-Url: https://codereview.chromium.org/2291983002
Cr-Commit-Position: refs/heads/master@{#415847}

[modify] https://crrev.com/1568856ab34e9581e2a7e0cf86116d3c23d0640e/chromeos/dbus/fake_permission_broker_client.cc
[modify] https://crrev.com/1568856ab34e9581e2a7e0cf86116d3c23d0640e/chromeos/dbus/fake_permission_broker_client.h
[modify] https://crrev.com/1568856ab34e9581e2a7e0cf86116d3c23d0640e/chromeos/dbus/mock_permission_broker_client.h
[modify] https://crrev.com/1568856ab34e9581e2a7e0cf86116d3c23d0640e/chromeos/dbus/permission_broker_client.cc
[modify] https://crrev.com/1568856ab34e9581e2a7e0cf86116d3c23d0640e/chromeos/dbus/permission_broker_client.h
[modify] https://crrev.com/1568856ab34e9581e2a7e0cf86116d3c23d0640e/chromeos/network/firewall_hole.cc
[modify] https://crrev.com/1568856ab34e9581e2a7e0cf86116d3c23d0640e/chromeos/network/firewall_hole.h
[modify] https://crrev.com/1568856ab34e9581e2a7e0cf86116d3c23d0640e/device/hid/hid_service_linux.cc
[modify] https://crrev.com/1568856ab34e9581e2a7e0cf86116d3c23d0640e/device/hid/hid_service_linux.h
[modify] https://crrev.com/1568856ab34e9581e2a7e0cf86116d3c23d0640e/device/serial/serial_io_handler.cc
[modify] https://crrev.com/1568856ab34e9581e2a7e0cf86116d3c23d0640e/device/serial/serial_io_handler.h
[modify] https://crrev.com/1568856ab34e9581e2a7e0cf86116d3c23d0640e/device/usb/usb_device_linux.cc
[modify] https://crrev.com/1568856ab34e9581e2a7e0cf86116d3c23d0640e/device/usb/usb_device_linux.h
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 6 2016 

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

commit 55cfa99f538429f875943ca54d3673a7394dc6e1
Author: hashimoto <hashimoto@chromium.org>
Date: Tue Sep 06 07:33:57 2016

dbus: Remove dbus::FileDescriptor from MediaTransferProtocolDaemonClient

BUG= 621841 

Review-Url: https://codereview.chromium.org/2309973002
Cr-Commit-Position: refs/heads/master@{#416602}

[modify] https://crrev.com/55cfa99f538429f875943ca54d3673a7394dc6e1/device/media_transfer_protocol/media_transfer_protocol_daemon_client.cc
Project Member

Comment 12 by bugdroid1@chromium.org, Sep 8 2016 

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

commit f64c34ead6bf89adcd3c16cc203add84be4323be
Author: hashimoto <hashimoto@chromium.org>
Date: Thu Sep 08 07:25:05 2016

dbus: No dbus::FileDescriptor in BluetoothProfileServiceProvider

Replace dbus::FileDescriptor with base::ScopedFD.

BUG= 621841 

Review-Url: https://codereview.chromium.org/2310883003
Cr-Commit-Position: refs/heads/master@{#417219}

[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/bluez/bluetooth_adapter_profile_bluez.cc
[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/bluez/bluetooth_adapter_profile_bluez.h
[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/bluez/bluetooth_adapter_profile_bluez_unittest.cc
[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/bluez/bluetooth_bluez_unittest.cc
[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/bluez/bluetooth_socket_bluez.cc
[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/bluez/bluetooth_socket_bluez.h
[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/dbus/bluetooth_profile_service_provider.cc
[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/dbus/bluetooth_profile_service_provider.h
[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/dbus/fake_bluetooth_device_client.cc
[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/dbus/fake_bluetooth_profile_service_provider.cc
[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/dbus/fake_bluetooth_profile_service_provider.h
Project Member

Comment 14 by bugdroid1@chromium.org, Sep 8 2016 

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

commit f64c34ead6bf89adcd3c16cc203add84be4323be
Author: hashimoto <hashimoto@chromium.org>
Date: Thu Sep 08 07:25:05 2016

dbus: No dbus::FileDescriptor in BluetoothProfileServiceProvider

Replace dbus::FileDescriptor with base::ScopedFD.

BUG= 621841 

Review-Url: https://codereview.chromium.org/2310883003
Cr-Commit-Position: refs/heads/master@{#417219}

[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/bluez/bluetooth_adapter_profile_bluez.cc
[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/bluez/bluetooth_adapter_profile_bluez.h
[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/bluez/bluetooth_adapter_profile_bluez_unittest.cc
[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/bluez/bluetooth_bluez_unittest.cc
[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/bluez/bluetooth_socket_bluez.cc
[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/bluez/bluetooth_socket_bluez.h
[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/dbus/bluetooth_profile_service_provider.cc
[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/dbus/bluetooth_profile_service_provider.h
[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/dbus/fake_bluetooth_device_client.cc
[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/dbus/fake_bluetooth_profile_service_provider.cc
[modify] https://crrev.com/f64c34ead6bf89adcd3c16cc203add84be4323be/device/bluetooth/dbus/fake_bluetooth_profile_service_provider.h
Project Member

Comment 15 by bugdroid1@chromium.org, Sep 13 2016 

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

commit 17c02a91835794ad2810c5d64cdd9007fa50efe5
Author: hashimoto <hashimoto@chromium.org>
Date: Tue Sep 13 08:17:38 2016

chromeos: Remove dbus::FileDescriptor from SessionManagerClient

Replace dbus::FileDescriptor with base::ScopedFD.
Move socket related code to ChromeRestartRequest to make SessionManagerClient a simple D-Bus binding class.

BUG= 621841 
TEST="Browse as Guest" on login screen works.

Review-Url: https://codereview.chromium.org/2310823003
Cr-Commit-Position: refs/heads/master@{#418194}

[modify] https://crrev.com/17c02a91835794ad2810c5d64cdd9007fa50efe5/chrome/browser/chromeos/login/chrome_restart_request.cc
[modify] https://crrev.com/17c02a91835794ad2810c5d64cdd9007fa50efe5/chrome/browser/chromeos/settings/device_settings_test_helper.cc
[modify] https://crrev.com/17c02a91835794ad2810c5d64cdd9007fa50efe5/chrome/browser/chromeos/settings/device_settings_test_helper.h
[modify] https://crrev.com/17c02a91835794ad2810c5d64cdd9007fa50efe5/chromeos/dbus/fake_session_manager_client.cc
[modify] https://crrev.com/17c02a91835794ad2810c5d64cdd9007fa50efe5/chromeos/dbus/fake_session_manager_client.h
[modify] https://crrev.com/17c02a91835794ad2810c5d64cdd9007fa50efe5/chromeos/dbus/mock_session_manager_client.h
[modify] https://crrev.com/17c02a91835794ad2810c5d64cdd9007fa50efe5/chromeos/dbus/session_manager_client.cc
[modify] https://crrev.com/17c02a91835794ad2810c5d64cdd9007fa50efe5/chromeos/dbus/session_manager_client.h
Project Member

Comment 16 by bugdroid1@chromium.org, Sep 14 2016 

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

commit dc693afc916d0596e07a0529f162516475c466f3
Author: hashimoto <hashimoto@chromium.org>
Date: Wed Sep 14 04:05:02 2016

dbus: Remove dbus::FileDescriptor

BUG= 621841 
TEST=dbus_unittests
TBR=steel@chromium.org for device/bluetooth/dbus

Review-Url: https://codereview.chromium.org/2337893002
Cr-Commit-Position: refs/heads/master@{#418481}

[modify] https://crrev.com/dc693afc916d0596e07a0529f162516475c466f3/chromeos/dbus/fake_permission_broker_client.cc
[modify] https://crrev.com/dc693afc916d0596e07a0529f162516475c466f3/chromeos/dbus/mock_permission_broker_client.cc
[modify] https://crrev.com/dc693afc916d0596e07a0529f162516475c466f3/chromeos/network/firewall_hole_unittest.cc
[modify] https://crrev.com/dc693afc916d0596e07a0529f162516475c466f3/dbus/BUILD.gn
[delete] https://crrev.com/40f77bbc12f69d345396d96032f635cfa8640ffd/dbus/file_descriptor.cc
[delete] https://crrev.com/40f77bbc12f69d345396d96032f635cfa8640ffd/dbus/file_descriptor.h
[modify] https://crrev.com/dc693afc916d0596e07a0529f162516475c466f3/dbus/message.cc
[modify] https://crrev.com/dc693afc916d0596e07a0529f162516475c466f3/dbus/message.h
[modify] https://crrev.com/dc693afc916d0596e07a0529f162516475c466f3/dbus/message_unittest.cc
[modify] https://crrev.com/dc693afc916d0596e07a0529f162516475c466f3/device/bluetooth/dbus/bluetooth_le_advertisement_service_provider.h
[modify] https://crrev.com/dc693afc916d0596e07a0529f162516475c466f3/device/bluetooth/dbus/fake_bluetooth_le_advertisement_service_provider.h
[modify] https://crrev.com/dc693afc916d0596e07a0529f162516475c466f3/device/bluetooth/dbus/fake_bluetooth_media_transport_client.cc

Comment 17 by hashimoto@chromium.org, Sep 14 2016

Status: Fixed (was: Started)

Comment 18 by dchan@chromium.org, Oct 7 2016

Labels: VerifyIn-55

Comment 19 by dchan@google.com, Nov 19 2016

Labels: VerifyIn-56

Comment 20 by dchan@google.com, Jan 21 2017

Labels: VerifyIn-57

Comment 21 by dchan@google.com, Mar 4 2017

Labels: VerifyIn-58

Comment 22 by dchan@google.com, Apr 17 2017

Labels: VerifyIn-59

Comment 23 by dchan@google.com, May 30 2017

Labels: VerifyIn-60

Comment 24 by dchan@chromium.org, Aug 1 2017

Labels: VerifyIn-61

Comment 25 by sontis@chromium.org, Aug 9 2017

Status: Verified (was: Fixed)
Verified.
►

Sign in to add a comment