Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

object_detect_udp_stage: fix name buffer overread and GCC 12 false positive#918

Open
Kletternaut wants to merge 2 commits into
raspberrypi:mainraspberrypi/rpicam-apps:mainfrom
Kletternaut:fix/object-detect-udp-name-bufferKletternaut/rpicam-apps:fix/object-detect-udp-name-bufferCopy head branch name to clipboard
Open

object_detect_udp_stage: fix name buffer overread and GCC 12 false positive#918
Kletternaut wants to merge 2 commits into
raspberrypi:mainraspberrypi/rpicam-apps:mainfrom
Kletternaut:fix/object-detect-udp-name-bufferKletternaut/rpicam-apps:fix/object-detect-udp-name-bufferCopy head branch name to clipboard

Conversation

@Kletternaut

Copy link
Copy Markdown
Contributor

Problem

In ObjectDetectUDPStage::Process() the name field of each detection was serialised with a hard-coded memcpy of 253 bytes:

std::array<uint8_t, name_length> name;
memcpy(name.data(), detection.name.c_str(), name_length - 2);  // always 253 bytes
name[name_length - 1] = '\0';                                   // only byte 254

Three bugs:

  1. Buffer overread: detection.name is typically a short label (e.g. "person", 6 bytes). memcpy always reads 253 bytes, reading well past the end of the string — undefined behaviour.
  2. Uninitialised bytes: the array is not zero-initialised, so bytes between name.size() and byte 253 contain stack garbage sent over the UDP socket.
  3. GCC 12 false positive: append<std::array<uint8_t,255>> triggers a spurious -Wstringop-overflow / -Warray-bounds diagnostic from vector::resize() inlined deep in stl_algobase.h. The access is entirely within bounds; GCC's alias analysis loses track of the allocation origin.

Fix

  • Zero-initialise the array (name{}).
  • Copy only min(detection.name.size(), name_length - 1) bytes.
  • The NUL terminator is already present from zero-initialisation; no explicit name[254] = '\0' needed.
  • Suppress the GCC 12 false positive with a narrow #pragma GCC diagnostic pair around the append template definition, with a comment explaining the root cause.

The on-wire format is unchanged — the receiver still reads exactly name_length (255) bytes for the name field.

Testing

Compiled with GCC 12.2.0, -O3 -Wall -Wextra -Warray-bounds=2 (OpenCV enabled) — no warnings.

…sitive

The name field was unconditionally copied using a fixed length of 253
bytes regardless of the actual string length.  If detection.name is
shorter than 253 characters the memcpy reads past the end of the string
data, causing undefined behaviour.

Fix by zero-initialising the name array and limiting the copy to
min(name.size(), name_length - 1) bytes.  The trailing NUL terminator
is already provided by the zero-initialisation.

GCC 12 additionally emits a spurious -Wstringop-overflow / -Warray-bounds
diagnostic originating from vector::resize() inlined inside the append()
helper when T = std::array<uint8_t,255>.  The accesses are entirely
within bounds; suppress the false positive with a narrow diagnostic
pragma around the template definition.

Signed-off-by: Kletternaut <tomge68@gmail.com>
Clang does not recognise -Wstringop-overflow and rejects the pragma
with -Werror,-Wunknown-warning-option, breaking the clang CI build.
Wrap the diagnostic push/pop pair in a #if defined(__GNUC__) &&
!defined(__clang__) guard so the pragmas are only seen by GCC.

Signed-off-by: Kletternaut <tomge68@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Morty Proxy This is a proxified and sanitized view of the page, visit original site.