Sharing file descriptor between an under-privileged and a privileged process (using C++ and Qt)

Question

This code is part of my Google Summer of Code project. It will be used in a file manager to provide file management in a root-owned directory.

As the title suggests this will be used to share file descriptor using unix local domain socket. There are two classes: FdReceiver and FdSender. FdReceiver acts as the server and will be used in the under-privileged process. FdSender is the client and will be used in the privileged/root-owned process.

Header File:

#ifndef SHAREFD_H
#define SHAREFD_H

#include <QSocketNotifier>

class FdReceiver : public QObject
{
    Q_OBJECT
public:
    FdReceiver(QObject *parent = nullptr);

    bool startListening(const QString &path);
    void stopListening();
    int fileDescriptor() const;
private:
    Q_SLOT void receiveFileDescriptor();

    QSocketNotifier *m_readNotifier;
    int m_socketDes;
    int m_fileDes;
};

class FdSender
{
public:
    FdSender();

    void connectToPath(const std::string &path);
    bool sendFileDescriptor(int fd);
    bool isConnected() const;

private:
    int m_socketDes;
    bool m_connected;
};

#endif

Source file:

#include <fcntl.h>
#include <unistd.h>
#include <sys/socket.h>
#include <sys/un.h>
#include "sharefd.h"

class KSockaddrUn
{
    sockaddr_un addr;
    int addrlen;

public:
    KSockaddrUn(const std::string &path)
    { 
        addrlen = sizeof(sockaddr_un);
        memset(&addr, 0, addrlen);
        addr.sun_family = AF_UNIX;
        std::string finalPath = "/tmp/" + path;
#ifdef __linux__
        strcpy(&addr.sun_path[1], finalPath.c_str());
#else  
        strcpy(&addr.sun_path, finalPath.c_str());
        unlink(finalPath.c_str());
#endif 
    }

    int length() const
    {
        return addrlen;
    }
    const sockaddr *address()
    {
        return reinterpret_cast<sockaddr*>(&addr);
    }
};

class KMsgHdr
{
    char io_buf[2];
    char cmsg_buf[CMSG_SPACE(sizeof(int))];
    msghdr msg;
    iovec io;

public:
    KMsgHdr()
    {
        io.iov_base = io_buf;
        io.iov_len = 2;
        msg.msg_name = NULL;
        msg.msg_namelen = 0;
        msg.msg_iov = &io;
        msg.msg_iovlen = 1;
        msg.msg_control = cmsg_buf;
        msg.msg_controllen = sizeof(cmsg_buf);
    }

    msghdr *message()
    {
        return &msg;
    } 

    cmsghdr *cmsgHeader() const
    {
        return CMSG_FIRSTHDR(&msg);
    }
};


// File descriptor reciever
FdReceiver::FdReceiver(QObject *parent)
          : QObject(parent)
          , m_socketDes(-1)
          , m_fileDes(-1)
{
}

bool FdReceiver::startListening(const QString &path)
{
    if (path.isEmpty())
        return false;

    m_socketDes = ::socket(AF_LOCAL, SOCK_STREAM | SOCK_NONBLOCK, 0);
    if (m_socketDes == -1)
        return false;

    KSockaddrUn addr(path.toStdString());
    bool bound = bind(m_socketDes, addr.address(), addr.length()) != -1;
    bool listening = listen(m_socketDes, 5) != -1;

    if (!bound || !listening) {
        ::close(m_socketDes);
        return false;
    }

    m_readNotifier = new QSocketNotifier(m_socketDes, QSocketNotifier::Read, this);
    connect(m_readNotifier, &QSocketNotifier::activated, this, &FdReceiver::receiveFileDescriptor);
    return true;
}

void FdReceiver::receiveFileDescriptor()
{
    int client = ::accept(m_socketDes, NULL, NULL);
    if (client > 0) {
        KMsgHdr msg;
        if (recvmsg(client, msg.message(), 0) == 2) {
            cmsghdr *cmsg = msg.cmsgHeader();
            memcpy(&m_fileDes, (int*)CMSG_DATA(cmsg), sizeof(int));
            m_readNotifier->setEnabled(false);
        }
        ::close(client);
    }
    ::close(m_socketDes);
}

void FdReceiver::stopListening()
{
    if (m_readNotifier->isEnabled()) {
        m_readNotifier->setEnabled(false);
        ::close(m_socketDes);
    }
}

int FdReceiver::fileDescriptor() const
{
    return m_fileDes;
}


// File descriptor sender
FdSender::FdSender()
        : m_socketDes(-1)
        , m_connected(false)
{
}

void FdSender::connectToPath(const std::string &path)
{
    if (path.empty())
        return;

    m_socketDes = ::socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
    if (m_socketDes == -1)
        return;

    KSockaddrUn addr(path);
    bool connected = ::connect(m_socketDes, addr.address(), addr.length()) == 0;

    if (!connected)
        close(m_socketDes);

    m_connected = true;
}

bool FdSender::sendFileDescriptor(int fd)
{
    KMsgHdr msg;
    cmsghdr *cmsg = msg.cmsgHeader();
    cmsg->cmsg_len = CMSG_LEN(sizeof(int));
    cmsg->cmsg_type = SCM_RIGHTS;
    cmsg->cmsg_level = SOL_SOCKET;
    memcpy((int*)CMSG_DATA(cmsg), &fd, sizeof(int));
    bool success = sendmsg(m_socketDes, msg.message(), 0) == 2;
    close(m_socketDes);
    return success;
}

bool FdSender::isConnected() const
{
    return m_connected;
}

Using FdReceiver:

    if (!file.isOpen()) {
        int src_fd = -1;
        FdReceiver fdRecv;
        bool _continue = (  fdRecv.startListening(QStringLiteral("org_kde_kio_file_helper_socket"))
                     && /*execute the root owned helper and open the file*/
                     && (src_fd = fdRecv.fileDescriptor()) != -1
                     && /*open file using src_fd*/);

        if (!_continue) {
            error();
            return;
        }
    }

Using FdSender (in helper):

bool sendFileDescriptor(int fd, const char *socketPath)
{
    FdSender fdSender;
    fdSender.connectToPath(socketPath);
    if (fdSender.isConnected()) {
        fdSender.sendFileDescriptor(fd);
        return true;
    }
    return false;
}

I would really appreciate some feedback regarding the code (both the classes and their example usage).

The coding style, class names KMsgHdr and KSockaddrUn are completely fine, in case it strikes someone as odd.

Answer 1

Headers

You don't actually need <QSocketNotifier> in the public header; a forward declaration is enough:

#include <QObject>
#include <QString>

class QSocketNotifier;

Also, a header is required for strcpy() and memcpy().

Hard-coded path

It's not obvious why the code refers to "/tmp" rather than (say) getenv("TMPDIR") - is there a good reason to ignore the usual temporary file locations?

Unnecessary member

KSockaddrUn::addrlen is never modified (so could be const), but more than that, it's always initialized to sizeof addr (cast to int), which is a compile-time constant. So it can be eliminated.

Address member can be constant

I prefer to declare addr constant, since we don't change it after construction. It does then need to be initialised in a static helper:

class KSockaddrUn
{
    const sockaddr_un addr;

public:
    KSockaddrUn(const std::string &path)
        : addr{make_address(path)}
    {
    }

    socklen_t length() const
    {
        return sizeof addr;
    }

    const sockaddr *address() const
    {
        return reinterpret_cast<const sockaddr*>(&addr);
    }

private:
    static sockaddr_un make_address(const std::string& path)
    {
        sockaddr_un a{ AF_UNIX, {0}};
        std::string finalPath = "/tmp/" + path;
#ifdef __linux__
        std::strcpy(&a.sun_path[1], finalPath.c_str());
#else
        std::strcpy(&a.sun_path, finalPath.c_str());
        ::unlink(finalPath.c_str());
#endif
        return a;
    }
};

Prefer initialisers to constructor body

It's easy to re-write KMsgHdr() to use initializers:

class KMsgHdr
{
    char io_buf[2];
    char cmsg_buf[CMSG_SPACE(sizeof(int))];
    iovec io;
    msghdr msg;

public:
    KMsgHdr()
        : io_buf{0},
          cmsg_buf{},
          io{io_buf, sizeof io_buf},
          msg{nullptr, 0,  &io, 1,  &cmsg_buf, sizeof cmsg_buf, 0}
    {
    }

    msghdr *message()
    {
        return &msg;
    }

    cmsghdr *cmsgHeader()
    {
        return CMSG_FIRSTHDR(&msg);
    }
};

I've also mode cmsgHeader() a non-const method: it's misleading to declare it const, as it gives away a writeable pointer to our internals.

A missed initializer

Avoid uninitialised pointers, even if you think you take care to avoid them:

FdReceiver::FdReceiver(QObject *parent)
    : QObject{parent}
    ,  m_readNotifier{}
    ,  m_socketDes{-1}
    ,  m_fileDes{-1}
{
}

Document the invariants

As I see it, the receiver has these states:

• Idle: socket and file descriptors negative, no notifier
• Waiting: socket descriptor set, notifier active

But when we transition from waiting back to Idle, we don't delete the notifier, and we don't close the socket. Also, we fail to clean up properly when we're destroyed, and when we re-listen (creating a new notifier; these accumulate as Qt children of this).

Perhaps we don't need an initial idle state, and we should start listening in the constructor, and only stop listening when we're destroyed. That would remove the need for the (incomplete) state machine, and prevent us leaking file descriptors or accumulating extra objects.

Here's my re-write:

class FdReceiver : public QObject
{
    Q_OBJECT
public:
    FdReceiver(const QString& path, QObject *parent = nullptr);
    ~FdReceiver();

    bool isValid() const;

    void stopListening();
    int fileDescriptor() const;
private:
    Q_SLOT void receiveFileDescriptor();

    QSocketNotifier *m_readNotifier;
    int m_socketDes;
    int m_fileDes;
};


FdReceiver::FdReceiver(const QString &path, QObject *parent)
    : QObject{parent}
    ,  m_readNotifier{}
    ,  m_socketDes{-1}
    ,  m_fileDes{-1}
{
    m_socketDes = ::socket(AF_LOCAL, SOCK_STREAM|SOCK_NONBLOCK, 0);
    if (m_socketDes == -1) {
        qWarning() << "socket:" << strerror(errno);
        return;
    }

    const KSockaddrUn addr{path.toStdString()};
    if (bind(m_socketDes, addr.address(), addr.length()) < 0 || listen(m_socketDes, 5) < 0) {
        qWarning() << "bind/listen:" << strerror(errno);
        ::close(m_socketDes);
        m_socketDes = -1;
        return;
    }

    m_readNotifier = new QSocketNotifier(m_socketDes, QSocketNotifier::Read, this);
    connect(m_readNotifier, &QSocketNotifier::activated, this, &FdReceiver::receiveFileDescriptor);
}

bool FdReceiver::isValid() const
{
    return m_socketDes >= 0;
}

void FdReceiver::receiveFileDescriptor()
{
    int client = ::accept(m_socketDes, NULL, NULL);
    if (client > 0) {
        KMsgHdr msg;
        if (::recvmsg(client, msg.message(), 0) == 2) {
            std::memcpy(&m_fileDes, CMSG_DATA(msg.cmsgHeader()), sizeof m_fileDes);
            stopListening();
        }
        ::close(client);
    }
}

void FdReceiver::stopListening()
{
    if (m_readNotifier) {
        delete m_readNotifier;
        m_readNotifier = nullptr;
    }
}

int FdReceiver::fileDescriptor() const
{
    return m_fileDes;
}

FdReceiver::~FdReceiver()
{
    if (m_socketDes >= 0)
        ::close(m_socketDes);
}

A similar change needs to happen to the sender - it needs to close its fd if its send method is never called, or to change so that it starts operation as soon as it's constructed. I'm not convinced it needs to be an object - why can't it simply be a method?