Message ID | 20210817044633.2061596-1-paul.elder@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Paul, On 8/17/21 10:16 AM, 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. > > Additionally, change the data portion of the serialized form of > FileDescriptor to a 32-bit unsigned integer, for alightnment purposes > and in preparation for conversion to an index into the fd array. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> The patch passes the Fd leak test v1 so: Tested-by: Umang Jain <umang.jain@ideasonboard.com> > > --- > Changes in v3: > - encode isValid as a uint32 > --- > .../libcamera/internal/ipa_data_serializer.h | 40 ++++----- > include/libcamera/internal/ipc_pipe.h | 10 ++- > src/libcamera/ipa_data_serializer.cpp | 86 +++++++++++-------- > 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 | 26 +++--- > 9 files changed, 105 insertions(+), 87 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, > 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..91471f52 100644 > --- a/src/libcamera/ipa_data_serializer.cpp > +++ b/src/libcamera/ipa_data_serializer.cpp > @@ -7,6 +7,8 @@ > > #include "libcamera/internal/ipa_data_serializer.h" > > +#include <unistd.h> > + > #include <libcamera/base/log.h> > > /** > @@ -141,7 +143,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 +164,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 +189,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>> \ > IPADataSerializer<type>::serialize(const type &data, \ > [[maybe_unused]] ControlSerializer *cs) \ > { \ > @@ -215,7 +217,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 +226,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 +251,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 +278,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 +288,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 +307,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 +407,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 +417,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 +431,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 +493,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 +503,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 +524,45 @@ 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; > + std::vector<uint8_t> dataVec; > + std::vector<FileDescriptor> fdVec; > + > + /* > + * Store as uint32_t to prepare for conversion from validity flag > + * to index, and for alignment > + */ > + appendPOD<uint32_t>(dataVec, data.isValid()); > + > if (data.isValid()) > - fdVec.push_back(data.fd()); > + fdVec.push_back(data); > + > > return { dataVec, fdVec }; > } > > 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); > + ASSERT(std::distance(dataBegin, dataEnd) >= 4); > > - bool valid = !!(*dataBegin); > + uint32_t valid = readPOD<uint32_t>(dataBegin, 0, dataEnd); > > ASSERT(!(valid && std::distance(fdsBegin, fdsEnd) < 1)); > > - return valid ? FileDescriptor(*fdsBegin) : FileDescriptor(); > + return valid ? *fdsBegin : FileDescriptor(); > } > > 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 +575,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,13 +598,13 @@ 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; > > - ret.fd = IPADataSerializer<FileDescriptor>::deserialize(dataBegin, dataBegin + 1, > + ret.fd = IPADataSerializer<FileDescriptor>::deserialize(dataBegin, dataBegin + 4, > fdsBegin, fdsBegin + 1); > ret.length = readPOD<uint32_t>(dataBegin, 1, dataEnd); > > @@ -604,7 +614,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()); > > return payload; > } > diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp > index bf7e34e2..eca19a66 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 c222f5f2..1979e68f 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..b8ef8e7b 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) = > @@ -104,9 +104,9 @@ > dataSize -= {{field_size}}; > {%- endif %} > {% elif field|is_fd %} > - {%- set field_size = 1 %} > + {%- set field_size = 4 %} > {{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data')}} > - ret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + 1, n, n + 1, cs); > + ret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}}, n, n + 1, cs); > {%- if not loop.last %} > m += {{field_size}}; > dataSize -= {{field_size}}; > @@ -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);
Hi Paul, Thank you for the patch. On Tue, Aug 17, 2021 at 01:46:33PM +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. > > Additionally, change the data portion of the serialized form of > FileDescriptor to a 32-bit unsigned integer, for alightnment purposes > and in preparation for conversion to an index into the fd array. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > Changes in v3: > - encode isValid as a uint32 > --- > .../libcamera/internal/ipa_data_serializer.h | 40 ++++----- > include/libcamera/internal/ipc_pipe.h | 10 ++- > src/libcamera/ipa_data_serializer.cpp | 86 +++++++++++-------- > 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 | 26 +++--- > 9 files changed, 105 insertions(+), 87 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, > 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..91471f52 100644 > --- a/src/libcamera/ipa_data_serializer.cpp > +++ b/src/libcamera/ipa_data_serializer.cpp > @@ -7,6 +7,8 @@ > > #include "libcamera/internal/ipa_data_serializer.h" > > +#include <unistd.h> > + > #include <libcamera/base/log.h> > > /** > @@ -141,7 +143,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 +164,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 +189,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>> \ > IPADataSerializer<type>::serialize(const type &data, \ > [[maybe_unused]] ControlSerializer *cs) \ > { \ > @@ -215,7 +217,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 +226,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 +251,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 +278,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 +288,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 +307,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 +407,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 +417,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 +431,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 +493,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 +503,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 +524,45 @@ 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; > + std::vector<uint8_t> dataVec; > + std::vector<FileDescriptor> fdVec; > + > + /* > + * Store as uint32_t to prepare for conversion from validity flag > + * to index, and for alignment s/alignment/alignment./ Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + */ > + appendPOD<uint32_t>(dataVec, data.isValid()); > + > if (data.isValid()) > - fdVec.push_back(data.fd()); > + fdVec.push_back(data); > + > > return { dataVec, fdVec }; > } > > 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); > + ASSERT(std::distance(dataBegin, dataEnd) >= 4); > > - bool valid = !!(*dataBegin); > + uint32_t valid = readPOD<uint32_t>(dataBegin, 0, dataEnd); > > ASSERT(!(valid && std::distance(fdsBegin, fdsEnd) < 1)); > > - return valid ? FileDescriptor(*fdsBegin) : FileDescriptor(); > + return valid ? *fdsBegin : FileDescriptor(); > } > > 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 +575,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,13 +598,13 @@ 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; > > - ret.fd = IPADataSerializer<FileDescriptor>::deserialize(dataBegin, dataBegin + 1, > + ret.fd = IPADataSerializer<FileDescriptor>::deserialize(dataBegin, dataBegin + 4, > fdsBegin, fdsBegin + 1); > ret.length = readPOD<uint32_t>(dataBegin, 1, dataEnd); > > @@ -604,7 +614,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()); > > return payload; > } > diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp > index bf7e34e2..eca19a66 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 c222f5f2..1979e68f 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..b8ef8e7b 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) = > @@ -104,9 +104,9 @@ > dataSize -= {{field_size}}; > {%- endif %} > {% elif field|is_fd %} > - {%- set field_size = 1 %} > + {%- set field_size = 4 %} > {{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data')}} > - ret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + 1, n, n + 1, cs); > + ret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}}, n, n + 1, cs); > {%- if not loop.last %} > m += {{field_size}}; > dataSize -= {{field_size}}; > @@ -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);
Hi Paul, Another comment. On Tue, Aug 17, 2021 at 03:49:18PM +0300, Laurent Pinchart wrote: > On Tue, Aug 17, 2021 at 01:46:33PM +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. > > > > Additionally, change the data portion of the serialized form of > > FileDescriptor to a 32-bit unsigned integer, for alightnment purposes > > and in preparation for conversion to an index into the fd array. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > --- > > Changes in v3: > > - encode isValid as a uint32 > > --- > > .../libcamera/internal/ipa_data_serializer.h | 40 ++++----- > > include/libcamera/internal/ipc_pipe.h | 10 ++- > > src/libcamera/ipa_data_serializer.cpp | 86 +++++++++++-------- > > 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 | 26 +++--- > > 9 files changed, 105 insertions(+), 87 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, > > 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..91471f52 100644 > > --- a/src/libcamera/ipa_data_serializer.cpp > > +++ b/src/libcamera/ipa_data_serializer.cpp > > @@ -7,6 +7,8 @@ > > > > #include "libcamera/internal/ipa_data_serializer.h" > > > > +#include <unistd.h> > > + > > #include <libcamera/base/log.h> > > > > /** > > @@ -141,7 +143,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 +164,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 +189,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>> \ > > IPADataSerializer<type>::serialize(const type &data, \ > > [[maybe_unused]] ControlSerializer *cs) \ > > { \ > > @@ -215,7 +217,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 +226,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 +251,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 +278,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 +288,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 +307,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 +407,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 +417,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 +431,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 +493,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 +503,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 +524,45 @@ 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; > > + std::vector<uint8_t> dataVec; > > + std::vector<FileDescriptor> fdVec; > > + > > + /* > > + * Store as uint32_t to prepare for conversion from validity flag > > + * to index, and for alignment > > s/alignment/alignment./ > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + */ > > + appendPOD<uint32_t>(dataVec, data.isValid()); > > + > > if (data.isValid()) > > - fdVec.push_back(data.fd()); > > + fdVec.push_back(data); > > + > > > > return { dataVec, fdVec }; > > } > > > > 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); > > + ASSERT(std::distance(dataBegin, dataEnd) >= 4); > > > > - bool valid = !!(*dataBegin); > > + uint32_t valid = readPOD<uint32_t>(dataBegin, 0, dataEnd); > > > > ASSERT(!(valid && std::distance(fdsBegin, fdsEnd) < 1)); > > > > - return valid ? FileDescriptor(*fdsBegin) : FileDescriptor(); > > + return valid ? *fdsBegin : FileDescriptor(); > > } > > > > 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 +575,15 @@ FileDescriptor IPADataSerializer<FileDescriptor>::deserialize(const std::vector< The comment here mentions 1 byte for the file descriptor, while it's now 4 bytes. Maybe the comment could be dropped altogether, up to you. > > * 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,13 +598,13 @@ 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; > > > > - ret.fd = IPADataSerializer<FileDescriptor>::deserialize(dataBegin, dataBegin + 1, > > + ret.fd = IPADataSerializer<FileDescriptor>::deserialize(dataBegin, dataBegin + 4, > > fdsBegin, fdsBegin + 1); > > ret.length = readPOD<uint32_t>(dataBegin, 1, dataEnd); > > > > @@ -604,7 +614,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()); > > > > return payload; > > } > > diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp > > index bf7e34e2..eca19a66 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 c222f5f2..1979e68f 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..b8ef8e7b 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) = > > @@ -104,9 +104,9 @@ > > dataSize -= {{field_size}}; > > {%- endif %} > > {% elif field|is_fd %} > > - {%- set field_size = 1 %} > > + {%- set field_size = 4 %} > > {{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data')}} > > - ret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + 1, n, n + 1, cs); > > + ret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}}, n, n + 1, cs); > > {%- if not loop.last %} > > m += {{field_size}}; > > dataSize -= {{field_size}}; > > @@ -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);
Hi Paul, On 8/17/21 10:16 AM, 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. > > Additionally, change the data portion of the serialized form of > FileDescriptor to a 32-bit unsigned integer, for alightnment purposes > and in preparation for conversion to an index into the fd array. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> Do you any preferences of merging this? I plan to merge the fd-leak test sometime today (so it'll fail / reproducing the issue), so this fix can be applied over it. I don't want a failing test lingering in master for too long, so I think merging this patch after the test is in, should be okay? Any thoughts? I can handle all the merging if you want that, as well :-) > > --- > Changes in v3: > - encode isValid as a uint32 > --- > .../libcamera/internal/ipa_data_serializer.h | 40 ++++----- > include/libcamera/internal/ipc_pipe.h | 10 ++- > src/libcamera/ipa_data_serializer.cpp | 86 +++++++++++-------- > 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 | 26 +++--- > 9 files changed, 105 insertions(+), 87 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, > 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..91471f52 100644 > --- a/src/libcamera/ipa_data_serializer.cpp > +++ b/src/libcamera/ipa_data_serializer.cpp > @@ -7,6 +7,8 @@ > > #include "libcamera/internal/ipa_data_serializer.h" > > +#include <unistd.h> > + > #include <libcamera/base/log.h> > > /** > @@ -141,7 +143,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 +164,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 +189,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>> \ > IPADataSerializer<type>::serialize(const type &data, \ > [[maybe_unused]] ControlSerializer *cs) \ > { \ > @@ -215,7 +217,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 +226,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 +251,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 +278,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 +288,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 +307,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 +407,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 +417,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 +431,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 +493,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 +503,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 +524,45 @@ 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; > + std::vector<uint8_t> dataVec; > + std::vector<FileDescriptor> fdVec; > + > + /* > + * Store as uint32_t to prepare for conversion from validity flag > + * to index, and for alignment > + */ > + appendPOD<uint32_t>(dataVec, data.isValid()); > + > if (data.isValid()) > - fdVec.push_back(data.fd()); > + fdVec.push_back(data); > + > > return { dataVec, fdVec }; > } > > 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); > + ASSERT(std::distance(dataBegin, dataEnd) >= 4); > > - bool valid = !!(*dataBegin); > + uint32_t valid = readPOD<uint32_t>(dataBegin, 0, dataEnd); > > ASSERT(!(valid && std::distance(fdsBegin, fdsEnd) < 1)); > > - return valid ? FileDescriptor(*fdsBegin) : FileDescriptor(); > + return valid ? *fdsBegin : FileDescriptor(); > } > > 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 +575,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,13 +598,13 @@ 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; > > - ret.fd = IPADataSerializer<FileDescriptor>::deserialize(dataBegin, dataBegin + 1, > + ret.fd = IPADataSerializer<FileDescriptor>::deserialize(dataBegin, dataBegin + 4, > fdsBegin, fdsBegin + 1); > ret.length = readPOD<uint32_t>(dataBegin, 1, dataEnd); > > @@ -604,7 +614,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()); > > return payload; > } > diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp > index bf7e34e2..eca19a66 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 c222f5f2..1979e68f 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..b8ef8e7b 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) = > @@ -104,9 +104,9 @@ > dataSize -= {{field_size}}; > {%- endif %} > {% elif field|is_fd %} > - {%- set field_size = 1 %} > + {%- set field_size = 4 %} > {{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data')}} > - ret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + 1, n, n + 1, cs); > + ret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}}, n, n + 1, cs); > {%- if not loop.last %} > m += {{field_size}}; > dataSize -= {{field_size}}; > @@ -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);
Hi Umang, On Thu, Aug 19, 2021 at 11:35:13AM +0530, Umang Jain wrote: > Hi Paul, > > On 8/17/21 10:16 AM, 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. > > > > Additionally, change the data portion of the serialized form of > > FileDescriptor to a 32-bit unsigned integer, for alightnment purposes > > and in preparation for conversion to an index into the fd array. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > Do you any preferences of merging this? I plan to merge the fd-leak test I was just waiting for one more review before pushing it, thanks :) > sometime today (so it'll fail / reproducing the issue), so this fix can be > applied over it. I don't want a failing test lingering in master for too > long, so I think merging this patch after the test is in, should be okay? > Any thoughts? I can handle all the merging if you want that, as well :-) Hiro's patch actually reminded me that I need to fix the deserializer offset for FrameBuffer::plane, so I'll send a new version just so that it can be tested first. I'll push it after that's approved. Thanks, Paul > > > > > > --- > > Changes in v3: > > - encode isValid as a uint32 > > --- > > .../libcamera/internal/ipa_data_serializer.h | 40 ++++----- > > include/libcamera/internal/ipc_pipe.h | 10 ++- > > src/libcamera/ipa_data_serializer.cpp | 86 +++++++++++-------- > > 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 | 26 +++--- > > 9 files changed, 105 insertions(+), 87 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, > > 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..91471f52 100644 > > --- a/src/libcamera/ipa_data_serializer.cpp > > +++ b/src/libcamera/ipa_data_serializer.cpp > > @@ -7,6 +7,8 @@ > > #include "libcamera/internal/ipa_data_serializer.h" > > +#include <unistd.h> > > + > > #include <libcamera/base/log.h> > > /** > > @@ -141,7 +143,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 +164,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 +189,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>> \ > > IPADataSerializer<type>::serialize(const type &data, \ > > [[maybe_unused]] ControlSerializer *cs) \ > > { \ > > @@ -215,7 +217,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 +226,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 +251,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 +278,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 +288,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 +307,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 +407,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 +417,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 +431,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 +493,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 +503,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 +524,45 @@ 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; > > + std::vector<uint8_t> dataVec; > > + std::vector<FileDescriptor> fdVec; > > + > > + /* > > + * Store as uint32_t to prepare for conversion from validity flag > > + * to index, and for alignment > > + */ > > + appendPOD<uint32_t>(dataVec, data.isValid()); > > + > > if (data.isValid()) > > - fdVec.push_back(data.fd()); > > + fdVec.push_back(data); > > + > > return { dataVec, fdVec }; > > } > > 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); > > + ASSERT(std::distance(dataBegin, dataEnd) >= 4); > > - bool valid = !!(*dataBegin); > > + uint32_t valid = readPOD<uint32_t>(dataBegin, 0, dataEnd); > > ASSERT(!(valid && std::distance(fdsBegin, fdsEnd) < 1)); > > - return valid ? FileDescriptor(*fdsBegin) : FileDescriptor(); > > + return valid ? *fdsBegin : FileDescriptor(); > > } > > 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 +575,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,13 +598,13 @@ 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; > > - ret.fd = IPADataSerializer<FileDescriptor>::deserialize(dataBegin, dataBegin + 1, > > + ret.fd = IPADataSerializer<FileDescriptor>::deserialize(dataBegin, dataBegin + 4, > > fdsBegin, fdsBegin + 1); > > ret.length = readPOD<uint32_t>(dataBegin, 1, dataEnd); > > @@ -604,7 +614,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()); > > return payload; > > } > > diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp > > index bf7e34e2..eca19a66 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 c222f5f2..1979e68f 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..b8ef8e7b 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) = > > @@ -104,9 +104,9 @@ > > dataSize -= {{field_size}}; > > {%- endif %} > > {% elif field|is_fd %} > > - {%- set field_size = 1 %} > > + {%- set field_size = 4 %} > > {{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data')}} > > - ret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + 1, n, n + 1, cs); > > + ret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}}, n, n + 1, cs); > > {%- if not loop.last %} > > m += {{field_size}}; > > dataSize -= {{field_size}}; > > @@ -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);
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, 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..91471f52 100644 --- a/src/libcamera/ipa_data_serializer.cpp +++ b/src/libcamera/ipa_data_serializer.cpp @@ -7,6 +7,8 @@ #include "libcamera/internal/ipa_data_serializer.h" +#include <unistd.h> + #include <libcamera/base/log.h> /** @@ -141,7 +143,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 +164,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 +189,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>> \ IPADataSerializer<type>::serialize(const type &data, \ [[maybe_unused]] ControlSerializer *cs) \ { \ @@ -215,7 +217,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 +226,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 +251,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 +278,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 +288,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 +307,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 +407,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 +417,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 +431,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 +493,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 +503,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 +524,45 @@ 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; + std::vector<uint8_t> dataVec; + std::vector<FileDescriptor> fdVec; + + /* + * Store as uint32_t to prepare for conversion from validity flag + * to index, and for alignment + */ + appendPOD<uint32_t>(dataVec, data.isValid()); + if (data.isValid()) - fdVec.push_back(data.fd()); + fdVec.push_back(data); + return { dataVec, fdVec }; } 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); + ASSERT(std::distance(dataBegin, dataEnd) >= 4); - bool valid = !!(*dataBegin); + uint32_t valid = readPOD<uint32_t>(dataBegin, 0, dataEnd); ASSERT(!(valid && std::distance(fdsBegin, fdsEnd) < 1)); - return valid ? FileDescriptor(*fdsBegin) : FileDescriptor(); + return valid ? *fdsBegin : FileDescriptor(); } 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 +575,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,13 +598,13 @@ 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; - ret.fd = IPADataSerializer<FileDescriptor>::deserialize(dataBegin, dataBegin + 1, + ret.fd = IPADataSerializer<FileDescriptor>::deserialize(dataBegin, dataBegin + 4, fdsBegin, fdsBegin + 1); ret.length = readPOD<uint32_t>(dataBegin, 1, dataEnd); @@ -604,7 +614,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()); return payload; } diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp index bf7e34e2..eca19a66 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 c222f5f2..1979e68f 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..b8ef8e7b 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) = @@ -104,9 +104,9 @@ dataSize -= {{field_size}}; {%- endif %} {% elif field|is_fd %} - {%- set field_size = 1 %} + {%- set field_size = 4 %} {{- check_data_size(field_size, 'dataSize', field.mojom_name, 'data')}} - ret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + 1, n, n + 1, cs); + ret.{{field.mojom_name}} = IPADataSerializer<{{field|name}}>::deserialize(m, m + {{field_size}}, n, n + 1, cs); {%- if not loop.last %} m += {{field_size}}; dataSize -= {{field_size}}; @@ -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);
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. Additionally, change the data portion of the serialized form of FileDescriptor to a 32-bit unsigned integer, for alightnment purposes and in preparation for conversion to an index into the fd array. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- Changes in v3: - encode isValid as a uint32 --- .../libcamera/internal/ipa_data_serializer.h | 40 ++++----- include/libcamera/internal/ipc_pipe.h | 10 ++- src/libcamera/ipa_data_serializer.cpp | 86 +++++++++++-------- 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 | 26 +++--- 9 files changed, 105 insertions(+), 87 deletions(-)