| Message ID | 20260723174644.6580-2-magdum.foss@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi 2026. 07. 23. 19:46 keltezéssel, Magdum írta: > Bump IPA controls serialization format to v3 and add name_len to > ControlInfoMap entries. > > Serialize null-terminated control names for local (V4L2-style) id > maps to preserve names across IPC. Do we need it to be null terminated? If not, I think we could just omit it. > > Extend serializer tests with V4L2-like name round-trip coverage and > malformed payload rejection for oversized length, missing terminator, > and too-long serialize input. > > Signed-off-by: Magdum <magdum.foss@gmail.com> > --- > include/libcamera/ipa/ipa_controls.h | 6 +- > src/libcamera/control_serializer.cpp | 133 +++++++++++++------ > test/serialization/control_serialization.cpp | 109 +++++++++++++++ > 3 files changed, 207 insertions(+), 41 deletions(-) > > diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h > index 6af962ff..7a7873d9 100644 > --- a/include/libcamera/ipa/ipa_controls.h > +++ b/include/libcamera/ipa/ipa_controls.h > @@ -15,7 +15,7 @@ namespace libcamera { > extern "C" { > #endif > > -#define IPA_CONTROLS_FORMAT_VERSION 2 > +#define IPA_CONTROLS_FORMAT_VERSION 3 > > enum ipa_controls_id_map_type { > IPA_CONTROL_ID_MAP_CONTROLS, > @@ -50,7 +50,9 @@ struct ipa_control_info_entry { > uint32_t id; > uint32_t type; > uint8_t direction; > - uint8_t padding[7]; > + uint8_t padding[3]; > + /* Length of the control name without the terminating null byte. */ > + uint32_t name_len; I'm wondering if we might want an "offset" here for name just like each `ipa_control_value_entry` has it. > struct ipa_control_value_entry min; > struct ipa_control_value_entry max; > struct ipa_control_value_entry def; The documentation in `ipa_controls.cpp` should also be updated if I'm not mistaken. > [...]
Hi 2026. 07. 23. 19:46 keltezéssel, Magdum írta: > Bump IPA controls serialization format to v3 and add name_len to > ControlInfoMap entries. > > Serialize null-terminated control names for local (V4L2-style) id > maps to preserve names across IPC. <<Do we need it to be null terminated? If not, I think we could just omit it. Good point — since name_len is already encoded in the entry, the receiver knows exactly how many bytes to consume without a sentinel. Dropped the null terminator from both the serialization (size calculation + write) and deserialization (read + validity check). > > Extend serializer tests with V4L2-like name round-trip coverage and > malformed payload rejection for oversized length, missing terminator, > and too-long serialize input. > > Signed-off-by: Magdum <magdum.foss@gmail.com> > --- > include/libcamera/ipa/ipa_controls.h | 6 +- > src/libcamera/control_serializer.cpp | 133 +++++++++++++------ > test/serialization/control_serialization.cpp | 109 +++++++++++++++ > 3 files changed, 207 insertions(+), 41 deletions(-) > > diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h > index 6af962ff..7a7873d9 100644 > --- a/include/libcamera/ipa/ipa_controls.h > +++ b/include/libcamera/ipa/ipa_controls.h > @@ -15,7 +15,7 @@ namespace libcamera { > extern "C" { > #endif > > -#define IPA_CONTROLS_FORMAT_VERSION 2 > +#define IPA_CONTROLS_FORMAT_VERSION 3 > > enum ipa_controls_id_map_type { > IPA_CONTROL_ID_MAP_CONTROLS, > @@ -50,7 +50,9 @@ struct ipa_control_info_entry { > uint32_t id; > uint32_t type; > uint8_t direction; > - uint8_t padding[7]; > + uint8_t padding[3]; > + /* Length of the control name without the terminating null byte. */ > + uint32_t name_len; <<I'm wondering if we might want an "offset" here for name <<just like each `ipa_control_value_entry` has it. Done — added name_offset to ipa_control_info_entry and wired it up the same way as the value entries: recorded at serialize time and checked against values.offset() before reading at deserialize time. > struct ipa_control_value_entry min; > struct ipa_control_value_entry max; > struct ipa_control_value_entry def; <<The documentation in `ipa_controls.cpp` should also <<be updated if I'm not mistaken. Done > [...]
Hi 2026. 07. 23. 19:46 keltezéssel, Magdum írta: > Bump IPA controls serialization format to v3 and add name_len to > ControlInfoMap entries. > > Serialize null-terminated control names for local (V4L2-style) id > maps to preserve names across IPC. <<Do we need it to be null terminated? If not, I think we could just omit it. Good point — since name_len is already encoded in the entry, the receiver knows exactly how many bytes to consume without a sentinel. Dropped the null terminator from both the serialization (size calculation + write) and deserialization (read + validity check). > > Extend serializer tests with V4L2-like name round-trip coverage and > malformed payload rejection for oversized length, missing terminator, > and too-long serialize input. > > Signed-off-by: Magdum <magdum.foss@gmail.com> > --- > include/libcamera/ipa/ipa_controls.h | 6 +- > src/libcamera/control_serializer.cpp | 133 +++++++++++++------ > test/serialization/control_serialization.cpp | 109 +++++++++++++++ > 3 files changed, 207 insertions(+), 41 deletions(-) > > diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h > index 6af962ff..7a7873d9 100644 > --- a/include/libcamera/ipa/ipa_controls.h > +++ b/include/libcamera/ipa/ipa_controls.h > @@ -15,7 +15,7 @@ namespace libcamera { > extern "C" { > #endif > > -#define IPA_CONTROLS_FORMAT_VERSION 2 > +#define IPA_CONTROLS_FORMAT_VERSION 3 > > enum ipa_controls_id_map_type { > IPA_CONTROL_ID_MAP_CONTROLS, > @@ -50,7 +50,9 @@ struct ipa_control_info_entry { > uint32_t id; > uint32_t type; > uint8_t direction; > - uint8_t padding[7]; > + uint8_t padding[3]; > + /* Length of the control name without the terminating null byte. */ > + uint32_t name_len; <<I'm wondering if we might want an "offset" here for name <<just like each `ipa_control_value_entry` has it. Done — added name_offset to ipa_control_info_entry and wired it up the same way as the value entries: recorded at serialize time and checked against values.offset() before reading at deserialize time. > struct ipa_control_value_entry min; > struct ipa_control_value_entry max; > struct ipa_control_value_entry def; <<The documentation in `ipa_controls.cpp` should also <<be updated if I'm not mistaken. Done > [...]
Hello Magdum, Quoting Magdum (2026-07-24 02:46:43) > Bump IPA controls serialization format to v3 and add name_len to > ControlInfoMap entries. > > Serialize null-terminated control names for local (V4L2-style) id > maps to preserve names across IPC. You've described "what" but not "why". Have you seen "How to Write a Git Commit Message"? https://cbea.ms/git-commit/ The commit subject should also be prefixed with "libcamera: controls:" and s/local/v4l2/. imo the v3 isn't significant enough to be in the subject. > > Extend serializer tests with V4L2-like name round-trip coverage and > malformed payload rejection for oversized length, missing terminator, > and too-long serialize input. The serialization format documentation in ipa_controls.cpp needs to be updated. > > Signed-off-by: Magdum <magdum.foss@gmail.com> > --- > include/libcamera/ipa/ipa_controls.h | 6 +- > src/libcamera/control_serializer.cpp | 133 +++++++++++++------ > test/serialization/control_serialization.cpp | 109 +++++++++++++++ > 3 files changed, 207 insertions(+), 41 deletions(-) > > diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h > index 6af962ff..7a7873d9 100644 > --- a/include/libcamera/ipa/ipa_controls.h > +++ b/include/libcamera/ipa/ipa_controls.h > @@ -15,7 +15,7 @@ namespace libcamera { > extern "C" { > #endif > > -#define IPA_CONTROLS_FORMAT_VERSION 2 > +#define IPA_CONTROLS_FORMAT_VERSION 3 > > enum ipa_controls_id_map_type { > IPA_CONTROL_ID_MAP_CONTROLS, > @@ -50,7 +50,9 @@ struct ipa_control_info_entry { > uint32_t id; > uint32_t type; > uint8_t direction; > - uint8_t padding[7]; > + uint8_t padding[3]; > + /* Length of the control name without the terminating null byte. */ This belongs in the documentation in ipa_controls.cpp. Just curious, did you get any assistance from LLMs to write these? > + uint32_t name_len; > struct ipa_control_value_entry min; > struct ipa_control_value_entry max; > struct ipa_control_value_entry def; > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp > index c0285cc6..b3c44ed0 100644 > --- a/src/libcamera/control_serializer.cpp > +++ b/src/libcamera/control_serializer.cpp > @@ -31,6 +31,27 @@ namespace libcamera { > > LOG_DEFINE_CATEGORY(Serializer) > > +namespace { > + > +constexpr uint32_t kMaxControlNameLength = 1024; > + > +enum ipa_controls_id_map_type idMapTypeFor(const ControlIdMap &idmap) > +{ > + if (&idmap == &controls::controls) > + return IPA_CONTROL_ID_MAP_CONTROLS; > + if (&idmap == &properties::properties) > + return IPA_CONTROL_ID_MAP_PROPERTIES; > + > + return IPA_CONTROL_ID_MAP_V4L2; > +} I think this helper could be a bit better. There's an instance in the ControlList serializer that looks like it could be refactored with this but isn't. Why is that? Also every single callsite of this is linked to the below function. Couldn't they be merged into a more direct and succint helper? > + > +bool idMapRequiresLocalIds(enum ipa_controls_id_map_type idMapType) > +{ > + return idMapType == IPA_CONTROL_ID_MAP_V4L2; > +} > + > +} /* namespace */ > + > /** > * \class ControlSerializer > * \brief Serializer and deserializer for control-related classes > @@ -165,10 +186,16 @@ size_t ControlSerializer::binarySize(const ControlInfoMap &infoMap) > { > size_t size = sizeof(struct ipa_controls_header) > + infoMap.size() * sizeof(struct ipa_control_info_entry); > + enum ipa_controls_id_map_type idMapType = idMapTypeFor(infoMap.idmap()); > > - for (const auto &ctrl : infoMap) > + for (const auto &ctrl : infoMap) { > size += binarySize(ctrl.second); > > + size += idMapRequiresLocalIds(idMapType) > + ? ctrl.first->name().size() + 1 > + : 1; > + } > + > return size; > } > > @@ -183,8 +210,7 @@ size_t ControlSerializer::binarySize(const ControlInfoMap &infoMap) > */ > size_t ControlSerializer::binarySize(const ControlList &list) > { > - size_t size = sizeof(struct ipa_controls_header) > - + list.size() * sizeof(struct ipa_control_list_entry); > + size_t size = sizeof(struct ipa_controls_header) + list.size() * sizeof(struct ipa_control_list_entry); Why is this indentation changed? > > for (const auto &ctrl : list) > size += binarySize(ctrl.second); > @@ -231,24 +257,21 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap, > return 0; > } > > + enum ipa_controls_id_map_type idMapType = idMapTypeFor(infoMap.idmap()); > + > /* Compute entries and data required sizes. */ > size_t entriesSize = infoMap.size() > * sizeof(struct ipa_control_info_entry); > size_t valuesSize = 0; > - for (const auto &ctrl : infoMap) > + for (const auto &ctrl : infoMap) { > valuesSize += binarySize(ctrl.second); > - > - const ControlIdMap *idmap = &infoMap.idmap(); > - enum ipa_controls_id_map_type idMapType; > - if (idmap == &controls::controls) > - idMapType = IPA_CONTROL_ID_MAP_CONTROLS; > - else if (idmap == &properties::properties) > - idMapType = IPA_CONTROL_ID_MAP_PROPERTIES; > - else > - idMapType = IPA_CONTROL_ID_MAP_V4L2; > + valuesSize += idMapRequiresLocalIds(idMapType) > + ? ctrl.first->name().size() + 1 > + : 1; > + } > > /* Prepare the packet header. */ > - struct ipa_controls_header hdr; > + struct ipa_controls_header hdr = {}; Every field is populated immediately below; do we need to explicitly zero-initialize everything? > hdr.version = IPA_CONTROLS_FORMAT_VERSION; > hdr.handle = serial_; > hdr.entries = infoMap.size(); > @@ -267,21 +290,29 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap, > */ > serial_ += 2; > > - /* > - * Serialize all entries. > - * \todo Serialize the control name too > - */ > + /* Serialize all entries. */ > ByteStreamBuffer entries = buffer.carveOut(entriesSize); > ByteStreamBuffer values = buffer.carveOut(valuesSize); > + static const std::string emptyName; > > for (const auto &ctrl : infoMap) { > const ControlId *id = ctrl.first; > const ControlInfo &info = ctrl.second; > + const std::string &name = idMapRequiresLocalIds(idMapType) > + ? id->name() > + : emptyName; > + > + if (name.size() > kMaxControlNameLength) { > + LOG(Serializer, Error) > + << "Control name too long: " << name.size(); > + return -EINVAL; > + } > > - struct ipa_control_info_entry entry; > + struct ipa_control_info_entry entry = {}; > entry.id = id->id(); > entry.type = id->type(); > entry.direction = static_cast<ControlId::DirectionFlags::Type>(id->direction()); > + entry.name_len = static_cast<uint32_t>(name.size()); > > populateControlValueEntry(entry.min, info.min(), values.offset()); > store(info.min(), values); > @@ -292,6 +323,10 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap, > populateControlValueEntry(entry.def, info.def(), values.offset()); > store(info.def(), values); > > + values.write(Span<const uint8_t>( > + reinterpret_cast<const uint8_t *>(name.c_str()), > + name.size() + 1)); > + > entries.write(&entry); > } > > @@ -355,7 +390,7 @@ int ControlSerializer::serialize(const ControlList &list, > valuesSize += binarySize(ctrl.second); > > /* Prepare the packet header. */ > - struct ipa_controls_header hdr; > + struct ipa_controls_header hdr = {}; Same here. Thanks, Paul > hdr.version = IPA_CONTROLS_FORMAT_VERSION; > hdr.handle = infoMapHandle; > hdr.entries = list.size(); > @@ -485,25 +520,6 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > > ControlType type = static_cast<ControlType>(entry->type); > > - /* If we're using a local id map, populate it. */ > - if (localIdMap) { > - ControlId::DirectionFlags flags{ > - static_cast<ControlId::Direction>(entry->direction) > - }; > - > - /** > - * \todo Find a way to preserve the control name for > - * debugging purpose. > - */ > - controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, > - "", "local", type, > - flags)); > - (*localIdMap)[entry->id] = controlIds_.back().get(); > - } > - > - const ControlId *controlId = idMap->at(entry->id); > - ASSERT(controlId); > - > const ipa_control_value_entry &min_entry = entry->min; > const ipa_control_value_entry &max_entry = entry->max; > const ipa_control_value_entry &def_entry = entry->def; > @@ -538,6 +554,45 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & > loadControlValue(values, static_cast<ControlType>(def_entry.type), > def_entry.is_array, def_entry.count); > > + /* > + * Deserialize the null-terminated control name from the values > + * section. Reject unreasonably long names to guard against > + * malformed packets. > + */ > + if (entry->name_len > kMaxControlNameLength) { > + LOG(Serializer, Error) > + << "Control name too long: " << entry->name_len; > + return {}; > + } > + > + const auto *nameData = values.read<const uint8_t>(entry->name_len + 1); > + if (!nameData) { > + LOG(Serializer, Error) << "Out of data reading control name"; > + return {}; > + } > + > + if (nameData[entry->name_len] != '\0') { > + LOG(Serializer, Error) << "Control name is not null-terminated"; > + return {}; > + } > + > + /* If we're using a local id map, populate it with the restored name. */ > + if (localIdMap) { > + std::string ctrlName(reinterpret_cast<const char *>(nameData), > + entry->name_len); > + > + ControlId::DirectionFlags flags{ > + static_cast<ControlId::Direction>(entry->direction) > + }; > + > + controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, > + ctrlName, "local", > + type, flags)); > + (*localIdMap)[entry->id] = controlIds_.back().get(); > + } > + > + const ControlId *controlId = idMap->at(entry->id); > + ASSERT(controlId); > > /* Create and store the ControlInfo. */ > ctrls.emplace(controlId, ControlInfo(min, max, def)); > diff --git a/test/serialization/control_serialization.cpp b/test/serialization/control_serialization.cpp > index 06c572b7..b1638db9 100644 > --- a/test/serialization/control_serialization.cpp > +++ b/test/serialization/control_serialization.cpp > @@ -11,6 +11,8 @@ > #include <libcamera/control_ids.h> > #include <libcamera/controls.h> > > +#include <libcamera/ipa/ipa_controls.h> > + > #include "libcamera/internal/byte_stream_buffer.h" > #include "libcamera/internal/control_serializer.h" > > @@ -169,6 +171,113 @@ protected: > return TestFail; > } > > + /* Build a local (V4L2-like) ControlInfoMap and verify name round-trip. */ > + vector<unique_ptr<ControlId>> v4l2ControlIds; > + ControlIdMap v4l2IdMap; > + constexpr uint32_t kV4L2TestControlId = 0x009a2001; > + const string kV4L2ControlName = "V4L2_CID_TEST_GAIN"; > + > + v4l2ControlIds.emplace_back(std::make_unique<ControlId>( > + kV4L2TestControlId, kV4L2ControlName, "v4l2", > + ControlTypeInteger32, ControlId::Direction::In)); > + v4l2IdMap.emplace(kV4L2TestControlId, v4l2ControlIds.back().get()); > + > + ControlInfoMap::Map v4l2Info; > + v4l2Info.emplace(v4l2ControlIds.back().get(), > + ControlInfo(ControlValue(int32_t{ 0 }), > + ControlValue(int32_t{ 255 }), > + ControlValue(int32_t{ 16 }))); > + ControlInfoMap v4l2InfoMap(std::move(v4l2Info), v4l2IdMap); > + > + ControlSerializer v4l2Serializer(ControlSerializer::Role::Proxy); > + ControlSerializer v4l2Deserializer(ControlSerializer::Role::Worker); > + > + size = v4l2Serializer.binarySize(v4l2InfoMap); > + infoData.resize(size); > + buffer = ByteStreamBuffer(infoData.data(), infoData.size()); > + > + ret = v4l2Serializer.serialize(v4l2InfoMap, buffer); > + if (ret < 0 || buffer.overflow()) { > + cerr << "Failed to serialize V4L2-like ControlInfoMap" << endl; > + return TestFail; > + } > + > + buffer = ByteStreamBuffer(const_cast<const uint8_t *>(infoData.data()), > + infoData.size()); > + ControlInfoMap v4l2InfoMapDes = > + v4l2Deserializer.deserialize<ControlInfoMap>(buffer); > + if (v4l2InfoMapDes.empty()) { > + cerr << "Failed to deserialize V4L2-like ControlInfoMap" << endl; > + return TestFail; > + } > + > + auto idIt = v4l2InfoMapDes.idmap().find(kV4L2TestControlId); > + if (idIt == v4l2InfoMapDes.idmap().end()) { > + cerr << "Deserialized V4L2-like id map misses test control" << endl; > + return TestFail; > + } > + > + if (idIt->second->name() != kV4L2ControlName) { > + cerr << "Deserialized V4L2-like control name doesn't match" << endl; > + return TestFail; > + } > + > + /* Reject malformed packets with over-sized names. */ > + vector<uint8_t> badNameLenData = infoData; > + auto *badNameLenHeader = > + reinterpret_cast<ipa_controls_header *>(badNameLenData.data()); > + auto *badNameLenEntry = reinterpret_cast<ipa_control_info_entry *>( > + badNameLenData.data() + sizeof(*badNameLenHeader)); > + badNameLenEntry->name_len = 2048; > + > + ControlSerializer badNameLenDeserializer(ControlSerializer::Role::Worker); > + buffer = ByteStreamBuffer(const_cast<const uint8_t *>(badNameLenData.data()), > + badNameLenData.size()); > + if (!badNameLenDeserializer.deserialize<ControlInfoMap>(buffer).empty()) { > + cerr << "Oversized control name should be rejected" << endl; > + return TestFail; > + } > + > + /* Reject malformed packets with non-null-terminated names. */ > + vector<uint8_t> badTermData = infoData; > + badTermData.back() = 'X'; > + > + ControlSerializer badTermDeserializer(ControlSerializer::Role::Worker); > + buffer = ByteStreamBuffer(const_cast<const uint8_t *>(badTermData.data()), > + badTermData.size()); > + if (!badTermDeserializer.deserialize<ControlInfoMap>(buffer).empty()) { > + cerr << "Control name without null terminator should be rejected" << endl; > + return TestFail; > + } > + > + /* Reject too-long names at serialization time. */ > + vector<unique_ptr<ControlId>> longNameControlIds; > + ControlIdMap longNameIdMap; > + string longName(1025, 'n'); > + > + longNameControlIds.emplace_back(std::make_unique<ControlId>( > + 0x009a2002, longName, "v4l2", ControlTypeInteger32, > + ControlId::Direction::In)); > + longNameIdMap.emplace(0x009a2002, longNameControlIds.back().get()); > + > + ControlInfoMap::Map longNameInfo; > + longNameInfo.emplace(longNameControlIds.back().get(), > + ControlInfo(ControlValue(int32_t{ 0 }), > + ControlValue(int32_t{ 255 }), > + ControlValue(int32_t{ 16 }))); > + ControlInfoMap longNameInfoMap(std::move(longNameInfo), longNameIdMap); > + > + ControlSerializer longNameSerializer(ControlSerializer::Role::Proxy); > + size = longNameSerializer.binarySize(longNameInfoMap); > + infoData.resize(size); > + buffer = ByteStreamBuffer(infoData.data(), infoData.size()); > + > + ret = longNameSerializer.serialize(longNameInfoMap, buffer); > + if (ret != -EINVAL) { > + cerr << "Too-long control name should fail serialization" << endl; > + return TestFail; > + } > + > return TestPass; > } > }; > -- > 2.43.0 >
diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h index 6af962ff..7a7873d9 100644 --- a/include/libcamera/ipa/ipa_controls.h +++ b/include/libcamera/ipa/ipa_controls.h @@ -15,7 +15,7 @@ namespace libcamera { extern "C" { #endif -#define IPA_CONTROLS_FORMAT_VERSION 2 +#define IPA_CONTROLS_FORMAT_VERSION 3 enum ipa_controls_id_map_type { IPA_CONTROL_ID_MAP_CONTROLS, @@ -50,7 +50,9 @@ struct ipa_control_info_entry { uint32_t id; uint32_t type; uint8_t direction; - uint8_t padding[7]; + uint8_t padding[3]; + /* Length of the control name without the terminating null byte. */ + uint32_t name_len; struct ipa_control_value_entry min; struct ipa_control_value_entry max; struct ipa_control_value_entry def; diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp index c0285cc6..b3c44ed0 100644 --- a/src/libcamera/control_serializer.cpp +++ b/src/libcamera/control_serializer.cpp @@ -31,6 +31,27 @@ namespace libcamera { LOG_DEFINE_CATEGORY(Serializer) +namespace { + +constexpr uint32_t kMaxControlNameLength = 1024; + +enum ipa_controls_id_map_type idMapTypeFor(const ControlIdMap &idmap) +{ + if (&idmap == &controls::controls) + return IPA_CONTROL_ID_MAP_CONTROLS; + if (&idmap == &properties::properties) + return IPA_CONTROL_ID_MAP_PROPERTIES; + + return IPA_CONTROL_ID_MAP_V4L2; +} + +bool idMapRequiresLocalIds(enum ipa_controls_id_map_type idMapType) +{ + return idMapType == IPA_CONTROL_ID_MAP_V4L2; +} + +} /* namespace */ + /** * \class ControlSerializer * \brief Serializer and deserializer for control-related classes @@ -165,10 +186,16 @@ size_t ControlSerializer::binarySize(const ControlInfoMap &infoMap) { size_t size = sizeof(struct ipa_controls_header) + infoMap.size() * sizeof(struct ipa_control_info_entry); + enum ipa_controls_id_map_type idMapType = idMapTypeFor(infoMap.idmap()); - for (const auto &ctrl : infoMap) + for (const auto &ctrl : infoMap) { size += binarySize(ctrl.second); + size += idMapRequiresLocalIds(idMapType) + ? ctrl.first->name().size() + 1 + : 1; + } + return size; } @@ -183,8 +210,7 @@ size_t ControlSerializer::binarySize(const ControlInfoMap &infoMap) */ size_t ControlSerializer::binarySize(const ControlList &list) { - size_t size = sizeof(struct ipa_controls_header) - + list.size() * sizeof(struct ipa_control_list_entry); + size_t size = sizeof(struct ipa_controls_header) + list.size() * sizeof(struct ipa_control_list_entry); for (const auto &ctrl : list) size += binarySize(ctrl.second); @@ -231,24 +257,21 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap, return 0; } + enum ipa_controls_id_map_type idMapType = idMapTypeFor(infoMap.idmap()); + /* Compute entries and data required sizes. */ size_t entriesSize = infoMap.size() * sizeof(struct ipa_control_info_entry); size_t valuesSize = 0; - for (const auto &ctrl : infoMap) + for (const auto &ctrl : infoMap) { valuesSize += binarySize(ctrl.second); - - const ControlIdMap *idmap = &infoMap.idmap(); - enum ipa_controls_id_map_type idMapType; - if (idmap == &controls::controls) - idMapType = IPA_CONTROL_ID_MAP_CONTROLS; - else if (idmap == &properties::properties) - idMapType = IPA_CONTROL_ID_MAP_PROPERTIES; - else - idMapType = IPA_CONTROL_ID_MAP_V4L2; + valuesSize += idMapRequiresLocalIds(idMapType) + ? ctrl.first->name().size() + 1 + : 1; + } /* Prepare the packet header. */ - struct ipa_controls_header hdr; + struct ipa_controls_header hdr = {}; hdr.version = IPA_CONTROLS_FORMAT_VERSION; hdr.handle = serial_; hdr.entries = infoMap.size(); @@ -267,21 +290,29 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap, */ serial_ += 2; - /* - * Serialize all entries. - * \todo Serialize the control name too - */ + /* Serialize all entries. */ ByteStreamBuffer entries = buffer.carveOut(entriesSize); ByteStreamBuffer values = buffer.carveOut(valuesSize); + static const std::string emptyName; for (const auto &ctrl : infoMap) { const ControlId *id = ctrl.first; const ControlInfo &info = ctrl.second; + const std::string &name = idMapRequiresLocalIds(idMapType) + ? id->name() + : emptyName; + + if (name.size() > kMaxControlNameLength) { + LOG(Serializer, Error) + << "Control name too long: " << name.size(); + return -EINVAL; + } - struct ipa_control_info_entry entry; + struct ipa_control_info_entry entry = {}; entry.id = id->id(); entry.type = id->type(); entry.direction = static_cast<ControlId::DirectionFlags::Type>(id->direction()); + entry.name_len = static_cast<uint32_t>(name.size()); populateControlValueEntry(entry.min, info.min(), values.offset()); store(info.min(), values); @@ -292,6 +323,10 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap, populateControlValueEntry(entry.def, info.def(), values.offset()); store(info.def(), values); + values.write(Span<const uint8_t>( + reinterpret_cast<const uint8_t *>(name.c_str()), + name.size() + 1)); + entries.write(&entry); } @@ -355,7 +390,7 @@ int ControlSerializer::serialize(const ControlList &list, valuesSize += binarySize(ctrl.second); /* Prepare the packet header. */ - struct ipa_controls_header hdr; + struct ipa_controls_header hdr = {}; hdr.version = IPA_CONTROLS_FORMAT_VERSION; hdr.handle = infoMapHandle; hdr.entries = list.size(); @@ -485,25 +520,6 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & ControlType type = static_cast<ControlType>(entry->type); - /* If we're using a local id map, populate it. */ - if (localIdMap) { - ControlId::DirectionFlags flags{ - static_cast<ControlId::Direction>(entry->direction) - }; - - /** - * \todo Find a way to preserve the control name for - * debugging purpose. - */ - controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, - "", "local", type, - flags)); - (*localIdMap)[entry->id] = controlIds_.back().get(); - } - - const ControlId *controlId = idMap->at(entry->id); - ASSERT(controlId); - const ipa_control_value_entry &min_entry = entry->min; const ipa_control_value_entry &max_entry = entry->max; const ipa_control_value_entry &def_entry = entry->def; @@ -538,6 +554,45 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer & loadControlValue(values, static_cast<ControlType>(def_entry.type), def_entry.is_array, def_entry.count); + /* + * Deserialize the null-terminated control name from the values + * section. Reject unreasonably long names to guard against + * malformed packets. + */ + if (entry->name_len > kMaxControlNameLength) { + LOG(Serializer, Error) + << "Control name too long: " << entry->name_len; + return {}; + } + + const auto *nameData = values.read<const uint8_t>(entry->name_len + 1); + if (!nameData) { + LOG(Serializer, Error) << "Out of data reading control name"; + return {}; + } + + if (nameData[entry->name_len] != '\0') { + LOG(Serializer, Error) << "Control name is not null-terminated"; + return {}; + } + + /* If we're using a local id map, populate it with the restored name. */ + if (localIdMap) { + std::string ctrlName(reinterpret_cast<const char *>(nameData), + entry->name_len); + + ControlId::DirectionFlags flags{ + static_cast<ControlId::Direction>(entry->direction) + }; + + controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, + ctrlName, "local", + type, flags)); + (*localIdMap)[entry->id] = controlIds_.back().get(); + } + + const ControlId *controlId = idMap->at(entry->id); + ASSERT(controlId); /* Create and store the ControlInfo. */ ctrls.emplace(controlId, ControlInfo(min, max, def)); diff --git a/test/serialization/control_serialization.cpp b/test/serialization/control_serialization.cpp index 06c572b7..b1638db9 100644 --- a/test/serialization/control_serialization.cpp +++ b/test/serialization/control_serialization.cpp @@ -11,6 +11,8 @@ #include <libcamera/control_ids.h> #include <libcamera/controls.h> +#include <libcamera/ipa/ipa_controls.h> + #include "libcamera/internal/byte_stream_buffer.h" #include "libcamera/internal/control_serializer.h" @@ -169,6 +171,113 @@ protected: return TestFail; } + /* Build a local (V4L2-like) ControlInfoMap and verify name round-trip. */ + vector<unique_ptr<ControlId>> v4l2ControlIds; + ControlIdMap v4l2IdMap; + constexpr uint32_t kV4L2TestControlId = 0x009a2001; + const string kV4L2ControlName = "V4L2_CID_TEST_GAIN"; + + v4l2ControlIds.emplace_back(std::make_unique<ControlId>( + kV4L2TestControlId, kV4L2ControlName, "v4l2", + ControlTypeInteger32, ControlId::Direction::In)); + v4l2IdMap.emplace(kV4L2TestControlId, v4l2ControlIds.back().get()); + + ControlInfoMap::Map v4l2Info; + v4l2Info.emplace(v4l2ControlIds.back().get(), + ControlInfo(ControlValue(int32_t{ 0 }), + ControlValue(int32_t{ 255 }), + ControlValue(int32_t{ 16 }))); + ControlInfoMap v4l2InfoMap(std::move(v4l2Info), v4l2IdMap); + + ControlSerializer v4l2Serializer(ControlSerializer::Role::Proxy); + ControlSerializer v4l2Deserializer(ControlSerializer::Role::Worker); + + size = v4l2Serializer.binarySize(v4l2InfoMap); + infoData.resize(size); + buffer = ByteStreamBuffer(infoData.data(), infoData.size()); + + ret = v4l2Serializer.serialize(v4l2InfoMap, buffer); + if (ret < 0 || buffer.overflow()) { + cerr << "Failed to serialize V4L2-like ControlInfoMap" << endl; + return TestFail; + } + + buffer = ByteStreamBuffer(const_cast<const uint8_t *>(infoData.data()), + infoData.size()); + ControlInfoMap v4l2InfoMapDes = + v4l2Deserializer.deserialize<ControlInfoMap>(buffer); + if (v4l2InfoMapDes.empty()) { + cerr << "Failed to deserialize V4L2-like ControlInfoMap" << endl; + return TestFail; + } + + auto idIt = v4l2InfoMapDes.idmap().find(kV4L2TestControlId); + if (idIt == v4l2InfoMapDes.idmap().end()) { + cerr << "Deserialized V4L2-like id map misses test control" << endl; + return TestFail; + } + + if (idIt->second->name() != kV4L2ControlName) { + cerr << "Deserialized V4L2-like control name doesn't match" << endl; + return TestFail; + } + + /* Reject malformed packets with over-sized names. */ + vector<uint8_t> badNameLenData = infoData; + auto *badNameLenHeader = + reinterpret_cast<ipa_controls_header *>(badNameLenData.data()); + auto *badNameLenEntry = reinterpret_cast<ipa_control_info_entry *>( + badNameLenData.data() + sizeof(*badNameLenHeader)); + badNameLenEntry->name_len = 2048; + + ControlSerializer badNameLenDeserializer(ControlSerializer::Role::Worker); + buffer = ByteStreamBuffer(const_cast<const uint8_t *>(badNameLenData.data()), + badNameLenData.size()); + if (!badNameLenDeserializer.deserialize<ControlInfoMap>(buffer).empty()) { + cerr << "Oversized control name should be rejected" << endl; + return TestFail; + } + + /* Reject malformed packets with non-null-terminated names. */ + vector<uint8_t> badTermData = infoData; + badTermData.back() = 'X'; + + ControlSerializer badTermDeserializer(ControlSerializer::Role::Worker); + buffer = ByteStreamBuffer(const_cast<const uint8_t *>(badTermData.data()), + badTermData.size()); + if (!badTermDeserializer.deserialize<ControlInfoMap>(buffer).empty()) { + cerr << "Control name without null terminator should be rejected" << endl; + return TestFail; + } + + /* Reject too-long names at serialization time. */ + vector<unique_ptr<ControlId>> longNameControlIds; + ControlIdMap longNameIdMap; + string longName(1025, 'n'); + + longNameControlIds.emplace_back(std::make_unique<ControlId>( + 0x009a2002, longName, "v4l2", ControlTypeInteger32, + ControlId::Direction::In)); + longNameIdMap.emplace(0x009a2002, longNameControlIds.back().get()); + + ControlInfoMap::Map longNameInfo; + longNameInfo.emplace(longNameControlIds.back().get(), + ControlInfo(ControlValue(int32_t{ 0 }), + ControlValue(int32_t{ 255 }), + ControlValue(int32_t{ 16 }))); + ControlInfoMap longNameInfoMap(std::move(longNameInfo), longNameIdMap); + + ControlSerializer longNameSerializer(ControlSerializer::Role::Proxy); + size = longNameSerializer.binarySize(longNameInfoMap); + infoData.resize(size); + buffer = ByteStreamBuffer(infoData.data(), infoData.size()); + + ret = longNameSerializer.serialize(longNameInfoMap, buffer); + if (ret != -EINVAL) { + cerr << "Too-long control name should fail serialization" << endl; + return TestFail; + } + return TestPass; } };
Bump IPA controls serialization format to v3 and add name_len to ControlInfoMap entries. Serialize null-terminated control names for local (V4L2-style) id maps to preserve names across IPC. Extend serializer tests with V4L2-like name round-trip coverage and malformed payload rejection for oversized length, missing terminator, and too-long serialize input. Signed-off-by: Magdum <magdum.foss@gmail.com> --- include/libcamera/ipa/ipa_controls.h | 6 +- src/libcamera/control_serializer.cpp | 133 +++++++++++++------ test/serialization/control_serialization.cpp | 109 +++++++++++++++ 3 files changed, 207 insertions(+), 41 deletions(-)