3
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ Thanks Toby for the review. I have taken into account most of your suggestions and changed my code accordingly. \$\endgroup\$ – Chinmoy Aug 20 '17 at 14:36
  • \$\begingroup\$ Two things I didn't changed 1> hard-coded path because this code will run on *nix and there are all sort of problems with using environment variable. QDir::tmpDir() is an option but the code will run as root so I didn't used it. 2> class->method. Honestly I don't have a valid reason as to why FdSender class is there and not just a method. It was instinctive. I just believe FdReceiver must have a FdSender. Apart from that no solid reason (even if there is one I am not aware of it). \$\endgroup\$ – Chinmoy Aug 20 '17 at 14:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.