From 31078711d6c3639073db97322c6f7d98dacbbefe Mon Sep 17 00:00:00 2001 From: Paul Elder Date: Tue, 20 Jul 2021 19:24:47 +0900 Subject: ipa: Use FileDescriptor instead of int in layers above IPC payload 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. Also update the deserializer of FrameBuffer::Plane accordingly. Signed-off-by: Paul Elder Tested-by: Umang Jain Reviewed-by: Laurent Pinchart Reviewed-by: Umang Jain --- src/libcamera/ipa_data_serializer.cpp | 95 +++++++++++++++++++---------------- src/libcamera/ipc_pipe.cpp | 12 +++-- 2 files changed, 60 insertions(+), 47 deletions(-) (limited to 'src') diff --git a/src/libcamera/ipa_data_serializer.cpp b/src/libcamera/ipa_data_serializer.cpp index fb941e6b..5b183c70 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 + #include /** @@ -141,7 +143,7 @@ namespace { /** * \fn template IPADataSerializer::deserialize( * const std::vector &data, - * const std::vector &fds, + * const std::vector &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 IPADataSerializer::deserialize( * std::vector::const_iterator dataBegin, * std::vector::const_iterator dataEnd, - * std::vector::const_iterator fdsBegin, - * std::vector::const_iterator fdsEnd, + * std::vector::const_iterator fdsBegin, + * std::vector::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> \ +std::tuple, std::vector> \ IPADataSerializer::serialize(const type &data, \ [[maybe_unused]] ControlSerializer *cs) \ { \ @@ -215,7 +217,7 @@ type IPADataSerializer::deserialize(const std::vector &data, \ \ template<> \ type IPADataSerializer::deserialize(const std::vector &data, \ - [[maybe_unused]] const std::vector &fds, \ + [[maybe_unused]] const std::vector &fds, \ ControlSerializer *cs) \ { \ return deserialize(data.cbegin(), data.end(), cs); \ @@ -224,8 +226,8 @@ type IPADataSerializer::deserialize(const std::vector &data, \ template<> \ type IPADataSerializer::deserialize(std::vector::const_iterator dataBegin, \ std::vector::const_iterator dataEnd, \ - [[maybe_unused]] std::vector::const_iterator fdsBegin, \ - [[maybe_unused]] std::vector::const_iterator fdsEnd, \ + [[maybe_unused]] std::vector::const_iterator fdsBegin, \ + [[maybe_unused]] std::vector::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> +std::tuple, std::vector> IPADataSerializer::serialize(const std::string &data, [[maybe_unused]] ControlSerializer *cs) { @@ -276,7 +278,7 @@ IPADataSerializer::deserialize(std::vector::const_iterator template<> std::string IPADataSerializer::deserialize(const std::vector &data, - [[maybe_unused]] const std::vector &fds, + [[maybe_unused]] const std::vector &fds, [[maybe_unused]] ControlSerializer *cs) { return { data.cbegin(), data.cend() }; @@ -286,8 +288,8 @@ template<> std::string IPADataSerializer::deserialize(std::vector::const_iterator dataBegin, std::vector::const_iterator dataEnd, - [[maybe_unused]] std::vector::const_iterator fdsBegin, - [[maybe_unused]] std::vector::const_iterator fdsEnd, + [[maybe_unused]] std::vector::const_iterator fdsBegin, + [[maybe_unused]] std::vector::const_iterator fdsEnd, [[maybe_unused]] ControlSerializer *cs) { return { dataBegin, dataEnd }; @@ -305,7 +307,7 @@ IPADataSerializer::deserialize(std::vector::const_iterator * be used. The serialized ControlInfoMap will have zero length. */ template<> -std::tuple, std::vector> +std::tuple, std::vector> IPADataSerializer::serialize(const ControlList &data, ControlSerializer *cs) { if (!cs) @@ -405,7 +407,7 @@ IPADataSerializer::deserialize(const std::vector &data, template<> ControlList IPADataSerializer::deserialize(const std::vector &data, - [[maybe_unused]] const std::vector &fds, + [[maybe_unused]] const std::vector &fds, ControlSerializer *cs) { return deserialize(data.cbegin(), data.end(), cs); @@ -415,8 +417,8 @@ template<> ControlList IPADataSerializer::deserialize(std::vector::const_iterator dataBegin, std::vector::const_iterator dataEnd, - [[maybe_unused]] std::vector::const_iterator fdsBegin, - [[maybe_unused]] std::vector::const_iterator fdsEnd, + [[maybe_unused]] std::vector::const_iterator fdsBegin, + [[maybe_unused]] std::vector::const_iterator fdsEnd, ControlSerializer *cs) { return deserialize(dataBegin, dataEnd, cs); @@ -429,7 +431,7 @@ IPADataSerializer::deserialize(std::vector::const_iterator * X bytes - Serialized ControlInfoMap (using ControlSerializer) */ template<> -std::tuple, std::vector> +std::tuple, std::vector> IPADataSerializer::serialize(const ControlInfoMap &map, ControlSerializer *cs) { @@ -491,7 +493,7 @@ IPADataSerializer::deserialize(const std::vector &data, template<> ControlInfoMap IPADataSerializer::deserialize(const std::vector &data, - [[maybe_unused]] const std::vector &fds, + [[maybe_unused]] const std::vector &fds, ControlSerializer *cs) { return deserialize(data.cbegin(), data.end(), cs); @@ -501,15 +503,15 @@ template<> ControlInfoMap IPADataSerializer::deserialize(std::vector::const_iterator dataBegin, std::vector::const_iterator dataEnd, - [[maybe_unused]] std::vector::const_iterator fdsBegin, - [[maybe_unused]] std::vector::const_iterator fdsEnd, + [[maybe_unused]] std::vector::const_iterator fdsBegin, + [[maybe_unused]] std::vector::const_iterator fdsEnd, ControlSerializer *cs) { return deserialize(dataBegin, dataEnd, cs); } /* - * FileDescriptors are serialized into a single byte that tells if the + * FileDescriptors are serialized into four bytes that tells if the * FileDescriptor is valid or not. If it is valid, then for serialization * the fd will be written to the fd vector, or for deserialization the * fd vector const_iterator will be valid. @@ -517,42 +519,47 @@ IPADataSerializer::deserialize(std::vector::const_itera * This validity is necessary so that we don't send -1 fd over sendmsg(). It * also allows us to simply send the entire fd vector into the deserializer * and it will be recursively consumed as necessary. - * - * \todo Consider serializing the FileDescriptor in 4 bytes to ensure - * 32-bit alignment of all serialized data */ template<> -std::tuple, std::vector> +std::tuple, std::vector> IPADataSerializer::serialize(const FileDescriptor &data, [[maybe_unused]] ControlSerializer *cs) { - std::vector dataVec = { data.isValid() }; - std::vector fdVec; + std::vector dataVec; + std::vector fdVec; + + /* + * Store as uint32_t to prepare for conversion from validity flag + * to index, and for alignment. + */ + appendPOD(dataVec, data.isValid()); + if (data.isValid()) - fdVec.push_back(data.fd()); + fdVec.push_back(data); + return { dataVec, fdVec }; } template<> -FileDescriptor IPADataSerializer::deserialize(std::vector::const_iterator dataBegin, - std::vector::const_iterator dataEnd, - std::vector::const_iterator fdsBegin, - std::vector::const_iterator fdsEnd, +FileDescriptor IPADataSerializer::deserialize([[maybe_unused]] std::vector::const_iterator dataBegin, + [[maybe_unused]] std::vector::const_iterator dataEnd, + std::vector::const_iterator fdsBegin, + std::vector::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(dataBegin, 0, dataEnd); ASSERT(!(valid && std::distance(fdsBegin, fdsEnd) < 1)); - return valid ? FileDescriptor(*fdsBegin) : FileDescriptor(); + return valid ? *fdsBegin : FileDescriptor(); } template<> FileDescriptor IPADataSerializer::deserialize(const std::vector &data, - const std::vector &fds, + const std::vector &fds, [[maybe_unused]] ControlSerializer *cs) { return deserialize(data.cbegin(), data.end(), fds.cbegin(), fds.end()); @@ -561,19 +568,19 @@ FileDescriptor IPADataSerializer::deserialize(const std::vector< /* * FrameBuffer::Plane is serialized as: * - * 1 byte - FileDescriptor + * 4 byte - FileDescriptor * 4 bytes - uint32_t Length */ template<> -std::tuple, std::vector> +std::tuple, std::vector> IPADataSerializer::serialize(const FrameBuffer::Plane &data, [[maybe_unused]] ControlSerializer *cs) { std::vector dataVec; - std::vector fdsVec; + std::vector fdsVec; std::vector fdBuf; - std::vector fdFds; + std::vector fdFds; std::tie(fdBuf, fdFds) = IPADataSerializer::serialize(data.fd); dataVec.insert(dataVec.end(), fdBuf.begin(), fdBuf.end()); @@ -588,15 +595,15 @@ template<> FrameBuffer::Plane IPADataSerializer::deserialize(std::vector::const_iterator dataBegin, std::vector::const_iterator dataEnd, - std::vector::const_iterator fdsBegin, - [[maybe_unused]] std::vector::const_iterator fdsEnd, + std::vector::const_iterator fdsBegin, + [[maybe_unused]] std::vector::const_iterator fdsEnd, [[maybe_unused]] ControlSerializer *cs) { FrameBuffer::Plane ret; - ret.fd = IPADataSerializer::deserialize(dataBegin, dataBegin + 1, + ret.fd = IPADataSerializer::deserialize(dataBegin, dataBegin + 4, fdsBegin, fdsBegin + 1); - ret.length = readPOD(dataBegin, 1, dataEnd); + ret.length = readPOD(dataBegin, 4, dataEnd); return ret; } @@ -604,7 +611,7 @@ IPADataSerializer::deserialize(std::vector::const_i template<> FrameBuffer::Plane IPADataSerializer::deserialize(const std::vector &data, - const std::vector &fds, + const std::vector &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(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; } -- cgit v1.2.1