Support for D-Bus file descriptors #5992

larskarlitski · 2017-02-28T23:33:37Z

This is based on @stefwalter's initial implementation at #4883. I've rebased it, fixed compile errors, and started to implement the missing bits. Still very much work in progress, but I'm opening this pull request to start the discussion again.

This is basic support for file descriptors passed over DBus. Several APIs like to include stuff like this in their APIs.

Fixes #4380

larskarlitski · 2017-03-08T23:00:28Z

I think this is ready for a first review.

I've moved the internal channel logic into CockpitPipeChannel, because that's the one that deals with fds instead of addresses.

I've also added a small test case which opens a pipe and passes the reader to cockpit via a D-Bus message.

I've also documented that cockpit can now receive (but not sent) file descriptors over D-Bus. Do you think this warrants an extra section in the docs?

Does anyone have ideas for an actual use case that's small enough in scope to implement before 7.4?

martinpitt · 2017-03-09T08:55:40Z

src/base1/test-dbus-common.js

+                channel.onmessage = function (event, data) {
+                    assert.equal(data, 'Hello, fd');
+                    channel.close();
+                    QUnit.start();


I don't think you are supposed to call QUint.start() twice in the same test, the always() handler already does that. Or is that somehow "refcounted" and the extra QUnit.stop() above makes that necessary? This could do with a comment.

Yes, start/stop are refcounted. We need this here because the done() path kicks off another async call to wait for the first message. We still need it in always() in case the dbus call fails. I'll add a comment.

martinpitt · 2017-03-09T09:00:56Z

doc/guide/cockpit-dbus.xml

+      <varlistentry>
+        <term><code>HANDLE 'h'</code></term>
+        <listitem><para>A javascript object that describes a cockpit channel which represents the
+            passed file descriptor. Pass it to <link linkend="cockpit-channels-channel">


Could this be more specific about how its members look like?

I left it intentionally opaque, because the channel is openend via an internal id which is useless anyway. I'll add that this is always a stream channel.

Good point about the opacity, so fine as it is.

martinpitt · 2017-03-09T09:09:09Z

src/bridge/cockpitpipechannel.c

@@ -66,6 +66,21 @@ typedef struct {

 G_DEFINE_TYPE (CockpitPipeChannel, cockpit_pipe_channel, COCKPIT_TYPE_CHANNEL);

+GHashTable *internal_fds;


How does this simple hash table keep track of which fds are for reading and which for writing? Or do we only support receiving read fds for now?

Based on the discussion below, this comment is moot.

martinpitt · 2017-03-09T09:10:20Z

src/base1/test-dbus-common.js

@@ -1130,4 +1130,28 @@ function dbus_track_tests(channel_options, bus_name) {
                    });
            });
    });
+
+    QUnit.asyncTest("receive fd", function() {


This tests receiving a read fd. It would be good to also have a test for getting a write fd, unless this shouldn't be supported (for now) -- but then that should be documented.

martinpitt · 2017-03-09T09:11:51Z

src/bridge/cockpitpipechannel.c

@@ -384,6 +407,10 @@ cockpit_pipe_channel_prepare (CockpitChannel *channel)
      else
        self->pipe = cockpit_pipe_spawn ((const gchar **)argv, (const gchar **)env, dir, flags);
    }
+  else if (internal && lookup_internal_fd (internal, &fd))
+    {
+      self->pipe = cockpit_pipe_new (internal, fd, fd);


This specifies the same fd for both reading and writing, which is error prone and misleading. Unix pipes are unidirectional, and the sender and receiver need to agree which one gets passed and which one the sender keeps. This is related to my question above how we keep track of reading and writing fds.

Hm, good point. Is there any way to know if the fd we get from D-Bus is read- or write-only? The pipe is only for the test, but I guess this could happen in a practical use case as well.

The only way known to me is to check if fdopen() with a particular mode returns EINVAL or succeeds. But I guess I may have misunderstood this -- so you are saying that cockpit itself should not touch/use the fd at all, just pass it between the service and the client? That would actually make a lot of sense, I just got misled when reviewing this.

Do I understand this alright -- with this, when the client opens a cockpit.channel() on this handle, it's still the client's responsibility to know whether it can read and/or write into this? If so, then we should really have a test case that ensures that cockpit doesn't touch the fds themselves -- i. e. trying to write to a read fd throws a sensible error, and we should have a test case for the write end of a pipe only to ensure that this direction works as well.

Do I understand this alright

Yep, that's exactly it. I'll add the test cases you suggested.

martinpitt · 2017-03-09T09:17:00Z

src/bridge/cockpitpipechannel.c

+cockpit_pipe_channel_add_internal_fd (gint fd)
+{
+  /* We are not multi-threaded. Also don't make this look like normal fd numbers */
+  static guint64 unique = 911111;


It seems to me that the keys of that GHashTable are mostly arbitrary and opaque, and we don't actually need them for anything? I must say I don't like these magic numbers. Would it be possible to use it as a set (g_hash_table_add() and g_hash_table_contains() and store the fds directly as keys, and use the fd as-is in the ID? Or can it happen that an fd gets closed and reused, but the corresponding cockpit channel doesn't get closed? (That would be an internal inconsistency and most likely lead to errors anyway?)

We need a unique string to refer to the channel from cockpit. Maybe a random string that doesn't look like it could be a fd is even better?

So an fd isn't unique enough? Once a cockpit channel gets closed because the underlying fd closes, a new one must never use the same name again, even if the next passed fd has the same number than the previously closed one?

No, the fd might be closed and reused before the client calls cockpit.channel(), which would give it the wrong fd.

s/wrong fd/wrong id/ I suppose. Understood, so let's not dwell on this further -- it's just an opaque ID, so we can change it later on if we want.

martinpitt · 2017-03-09T09:18:19Z

src/bridge/cockpitpipechannel.c

+    internal_fds = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, internal_fd_free);
+
+  id = g_strdup_printf ("file-descriptor-%" G_GUINT64_FORMAT, unique++);
+  g_hash_table_replace (internal_fds, id, GINT_TO_POINTER (fd));


This comment would be moot when moving to a set, but I think you should use g_assert (g_hash_table_add ()) here -- like this we don't ever expect an ID to be already present.

I'd actually do the assertion here -- as we've discussed above, we do want the IDs to be unique even if the fds get reused, so this prevents falling into a trap in the future when someone "simplifies" the ID housekeeping.

martinpitt · 2017-03-09T09:20:13Z

src/bridge/cockpitpipechannel.c

+    {
+      g_hash_table_destroy (internal_fds);
+      internal_fds = NULL;
+    }


I'm not sure if that's worth the overhead -- if you keep adding and removing fds, you would always construct and delete the hash table. Seems easier and more efficient to just keep it around?

martinpitt · 2017-03-09T09:31:35Z

src/bridge/cockpitdbusjson.c

+      /* And only keep the last N ready for opening channels */
+      while (g_queue_get_length (context->fdids) > MAX_RECEIVED_DBUS_FDS)
+        {
+          old = (const gchar *)g_queue_pop_head (context->fdids);


It seems a bit odd to silently kill the oldest fd instead of rejecting to receive a new one and returning an error instead? This feels much harder to handle from clients than checking for an error on sending. At least this dropping should cause a warning somewhere.

Client code can decide to ignore a passed fd. So we have to keep the fd open for a while, so that the cockpit channel can be created if a client does not ignore it. The problem is when to close fds that were ignored. We decided to just keep the last N fds open.

This is why we can't warn about it, because many of the instances are just normal operation. If the limit turns out too low, the client code will get an error when it establishes the channel.

Makes sense, thanks!

martinpitt · 2017-03-09T09:39:22Z

src/bridge/cockpitdbusjson.c

+          g_signal_handler_disconnect (peer->cache, peer->update_sig);
+          g_object_run_dispose (G_OBJECT (peer->cache));
+          g_object_unref (peer->cache);
+        }


This should be a separate commit. (Not important, though)

larskarlitski · 2017-03-09T17:43:15Z

Update: implemented @martinpitt's suggestions. Thanks a lot!

martinpitt

Thanks for the updates! Looks fine to me now, and we can change the ID generation to something nicer in the future if we feel like it, as they are opaque. So, 👍 from me, but I won't merge yet as I want to give @stefwalter a chance to review this too.

dperpeet · 2017-03-10T06:49:38Z

src/common/mock-service.c

+
+  g_assert (condition == G_IO_IN);
+
+  read (fd, buffer, 100);


semaphore complains here while checking:

src/common/mock-service.c: In function ‘test_fd_pipe_readable’: src/common/mock-service.c:469:8: error: ignoring return value of ‘read’, declared with attribute warn_unused_result [-Werror=unused-result] read (fd, buffer, 100); ^

Even in a mock function it's probably good practice to check this.

Thanks for the catch. I've added a similar check like I have for write. I think we can safely assume that we always get all data at once since this is a pipe and very little data.

dperpeet · 2017-03-10T06:50:24Z

I didn't review anything besides the Semaphore error.

semaphore error was fixed

stefwalter

The main change I would suggest is transferring ownership of the fd (or duping it, if ownership is shared).

stefwalter · 2017-03-11T13:00:12Z

src/bridge/cockpitpipechannel.c

@@ -384,6 +407,10 @@ cockpit_pipe_channel_prepare (CockpitChannel *channel)
      else
        self->pipe = cockpit_pipe_spawn ((const gchar **)argv, (const gchar **)env, dir, flags);
    }
+  else if (internal && lookup_internal_fd (internal, &fd))


So the following line claims the fd, however lookup_internal_fd doesn't steal it from the hashtable or otherwise transfer ownership.

I imagine that such fd channels are open-once channels, after which the identifier becomes invalid? Or did you imagine them to be able to open many channels to the same fd? This is obviously a corner case, but either way needs to be made consistent.

I didn't bother because you wrote this

Yes, in theory multiple channels can be opened with the same unique string. This has similar properties to file descriptors used in process.

in bug #4380. Changed it to remove the channel and added a test.

Well it would have been fine if you had dup()'d it ... then the channel could have been opened more than once. But I think this makes sense, just have it be opened once.

stefwalter · 2017-03-11T13:00:25Z

src/bridge/cockpitpipechannel.c

+GHashTable *internal_fds;
+
+static gboolean
+lookup_internal_fd (const gchar *name, gint *fdp)


Arguments should be on different lines.

stefwalter · 2017-03-11T13:02:30Z

src/bridge/cockpitpipechannel.c

+  if (!internal_fds)
+    internal_fds = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, internal_fd_free);
+
+  id = g_strdup_printf ("file-descriptor-%" G_GUINT64_FORMAT, unique++);


This name is a bit misleading. For folks debugging this, they'll think that the file descriptor N is being opened. Worth choosing another name like 'internal-stream-XXX' or something like that? If you don't think so, I'm fine with keeping it as it is. Just consider it.

Changed it.

This works by replacing the file descriptor with a channel identifier and then allowing that channel to be opened as an internal stream channel. There is a maximum number of file descriptors that will be held in this state by a dbus client until those file descriptors are used. Fixes #4380

Don't attempt to free 'peer' member if it is unset.

larskarlitski · 2017-03-12T19:26:48Z

Update: address issues raised by @stefwalter. Thanks!

larskarlitski mentioned this pull request Feb 28, 2017

Support for DBus file descriptors #4883

Closed

6 tasks

larskarlitski added needswork bot labels Feb 28, 2017

larskarlitski changed the title ~~WIP: Support for D-Bus file descriptors~~ Support for D-Bus file descriptors Mar 8, 2017

larskarlitski removed the needswork label Mar 8, 2017

larskarlitski requested a review from stefwalter March 9, 2017 08:07

martinpitt requested changes Mar 9, 2017

View reviewed changes

martinpitt approved these changes Mar 9, 2017

View reviewed changes

martinpitt removed the bot label Mar 9, 2017

dperpeet previously requested changes Mar 10, 2017

View reviewed changes

stefwalter suggested changes Mar 11, 2017

View reviewed changes

stefwalter added the needswork label Mar 11, 2017

larskarlitski and others added 5 commits March 12, 2017 20:25

bridge: Add internal pipe channels based on fds

02cacef

bridge: fix possible crash in CockpitDBusJson

a6c5436

Don't attempt to free 'peer' member if it is unset.

doc: Document D-Bus fd passing

a5dc7d1

bridge: Test fd passing

c90f4d7

larskarlitski removed the needswork label Mar 12, 2017

stefwalter approved these changes Mar 12, 2017

View reviewed changes

martinpitt closed this in 208190d Mar 13, 2017

larskarlitski deleted the dbus-file-descriptor branch March 13, 2017 08:24

		@@ -66,6 +66,21 @@ typedef struct {

		G_DEFINE_TYPE (CockpitPipeChannel, cockpit_pipe_channel, COCKPIT_TYPE_CHANNEL);

		GHashTable *internal_fds;

Support for D-Bus file descriptors #5992

Support for D-Bus file descriptors #5992

Conversation

larskarlitski commented Feb 28, 2017 • edited Loading

larskarlitski commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larskarlitski commented Mar 9, 2017

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dperpeet commented Mar 10, 2017

stefwalter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larskarlitski commented Mar 12, 2017

larskarlitski commented Feb 28, 2017 •

edited

Loading