Skip to content

lib: fix heap buffer overflow in dlt_with_filename_and_line_number#865

Open
SoundMatt wants to merge 1 commit into
COVESA:masterfrom
SoundMatt:fix/dlt-with-filename-heap-overflow
Open

lib: fix heap buffer overflow in dlt_with_filename_and_line_number#865
SoundMatt wants to merge 1 commit into
COVESA:masterfrom
SoundMatt:fix/dlt-with-filename-heap-overflow

Conversation

@SoundMatt

Copy link
Copy Markdown

Problem

dlt_with_filename_and_line_number() in src/lib/dlt_user.c writes
past the end of a heap allocation when strlen(fina) > 255:

dlt_user.filenamelen = (uint8_t)strlen(fina);          // truncates
...
dlt_user.filename = malloc(dlt_user.filenamelen + 1);  // small
strcpy(dlt_user.filename, fina);                       // full

filenamelen is uint8_t (matches the V2 wire-format field width
declared in include/dlt/dlt_user.h.in:235), so the cast truncates
strlen(fina) modulo 256. The subsequent malloc allocates the
truncated length + 1, but strcpy copies the entire fina including
its real terminator — writing strlen(fina) - filenamelen bytes past
the end of the heap chunk.

Reachable in practice via long compile-time __FILE__ paths
(monorepo-rooted absolute paths regularly exceed 255 chars) and any
application that forwards a user-controlled filename through this
function. Heap-write primitive in the linked-into-everything client
library.

Fix

Clamp strlen(fina) to UINT8_MAX before assigning filenamelen,
allocate against the clamped length, and copy with memcpy + explicit
null terminator instead of strcpy. The on-wire field still gets the
maximum the V2 protocol can encode.

Notes

  • I have not added a unit test in this PR. If maintainers can point me
    at a harness for dlt_user.c (e.g. via gtest_dlt_user.cpp) I'm
    happy to follow up with one.
  • Per-call filename length (dlt_user_param.filenamelen at line
    6555) appears to use the same uint8_t width. Worth a separate
    audit pass to confirm callers can't trigger the same pattern; not
    in this PR's scope.

@minminlittleshrimp minminlittleshrimp left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The PR is good in Root cause Analysis, please rework with strncpy and then we can merge.
Thank you

Comment thread src/lib/dlt_user.c Outdated
* Clamp the source length to UINT8_MAX before truncation so that
* the malloc + copy below cannot write past the end of the heap
* allocation when strlen(fina) > 255. */
size_t fina_len = strlen(fina);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

remove reinit of this var, this var is function opt agr

Comment thread src/lib/dlt_user.c Outdated
return DLT_RETURN_ERROR;
}
strcpy(dlt_user.filename, fina);
memcpy(dlt_user.filename, fina, fina_len);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This pattern is not good.
filename is a string, use strncpy for truncation here + 1 null padding in next line is the common pattern.
e.g.

strncpy(dlt_user.filename, fina, fina_len-1);
dlt_user.filename[fina_len] = '/0';

When the source filename string is longer than 255 bytes,
dlt_with_filename_and_line_number(fina, linr) overflows the heap
buffer it allocates for dlt_user.filename:

  dlt_user.filenamelen = (uint8_t)strlen(fina);          // truncates
  ...
  dlt_user.filename = malloc(dlt_user.filenamelen + 1);  // small
  strcpy(dlt_user.filename, fina);                       // full

filenamelen is uint8_t (matching the V2 wire-format field width
declared in include/dlt/dlt_user.h.in), so the cast truncates
strlen(fina) modulo 256. The subsequent malloc allocates the
truncated length + 1, but strcpy copies the entire fina including
its real terminator, writing strlen(fina) - filenamelen bytes past
the end of the heap chunk.

Reachable in practice via long compile-time __FILE__ paths
(monorepo-rooted absolute paths) and any application that forwards
a user-controlled filename through this function.

Fix: clamp strlen(fina) to UINT8_MAX before assigning filenamelen,
allocate against the clamped length, and copy with memcpy + explicit
null terminator instead of strcpy. The on-wire field still gets the
truncated length, which matches the maximum the V2 protocol can
encode.

Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com>
@SoundMatt SoundMatt force-pushed the fix/dlt-with-filename-heap-overflow branch from c7e904c to 6f4d59f Compare June 15, 2026 15:01
@SoundMatt

Copy link
Copy Markdown
Author

Thanks for the detailed feedback! Applied both changes in the latest push:

  1. Removed fina_len intermediate variablefilenamelen is now set directly via a ternary that clamps at UINT8_MAX, eliminating the separate local variable.
  2. Switched to strncpy — replaced memcpy + explicit null with strncpy(dlt_user.filename, fina, (size_t)dlt_user.filenamelen) followed by the explicit null terminator at [filenamelen], which is the conventional C string-truncation idiom.

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