Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When to close a passed file descriptor? #63

Closed
cvuchener opened this issue Jun 16, 2019 · 7 comments · Fixed by #69
Closed

When to close a passed file descriptor? #63

cvuchener opened this issue Jun 16, 2019 · 7 comments · Fixed by #69

Comments

@cvuchener
Copy link

I want my server to pass one end of a socket pair to a client as a return value from a method. I need to close the client side after it is sent. I had a look at sdbus::UnixFd and it does not seem to take ownership. How can I know when it is safe to close the file descriptor so only the client has it?

@cvuchener
Copy link
Author

If I understand sdbus correctly it should be safe to close it after Message& Message::operator<<(const UnixFd &item) (or sd_bus_message_append_basic)

https://www.freedesktop.org/software/systemd/man/sd_bus_message_append_basic.html

If type is "h" (UNIX file descriptor), the descriptor is duplicated by this call and the passed descriptor stays in possession of the caller.

So it should be possible with the low-level API, but not the convenience API. I would still like to use the convenience API and generated stubs for the other methods and properties. Is there a way to mix all that?

@sangelovic
Copy link
Collaborator

Hi, UnixFd was meant as a simple, thin wrapper for fd to allow sdbus-c++ type system to distinguish it from the general int type, without any additional logic.

But you're right, it is impossible to close a fd after putting it into a method reply message on server side using convenience API, because the fd is put into the message after the method handler body.

And yes, a way to solve this would be to use RAII, i.e. close the fd in UnixFd's destructor -- but only if the client wishes so. An additional boolean member of UnixFd and simple close() in the d-tor could do. For higher flexibility (maybe simple close() with ignoring the return value is too limiting?), we could even add a deleter in unique_ptr's style, but UnixFd would have to change to a class template (version with no closing the fd would be UnixFd<>).

What do you think?

@cvuchener
Copy link
Author

I was thinking about trying to add an owning UniqueUnixFd type, and creating an alias like using AnyUnixFd = std::variant<UnixFd, UniqueUnixFd>. The point of the variant is for using the same type in generated stubs, so there is no need to annotate the xml with the wanted type. I think using a template UnixFd<> won't work because of this.

Returning an integer would implicitly use the first type, using the owning fd would require calling its explicit constructor. Implementing operator<< is a simple call to std::visit. I don't think operator>> make sense for the variant, so one of the type must be chosen for receiving file descriptors (I would have a preference for the owning version for this case, since you are owning the descriptor you've just received, but both would work).

@lechndo
Copy link

lechndo commented Jun 27, 2019

Hi guys,

again an interesting topic.

What came to my mind was to make the method async on the server side but directly calling 'returnResults' to return the FD and closing the local one directly afterwards before the incoming method call ends.
Might this be a work around for now?

I also came across another issue with the FDs on the receiver side.
It seems that in Message& Message::operator>>(UnixFd &item) we also have to dup the FD because the original received FD is owned by and closed together with the low level SD-BUS C message object.

Think for that an UnixFD class that handles ownership and closes the FD if needed might also be really handy.

For testing I adapted the code to the following:

Message& Message::operator>>(UnixFd &item)e
{
    int fd = -1;
    auto r = sd_bus_message_read_basic((sd_bus_message*)msg_, SD_BUS_TYPE_UNIX_FD, &fd);
    if (r == 0)
        ok_ = false;

    SDBUS_THROW_ERROR_IF(r < 0, "Failed to deserialize a UnixFd value", -r);

    item.fd_ = ::dup(fd);

    return *this;
}

@cvuchener
Copy link
Author

I think it is best to let the user duplicate the file descriptor as needed.

I thought the received fd was owned, but I did not test anything yet. There is no mention of the file descriptor lifetime in sd_bus_message_read_basic doc, only that the strings are borrowed.

@cvuchener
Copy link
Author

I've just checked libsystemd code, file descriptors are indeed closed when the message is freed: https://github.com/systemd/systemd/blob/a4eb991831099756e9debc2cfeaf689584deed08/src/libsystemd/sd-bus/bus-message.c#L125

@lechndo
Copy link

lechndo commented Jun 27, 2019

There is no mention of the file descriptor lifetime in sd_bus_message_read_basic doc, only that the strings are borrowed.

I also couldn't find anything mentioned in the docu but in the source it is quite clear the the FDs are closed when the sd_bus_message is freed:
In static sd_bus_message* message_free(sd_bus_message *m) they do:

// File: libsystemd/sd-bus/bus-message.c
// Line: 124
if (m->free_fds) {
    close_many(m->fds, m->n_fds);
    free(m->fds);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants