Skip to content

Conversation

@jk-arm
Copy link
Contributor

@jk-arm jk-arm commented Dec 17, 2025

Summary

  • add a destination-size parameter to val_mem_copy and clamp copies to it
  • update the only caller to pass the buffer span

Testing

  • Not run (not requested)

Copilot AI review requested due to automatic review settings December 17, 2025 10:55
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds buffer overflow protection to the val_mem_copy function by introducing a destination size parameter and clamping copy operations to prevent writing beyond buffer boundaries. It also adds format string checking attributes for val_printf on GCC and Clang compilers.

  • Added dest_size parameter to val_mem_copy with bounds checking
  • Updated the single caller in val_common_log.c to pass the destination buffer size
  • Added compiler-specific format string checking attributes to val_printf

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/val_common_framework.c Implements bounds checking in val_mem_copy by adding dest_size parameter and clamping logic
inc/val_common_framework.h Updates function signature to include new dest_size parameter
src/val_common_log.c Updates caller to pass sizeof(formatted_msg) as destination size
inc/val_common_log.h Adds format attribute macro for compile-time printf format checking

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +183 to +184
if (len > dest_size)
len = dest_size; /* Clamp to prevent writing past dest */
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The clamping logic should account for null termination. When len equals dest_size, the code will write up to the last byte of the destination buffer, leaving no room for a null terminator if one is expected. Consider clamping to dest_size - 1 if the destination is intended to be a null-terminated string, or document that this function does not null-terminate.

Copilot uses AI. Check for mistakes.
Comment on lines +635 to 638
val_mem_copy(formatted_msg, sizeof(formatted_msg), msg, len - 1);
formatted_msg[len - 1] = '\r';
formatted_msg[len] = '\n';
formatted_msg[len + 1] = '\0';
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the call to val_mem_copy, lines 636-638 write to indices len - 1, len, and len + 1 without checking if these indices are within the bounds of formatted_msg. If len is close to sizeof(formatted_msg), this could write beyond the buffer. The bounds check should be performed before these operations.

Copilot uses AI. Check for mistakes.
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.

2 participants