-
Notifications
You must be signed in to change notification settings - Fork 2
Return typed shared region pointers #42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 fixes undefined behavior in pointer arithmetic operations and improves the safety of a memory copy function. The main change addresses void pointer arithmetic by explicitly casting to uint8_t* before performing offset calculations, which is required by strict C standards. Additionally, the val_mem_copy function is enhanced with bounds checking to prevent buffer overflows.
- Fixed void pointer arithmetic by casting to
uint8_t*before adding offsets - Added destination buffer size parameter and bounds checking to
val_mem_copy - Updated all function signatures and call sites to match the new API
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/val_common_status.c | Fixed void pointer arithmetic in val_set_status and val_get_status by casting to uint8_t* |
| src/val_common_log.c | Updated val_mem_copy call to include destination buffer size parameter |
| src/val_common_framework.c | Enhanced val_mem_copy with destination size parameter and bounds checking logic |
| inc/val_common_framework.h | Updated val_mem_copy function signature to include dest_size parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return; | ||
|
|
||
| /* Clamp requested length so we never write beyond the destination buffer */ | ||
| size_t bytes_to_copy = len < dest_size ? len : dest_size; |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bounds check should account for null termination. If the caller expects a null-terminated string, this could write dest_size bytes and overflow when adding the null terminator. Consider using len < dest_size - 1 to reserve space for null termination, or document that this function does not null-terminate.
| if (len > 0 && msg[len - 1] == '\n') | ||
| { | ||
| val_mem_copy(formatted_msg, msg, len - 1); | ||
| val_mem_copy(formatted_msg, sizeof(formatted_msg), msg, len - 1); |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The length passed to val_mem_copy is len - 1, but the code then writes to formatted_msg[len - 1] and formatted_msg[len] after the copy. If len >= sizeof(formatted_msg), this will write beyond the bounds of formatted_msg since val_mem_copy will clamp to sizeof(formatted_msg) bytes but the subsequent writes assume len bytes were actually copied.
Summary
Testing