[libcamera-devel] [PATCH v2] ipa: Use FileDescriptor instead of int in layers above IPC payload
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jul 21 15:35:45 CEST 2021
Hi Paul,
Thank you for the patch.
On Wed, Jul 21, 2021 at 07:29:52PM +0900, Paul Elder wrote:
> Regarding (de)serialization in isolated IPA calls, we have four layers:
> - struct
> - byte vector + fd vector
> - IPCMessage
> - IPC payload
>
> The proxy handles the upper three layers (with help from the
> IPADataSerializer), and passes an IPCMessage to the IPC mechanism
> (implemented as an IPCPipe), which sends an IPC payload to its worker
> counterpart.
>
> When a FileDescriptor is involved, previously it was only a
> FileDescriptor in the first layer; in the lower three it was an int. To
> reduce the risk of potential fd leaks in the future, keep the
> FileDescriptor as-is throughout the upper three layers. Only the IPC
> mechanism will deal with ints, if it so wishes, when it does the actual
> IPC. IPCPipeUnixSocket does deal with ints for sending fds, so the
> conversion between IPCMessage and IPCUnixSocket::Payload converts
> between FileDescriptor and int.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> Umang, could you please check if this fixes the issue?
> ---
> .../libcamera/internal/ipa_data_serializer.h | 40 +++++-----
> include/libcamera/internal/ipc_pipe.h | 10 ++-
> src/libcamera/ipa_data_serializer.cpp | 78 +++++++++----------
> src/libcamera/ipc_pipe.cpp | 12 ++-
> .../ipa_data_serializer_test.cpp | 12 +--
> .../module_ipa_proxy.cpp.tmpl | 2 +-
> .../module_ipa_proxy.h.tmpl | 2 +-
> .../libcamera_templates/proxy_functions.tmpl | 2 +-
> .../libcamera_templates/serializer.tmpl | 22 +++---
> 9 files changed, 90 insertions(+), 90 deletions(-)
>
> diff --git a/include/libcamera/internal/ipa_data_serializer.h b/include/libcamera/internal/ipa_data_serializer.h
> index 519093bd..4353e07b 100644
> --- a/include/libcamera/internal/ipa_data_serializer.h
> +++ b/include/libcamera/internal/ipa_data_serializer.h
> @@ -66,7 +66,7 @@ template<typename T>
> class IPADataSerializer
> {
> public:
> - static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> + static std::tuple<std::vector<uint8_t>, std::vector<FileDescriptor>>
> serialize(const T &data, ControlSerializer *cs = nullptr);
>
> static T deserialize(const std::vector<uint8_t> &data,
> @@ -76,12 +76,12 @@ public:
> ControlSerializer *cs = nullptr);
>
> static T deserialize(const std::vector<uint8_t> &data,
> - const std::vector<int32_t> &fds,
> + const std::vector<FileDescriptor> &fds,
> ControlSerializer *cs = nullptr);
> static T deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> std::vector<uint8_t>::const_iterator dataEnd,
> - std::vector<int32_t>::const_iterator fdsBegin,
> - std::vector<int32_t>::const_iterator fdsEnd,
> + std::vector<FileDescriptor>::const_iterator fdsBegin,
> + std::vector<FileDescriptor>::const_iterator fdsEnd,
We could make the iterators non-const to allow moving FileDescriptor
instances (as was initially envisioned to fix the fd leak, by moving the
ints to use the FileDescriptor(int &&fd) constructor), but copying a
FileDescriptor is quite cheap due to the implicit sharing of the
internal data, so it's not a must. In any case, I don't think it should
be done as part of this patch.
> ControlSerializer *cs = nullptr);
> };
>
> @@ -104,11 +104,11 @@ template<typename V>
> class IPADataSerializer<std::vector<V>>
> {
> public:
> - static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> + static std::tuple<std::vector<uint8_t>, std::vector<FileDescriptor>>
> serialize(const std::vector<V> &data, ControlSerializer *cs = nullptr)
> {
> std::vector<uint8_t> dataVec;
> - std::vector<int32_t> fdsVec;
> + std::vector<FileDescriptor> fdsVec;
>
> /* Serialize the length. */
> uint32_t vecLen = data.size();
> @@ -117,7 +117,7 @@ public:
> /* Serialize the members. */
> for (auto const &it : data) {
> std::vector<uint8_t> dvec;
> - std::vector<int32_t> fvec;
> + std::vector<FileDescriptor> fvec;
>
> std::tie(dvec, fvec) =
> IPADataSerializer<V>::serialize(it, cs);
> @@ -141,11 +141,11 @@ public:
> std::vector<uint8_t>::const_iterator dataEnd,
> ControlSerializer *cs = nullptr)
> {
> - std::vector<int32_t> fds;
> + std::vector<FileDescriptor> fds;
> return deserialize(dataBegin, dataEnd, fds.cbegin(), fds.end(), cs);
> }
>
> - static std::vector<V> deserialize(std::vector<uint8_t> &data, std::vector<int32_t> &fds,
> + static std::vector<V> deserialize(std::vector<uint8_t> &data, std::vector<FileDescriptor> &fds,
> ControlSerializer *cs = nullptr)
> {
> return deserialize(data.cbegin(), data.end(), fds.cbegin(), fds.end(), cs);
> @@ -153,15 +153,15 @@ public:
>
> static std::vector<V> deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> std::vector<uint8_t>::const_iterator dataEnd,
> - std::vector<int32_t>::const_iterator fdsBegin,
> - [[maybe_unused]] std::vector<int32_t>::const_iterator fdsEnd,
> + std::vector<FileDescriptor>::const_iterator fdsBegin,
> + [[maybe_unused]] std::vector<FileDescriptor>::const_iterator fdsEnd,
> ControlSerializer *cs = nullptr)
> {
> uint32_t vecLen = readPOD<uint32_t>(dataBegin, 0, dataEnd);
> std::vector<V> ret(vecLen);
>
> std::vector<uint8_t>::const_iterator dataIter = dataBegin + 4;
> - std::vector<int32_t>::const_iterator fdIter = fdsBegin;
> + std::vector<FileDescriptor>::const_iterator fdIter = fdsBegin;
> for (uint32_t i = 0; i < vecLen; i++) {
> uint32_t sizeofData = readPOD<uint32_t>(dataIter, 0, dataEnd);
> uint32_t sizeofFds = readPOD<uint32_t>(dataIter, 4, dataEnd);
> @@ -201,11 +201,11 @@ template<typename K, typename V>
> class IPADataSerializer<std::map<K, V>>
> {
> public:
> - static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> + static std::tuple<std::vector<uint8_t>, std::vector<FileDescriptor>>
> serialize(const std::map<K, V> &data, ControlSerializer *cs = nullptr)
> {
> std::vector<uint8_t> dataVec;
> - std::vector<int32_t> fdsVec;
> + std::vector<FileDescriptor> fdsVec;
>
> /* Serialize the length. */
> uint32_t mapLen = data.size();
> @@ -214,7 +214,7 @@ public:
> /* Serialize the members. */
> for (auto const &it : data) {
> std::vector<uint8_t> dvec;
> - std::vector<int32_t> fvec;
> + std::vector<FileDescriptor> fvec;
>
> std::tie(dvec, fvec) =
> IPADataSerializer<K>::serialize(it.first, cs);
> @@ -247,11 +247,11 @@ public:
> std::vector<uint8_t>::const_iterator dataEnd,
> ControlSerializer *cs = nullptr)
> {
> - std::vector<int32_t> fds;
> + std::vector<FileDescriptor> fds;
> return deserialize(dataBegin, dataEnd, fds.cbegin(), fds.end(), cs);
> }
>
> - static std::map<K, V> deserialize(std::vector<uint8_t> &data, std::vector<int32_t> &fds,
> + static std::map<K, V> deserialize(std::vector<uint8_t> &data, std::vector<FileDescriptor> &fds,
> ControlSerializer *cs = nullptr)
> {
> return deserialize(data.cbegin(), data.end(), fds.cbegin(), fds.end(), cs);
> @@ -259,8 +259,8 @@ public:
>
> static std::map<K, V> deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> std::vector<uint8_t>::const_iterator dataEnd,
> - std::vector<int32_t>::const_iterator fdsBegin,
> - [[maybe_unused]] std::vector<int32_t>::const_iterator fdsEnd,
> + std::vector<FileDescriptor>::const_iterator fdsBegin,
> + [[maybe_unused]] std::vector<FileDescriptor>::const_iterator fdsEnd,
> ControlSerializer *cs = nullptr)
> {
> std::map<K, V> ret;
> @@ -268,7 +268,7 @@ public:
> uint32_t mapLen = readPOD<uint32_t>(dataBegin, 0, dataEnd);
>
> std::vector<uint8_t>::const_iterator dataIter = dataBegin + 4;
> - std::vector<int32_t>::const_iterator fdIter = fdsBegin;
> + std::vector<FileDescriptor>::const_iterator fdIter = fdsBegin;
> for (uint32_t i = 0; i < mapLen; i++) {
> uint32_t sizeofData = readPOD<uint32_t>(dataIter, 0, dataEnd);
> uint32_t sizeofFds = readPOD<uint32_t>(dataIter, 4, dataEnd);
> diff --git a/include/libcamera/internal/ipc_pipe.h b/include/libcamera/internal/ipc_pipe.h
> index e58de340..52cc8fa3 100644
> --- a/include/libcamera/internal/ipc_pipe.h
> +++ b/include/libcamera/internal/ipc_pipe.h
> @@ -11,6 +11,8 @@
>
> #include <libcamera/base/signal.h>
>
> +#include <libcamera/file_descriptor.h>
> +
> #include "libcamera/internal/ipc_unixsocket.h"
>
> namespace libcamera {
> @@ -26,23 +28,23 @@ public:
> IPCMessage();
> IPCMessage(uint32_t cmd);
> IPCMessage(const Header &header);
> - IPCMessage(const IPCUnixSocket::Payload &payload);
> + IPCMessage(IPCUnixSocket::Payload &payload);
>
> IPCUnixSocket::Payload payload() const;
>
> Header &header() { return header_; }
> std::vector<uint8_t> &data() { return data_; }
> - std::vector<int32_t> &fds() { return fds_; }
> + std::vector<FileDescriptor> &fds() { return fds_; }
>
> const Header &header() const { return header_; }
> const std::vector<uint8_t> &data() const { return data_; }
> - const std::vector<int32_t> &fds() const { return fds_; }
> + const std::vector<FileDescriptor> &fds() const { return fds_; }
>
> private:
> Header header_;
>
> std::vector<uint8_t> data_;
> - std::vector<int32_t> fds_;
> + std::vector<FileDescriptor> fds_;
> };
>
> class IPCPipe
> diff --git a/src/libcamera/ipa_data_serializer.cpp b/src/libcamera/ipa_data_serializer.cpp
> index fb941e6b..e991d751 100644
> --- a/src/libcamera/ipa_data_serializer.cpp
> +++ b/src/libcamera/ipa_data_serializer.cpp
> @@ -8,6 +8,7 @@
> #include "libcamera/internal/ipa_data_serializer.h"
>
> #include <libcamera/base/log.h>
> +#include <unistd.h>
This should go in a separate group above log.h.
>
> /**
> * \file ipa_data_serializer.h
> @@ -141,7 +142,7 @@ namespace {
> /**
> * \fn template<typename T> IPADataSerializer<T>::deserialize(
> * const std::vector<uint8_t> &data,
> - * const std::vector<int32_t> &fds,
> + * const std::vector<FileDescriptor> &fds,
> * ControlSerializer *cs = nullptr)
> * \brief Deserialize byte vector and fd vector into an object
> * \tparam T Type of object to deserialize to
> @@ -162,8 +163,8 @@ namespace {
> * \fn template<typename T> IPADataSerializer::deserialize(
> * std::vector<uint8_t>::const_iterator dataBegin,
> * std::vector<uint8_t>::const_iterator dataEnd,
> - * std::vector<int32_t>::const_iterator fdsBegin,
> - * std::vector<int32_t>::const_iterator fdsEnd,
> + * std::vector<FileDescriptor>::const_iterator fdsBegin,
> + * std::vector<FileDescriptor>::const_iterator fdsEnd,
> * ControlSerializer *cs = nullptr)
> * \brief Deserialize byte vector and fd vector into an object
> * \tparam T Type of object to deserialize to
> @@ -187,7 +188,7 @@ namespace {
> #define DEFINE_POD_SERIALIZER(type) \
> \
> template<> \
> -std::tuple<std::vector<uint8_t>, std::vector<int32_t>> \
> +std::tuple<std::vector<uint8_t>, std::vector<FileDescriptor>> \
There's now one tab too much before the \.
> IPADataSerializer<type>::serialize(const type &data, \
> [[maybe_unused]] ControlSerializer *cs) \
> { \
> @@ -215,7 +216,7 @@ type IPADataSerializer<type>::deserialize(const std::vector<uint8_t> &data, \
> \
> template<> \
> type IPADataSerializer<type>::deserialize(const std::vector<uint8_t> &data, \
> - [[maybe_unused]] const std::vector<int32_t> &fds, \
> + [[maybe_unused]] const std::vector<FileDescriptor> &fds, \
> ControlSerializer *cs) \
> { \
> return deserialize(data.cbegin(), data.end(), cs); \
> @@ -224,8 +225,8 @@ type IPADataSerializer<type>::deserialize(const std::vector<uint8_t> &data, \
> template<> \
> type IPADataSerializer<type>::deserialize(std::vector<uint8_t>::const_iterator dataBegin, \
> std::vector<uint8_t>::const_iterator dataEnd, \
> - [[maybe_unused]] std::vector<int32_t>::const_iterator fdsBegin, \
> - [[maybe_unused]] std::vector<int32_t>::const_iterator fdsEnd, \
> + [[maybe_unused]] std::vector<FileDescriptor>::const_iterator fdsBegin, \
> + [[maybe_unused]] std::vector<FileDescriptor>::const_iterator fdsEnd, \
> ControlSerializer *cs) \
> { \
> return deserialize(dataBegin, dataEnd, cs); \
> @@ -249,7 +250,7 @@ DEFINE_POD_SERIALIZER(double)
> * function parameter serdes).
> */
> template<>
> -std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> +std::tuple<std::vector<uint8_t>, std::vector<FileDescriptor>>
> IPADataSerializer<std::string>::serialize(const std::string &data,
> [[maybe_unused]] ControlSerializer *cs)
> {
> @@ -276,7 +277,7 @@ IPADataSerializer<std::string>::deserialize(std::vector<uint8_t>::const_iterator
> template<>
> std::string
> IPADataSerializer<std::string>::deserialize(const std::vector<uint8_t> &data,
> - [[maybe_unused]] const std::vector<int32_t> &fds,
> + [[maybe_unused]] const std::vector<FileDescriptor> &fds,
> [[maybe_unused]] ControlSerializer *cs)
> {
> return { data.cbegin(), data.cend() };
> @@ -286,8 +287,8 @@ template<>
> std::string
> IPADataSerializer<std::string>::deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> std::vector<uint8_t>::const_iterator dataEnd,
> - [[maybe_unused]] std::vector<int32_t>::const_iterator fdsBegin,
> - [[maybe_unused]] std::vector<int32_t>::const_iterator fdsEnd,
> + [[maybe_unused]] std::vector<FileDescriptor>::const_iterator fdsBegin,
> + [[maybe_unused]] std::vector<FileDescriptor>::const_iterator fdsEnd,
> [[maybe_unused]] ControlSerializer *cs)
> {
> return { dataBegin, dataEnd };
> @@ -305,7 +306,7 @@ IPADataSerializer<std::string>::deserialize(std::vector<uint8_t>::const_iterator
> * be used. The serialized ControlInfoMap will have zero length.
> */
> template<>
> -std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> +std::tuple<std::vector<uint8_t>, std::vector<FileDescriptor>>
> IPADataSerializer<ControlList>::serialize(const ControlList &data, ControlSerializer *cs)
> {
> if (!cs)
> @@ -405,7 +406,7 @@ IPADataSerializer<ControlList>::deserialize(const std::vector<uint8_t> &data,
> template<>
> ControlList
> IPADataSerializer<ControlList>::deserialize(const std::vector<uint8_t> &data,
> - [[maybe_unused]] const std::vector<int32_t> &fds,
> + [[maybe_unused]] const std::vector<FileDescriptor> &fds,
> ControlSerializer *cs)
> {
> return deserialize(data.cbegin(), data.end(), cs);
> @@ -415,8 +416,8 @@ template<>
> ControlList
> IPADataSerializer<ControlList>::deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> std::vector<uint8_t>::const_iterator dataEnd,
> - [[maybe_unused]] std::vector<int32_t>::const_iterator fdsBegin,
> - [[maybe_unused]] std::vector<int32_t>::const_iterator fdsEnd,
> + [[maybe_unused]] std::vector<FileDescriptor>::const_iterator fdsBegin,
> + [[maybe_unused]] std::vector<FileDescriptor>::const_iterator fdsEnd,
> ControlSerializer *cs)
> {
> return deserialize(dataBegin, dataEnd, cs);
> @@ -429,7 +430,7 @@ IPADataSerializer<ControlList>::deserialize(std::vector<uint8_t>::const_iterator
> * X bytes - Serialized ControlInfoMap (using ControlSerializer)
> */
> template<>
> -std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> +std::tuple<std::vector<uint8_t>, std::vector<FileDescriptor>>
> IPADataSerializer<ControlInfoMap>::serialize(const ControlInfoMap &map,
> ControlSerializer *cs)
> {
> @@ -491,7 +492,7 @@ IPADataSerializer<ControlInfoMap>::deserialize(const std::vector<uint8_t> &data,
> template<>
> ControlInfoMap
> IPADataSerializer<ControlInfoMap>::deserialize(const std::vector<uint8_t> &data,
> - [[maybe_unused]] const std::vector<int32_t> &fds,
> + [[maybe_unused]] const std::vector<FileDescriptor> &fds,
> ControlSerializer *cs)
> {
> return deserialize(data.cbegin(), data.end(), cs);
> @@ -501,8 +502,8 @@ template<>
> ControlInfoMap
> IPADataSerializer<ControlInfoMap>::deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> std::vector<uint8_t>::const_iterator dataEnd,
> - [[maybe_unused]] std::vector<int32_t>::const_iterator fdsBegin,
> - [[maybe_unused]] std::vector<int32_t>::const_iterator fdsEnd,
> + [[maybe_unused]] std::vector<FileDescriptor>::const_iterator fdsBegin,
> + [[maybe_unused]] std::vector<FileDescriptor>::const_iterator fdsEnd,
> ControlSerializer *cs)
> {
> return deserialize(dataBegin, dataEnd, cs);
> @@ -522,37 +523,28 @@ IPADataSerializer<ControlInfoMap>::deserialize(std::vector<uint8_t>::const_itera
> * 32-bit alignment of all serialized data
> */
> template<>
> -std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> +std::tuple<std::vector<uint8_t>, std::vector<FileDescriptor>>
> IPADataSerializer<FileDescriptor>::serialize(const FileDescriptor &data,
> [[maybe_unused]] ControlSerializer *cs)
> {
> - std::vector<uint8_t> dataVec = { data.isValid() };
> - std::vector<int32_t> fdVec;
> - if (data.isValid())
> - fdVec.push_back(data.fd());
> -
> - return { dataVec, fdVec };
> + return { {}, { data } };
As discussed on IRC, moving forward, I think we should serialize file
descriptors with an entry in the data array that would point (as an
offset) to the FileDescriptor entry in the fds vector. This would help
not making the wire protocol dependent on the serializer implementation.
We can't do so right now as the serialize() function doesn't have access
to the necessary information, so dropping the isValid() byte should be
fine for now. Could you however file a bug to record this ?
> }
>
> template<>
> -FileDescriptor IPADataSerializer<FileDescriptor>::deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> - std::vector<uint8_t>::const_iterator dataEnd,
> - std::vector<int32_t>::const_iterator fdsBegin,
> - std::vector<int32_t>::const_iterator fdsEnd,
> +FileDescriptor IPADataSerializer<FileDescriptor>::deserialize([[maybe_unused]] std::vector<uint8_t>::const_iterator dataBegin,
> + [[maybe_unused]] std::vector<uint8_t>::const_iterator dataEnd,
> + std::vector<FileDescriptor>::const_iterator fdsBegin,
> + std::vector<FileDescriptor>::const_iterator fdsEnd,
> [[maybe_unused]] ControlSerializer *cs)
> {
> - ASSERT(std::distance(dataBegin, dataEnd) >= 1);
> -
> - bool valid = !!(*dataBegin);
> -
> - ASSERT(!(valid && std::distance(fdsBegin, fdsEnd) < 1));
> + ASSERT(std::distance(fdsBegin, fdsEnd) == 1);
I don't recall if deserialize<T>() is called with [begin, end[ covering
only the fd entries needed to deserialize the type T, or with all the
remaining entries. In the latter case (which seems to be implied by the
existing code checking for >= 1), this should be
ASSERT(std::distance(fdsBegin, fdsEnd) >= 1);
>
> - return valid ? FileDescriptor(*fdsBegin) : FileDescriptor();
> + return *fdsBegin;
> }
>
> template<>
> FileDescriptor IPADataSerializer<FileDescriptor>::deserialize(const std::vector<uint8_t> &data,
> - const std::vector<int32_t> &fds,
> + const std::vector<FileDescriptor> &fds,
> [[maybe_unused]] ControlSerializer *cs)
> {
> return deserialize(data.cbegin(), data.end(), fds.cbegin(), fds.end());
> @@ -565,15 +557,15 @@ FileDescriptor IPADataSerializer<FileDescriptor>::deserialize(const std::vector<
> * 4 bytes - uint32_t Length
> */
> template<>
> -std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> +std::tuple<std::vector<uint8_t>, std::vector<FileDescriptor>>
> IPADataSerializer<FrameBuffer::Plane>::serialize(const FrameBuffer::Plane &data,
> [[maybe_unused]] ControlSerializer *cs)
> {
> std::vector<uint8_t> dataVec;
> - std::vector<int32_t> fdsVec;
> + std::vector<FileDescriptor> fdsVec;
>
> std::vector<uint8_t> fdBuf;
> - std::vector<int32_t> fdFds;
> + std::vector<FileDescriptor> fdFds;
> std::tie(fdBuf, fdFds) =
> IPADataSerializer<FileDescriptor>::serialize(data.fd);
> dataVec.insert(dataVec.end(), fdBuf.begin(), fdBuf.end());
> @@ -588,8 +580,8 @@ template<>
> FrameBuffer::Plane
> IPADataSerializer<FrameBuffer::Plane>::deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> std::vector<uint8_t>::const_iterator dataEnd,
> - std::vector<int32_t>::const_iterator fdsBegin,
> - [[maybe_unused]] std::vector<int32_t>::const_iterator fdsEnd,
> + std::vector<FileDescriptor>::const_iterator fdsBegin,
> + [[maybe_unused]] std::vector<FileDescriptor>::const_iterator fdsEnd,
> [[maybe_unused]] ControlSerializer *cs)
> {
> FrameBuffer::Plane ret;
> @@ -604,7 +596,7 @@ IPADataSerializer<FrameBuffer::Plane>::deserialize(std::vector<uint8_t>::const_i
> template<>
> FrameBuffer::Plane
> IPADataSerializer<FrameBuffer::Plane>::deserialize(const std::vector<uint8_t> &data,
> - const std::vector<int32_t> &fds,
> + const std::vector<FileDescriptor> &fds,
> ControlSerializer *cs)
> {
> return deserialize(data.cbegin(), data.end(), fds.cbegin(), fds.end(), cs);
> diff --git a/src/libcamera/ipc_pipe.cpp b/src/libcamera/ipc_pipe.cpp
> index 28e20e03..84136a82 100644
> --- a/src/libcamera/ipc_pipe.cpp
> +++ b/src/libcamera/ipc_pipe.cpp
> @@ -77,13 +77,17 @@ IPCMessage::IPCMessage(const Header &header)
> *
> * This essentially converts an IPCUnixSocket payload into an IPCMessage.
> * The header is extracted from the payload into the IPCMessage's header field.
> + *
> + * If the IPCUnixSocket payload had any valid file descriptors, then they will
> + * all be invalidated.
> */
> -IPCMessage::IPCMessage(const IPCUnixSocket::Payload &payload)
> +IPCMessage::IPCMessage(IPCUnixSocket::Payload &payload)
> {
> memcpy(&header_, payload.data.data(), sizeof(header_));
> data_ = std::vector<uint8_t>(payload.data.begin() + sizeof(header_),
> payload.data.end());
> - fds_ = payload.fds;
> + for (int32_t &fd : payload.fds)
> + fds_.push_back(FileDescriptor(std::move(fd)));
> }
>
> /**
> @@ -104,7 +108,9 @@ IPCUnixSocket::Payload IPCMessage::payload() const
>
> /* \todo Make this work without copy */
> memcpy(payload.data.data() + sizeof(Header), data_.data(), data_.size());
> - payload.fds = fds_;
> +
> + for (const FileDescriptor &fd : fds_)
> + payload.fds.push_back(fd.fd());
This is the tricky part, where the invariants are broken. If one were to
write
IPCMessage message = ...;
IPCUnixSocket::Payload payload = message.payload();
IPCMessage otherMessage{ payload };
then we would have two messages that contain file descriptor instances
referring to the same numerical fd, without knowing about each other. We
would also have issues with
IPCUnixSocket::Payload payload;
{
IPCMessage message = ...;
payload = message.payload();
}
send(payload);
as the file descriptors in the payload would get closed when message is
destroyed.
I expect the lower IPC layers to be reworked in the future, and as
payloads are only used in a small number of places where they're
directly passed to a send() function while the IPCMessage is still
valid, I think we can live with this issue for the time being. It would
however be good to document it, stating in the documentation of
IPCMessage::payload() that the returned payload is only valid during the
lifetime of the IPCMessage, and with a \todo comment in the function to
explain the problems.
>
> return payload;
> }
> diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp
> index 880bcd02..1e8f7e21 100644
> --- a/test/serialization/ipa_data_serializer_test.cpp
> +++ b/test/serialization/ipa_data_serializer_test.cpp
> @@ -53,7 +53,7 @@ template<typename T>
> int testPodSerdes(T in)
> {
> std::vector<uint8_t> buf;
> - std::vector<int32_t> fds;
> + std::vector<FileDescriptor> fds;
>
> std::tie(buf, fds) = IPADataSerializer<T>::serialize(in);
> T out = IPADataSerializer<T>::deserialize(buf, fds);
> @@ -72,7 +72,7 @@ int testVectorSerdes(const std::vector<T> &in,
> ControlSerializer *cs = nullptr)
> {
> std::vector<uint8_t> buf;
> - std::vector<int32_t> fds;
> + std::vector<FileDescriptor> fds;
>
> std::tie(buf, fds) = IPADataSerializer<std::vector<T>>::serialize(in, cs);
> std::vector<T> out = IPADataSerializer<std::vector<T>>::deserialize(buf, fds, cs);
> @@ -92,7 +92,7 @@ int testMapSerdes(const std::map<K, V> &in,
> ControlSerializer *cs = nullptr)
> {
> std::vector<uint8_t> buf;
> - std::vector<int32_t> fds;
> + std::vector<FileDescriptor> fds;
>
> std::tie(buf, fds) = IPADataSerializer<std::map<K, V>>::serialize(in, cs);
> std::map<K, V> out = IPADataSerializer<std::map<K, V>>::deserialize(buf, fds, cs);
> @@ -219,7 +219,7 @@ private:
> };
>
> std::vector<uint8_t> buf;
> - std::vector<int32_t> fds;
> + std::vector<FileDescriptor> fds;
>
> if (testVectorSerdes(vecUint8) != TestPass)
> return TestFail;
> @@ -291,7 +291,7 @@ private:
> { { "a", { 1, 2, 3 } }, { "b", { 4, 5, 6 } }, { "c", { 7, 8, 9 } } };
>
> std::vector<uint8_t> buf;
> - std::vector<int32_t> fds;
> + std::vector<FileDescriptor> fds;
>
> if (testMapSerdes(mapUintStr) != TestPass)
> return TestFail;
> @@ -359,7 +359,7 @@ private:
> std::string strEmpty = "";
>
> std::vector<uint8_t> buf;
> - std::vector<int32_t> fds;
> + std::vector<FileDescriptor> fds;
>
> if (testPodSerdes(u32min) != TestPass)
> return TestFail;
> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> index a4e008c7..aab1fffb 100644
> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl
> @@ -236,7 +236,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data)
> void {{proxy_name}}::{{method.mojom_name}}IPC(
> std::vector<uint8_t>::const_iterator data,
> size_t dataSize,
> - [[maybe_unused]] const std::vector<int32_t> &fds)
> + [[maybe_unused]] const std::vector<FileDescriptor> &fds)
> {
> {%- for param in method.parameters %}
> {{param|name}} {{param.mojom_name}};
> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> index ae168548..09e3af7d 100644
> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl
> @@ -65,7 +65,7 @@ private:
> void {{method.mojom_name}}IPC(
> std::vector<uint8_t>::const_iterator data,
> size_t dataSize,
> - const std::vector<int32_t> &fds);
> + const std::vector<FileDescriptor> &fds);
> {% endfor %}
>
> /* Helper class to invoke async functions in another thread. */
> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> index ea9cc86b..ebcd2aaa 100644
> --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl
> @@ -54,7 +54,7 @@
> {%- for param in params %}
> std::vector<uint8_t> {{param.mojom_name}}Buf;
> {%- if param|has_fd %}
> - std::vector<int32_t> {{param.mojom_name}}Fds;
> + std::vector<FileDescriptor> {{param.mojom_name}}Fds;
> std::tie({{param.mojom_name}}Buf, {{param.mojom_name}}Fds) =
> {%- else %}
> std::tie({{param.mojom_name}}Buf, std::ignore) =
> diff --git a/utils/ipc/generators/libcamera_templates/serializer.tmpl b/utils/ipc/generators/libcamera_templates/serializer.tmpl
> index d8d55807..cdd31df8 100644
> --- a/utils/ipc/generators/libcamera_templates/serializer.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/serializer.tmpl
> @@ -40,7 +40,7 @@
> retData.insert(retData.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> {%- elif field|is_fd %}
> std::vector<uint8_t> {{field.mojom_name}};
> - std::vector<int32_t> {{field.mojom_name}}Fds;
> + std::vector<FileDescriptor> {{field.mojom_name}}Fds;
> std::tie({{field.mojom_name}}, {{field.mojom_name}}Fds) =
> IPADataSerializer<{{field|name}}>::serialize(data.{{field.mojom_name}});
> retData.insert(retData.end(), {{field.mojom_name}}.begin(), {{field.mojom_name}}.end());
> @@ -58,7 +58,7 @@
> {%- elif field|is_plain_struct or field|is_array or field|is_map or field|is_str %}
> std::vector<uint8_t> {{field.mojom_name}};
> {%- if field|has_fd %}
> - std::vector<int32_t> {{field.mojom_name}}Fds;
> + std::vector<FileDescriptor> {{field.mojom_name}}Fds;
> std::tie({{field.mojom_name}}, {{field.mojom_name}}Fds) =
> {%- else %}
> std::tie({{field.mojom_name}}, std::ignore) =
> @@ -177,7 +177,7 @@
> # \a struct.
> #}
> {%- macro serializer(struct, namespace) %}
> - static std::tuple<std::vector<uint8_t>, std::vector<int32_t>>
> + static std::tuple<std::vector<uint8_t>, std::vector<FileDescriptor>>
> serialize(const {{struct|name_full}} &data,
> {%- if struct|needs_control_serializer %}
> ControlSerializer *cs)
> @@ -187,7 +187,7 @@
> {
> std::vector<uint8_t> retData;
> {%- if struct|has_fd %}
> - std::vector<int32_t> retFds;
> + std::vector<FileDescriptor> retFds;
> {%- endif %}
> {%- for field in struct.fields %}
> {{serializer_field(field, namespace, loop)}}
> @@ -210,7 +210,7 @@
> {%- macro deserializer_fd(struct, namespace) %}
> static {{struct|name_full}}
> deserialize(std::vector<uint8_t> &data,
> - std::vector<int32_t> &fds,
> + std::vector<FileDescriptor> &fds,
> {%- if struct|needs_control_serializer %}
> ControlSerializer *cs)
> {%- else %}
> @@ -224,8 +224,8 @@
> static {{struct|name_full}}
> deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> std::vector<uint8_t>::const_iterator dataEnd,
> - std::vector<int32_t>::const_iterator fdsBegin,
> - std::vector<int32_t>::const_iterator fdsEnd,
> + std::vector<FileDescriptor>::const_iterator fdsBegin,
> + std::vector<FileDescriptor>::const_iterator fdsEnd,
> {%- if struct|needs_control_serializer %}
> ControlSerializer *cs)
> {%- else %}
> @@ -234,7 +234,7 @@
> {
> {{struct|name_full}} ret;
> std::vector<uint8_t>::const_iterator m = dataBegin;
> - std::vector<int32_t>::const_iterator n = fdsBegin;
> + std::vector<FileDescriptor>::const_iterator n = fdsBegin;
>
> size_t dataSize = std::distance(dataBegin, dataEnd);
> [[maybe_unused]] size_t fdsSize = std::distance(fdsBegin, fdsEnd);
> @@ -255,7 +255,7 @@
> {%- macro deserializer_fd_simple(struct, namespace) %}
> static {{struct|name_full}}
> deserialize(std::vector<uint8_t> &data,
> - [[maybe_unused]] std::vector<int32_t> &fds,
> + [[maybe_unused]] std::vector<FileDescriptor> &fds,
> ControlSerializer *cs = nullptr)
> {
> return IPADataSerializer<{{struct|name_full}}>::deserialize(data.cbegin(), data.cend(), cs);
> @@ -264,8 +264,8 @@
> static {{struct|name_full}}
> deserialize(std::vector<uint8_t>::const_iterator dataBegin,
> std::vector<uint8_t>::const_iterator dataEnd,
> - [[maybe_unused]] std::vector<int32_t>::const_iterator fdsBegin,
> - [[maybe_unused]] std::vector<int32_t>::const_iterator fdsEnd,
> + [[maybe_unused]] std::vector<FileDescriptor>::const_iterator fdsBegin,
> + [[maybe_unused]] std::vector<FileDescriptor>::const_iterator fdsEnd,
> ControlSerializer *cs = nullptr)
> {
> return IPADataSerializer<{{struct|name_full}}>::deserialize(dataBegin, dataEnd, cs);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list