Skip to content

Conversation

@Reodus
Copy link

@Reodus Reodus commented Jun 9, 2025

This patch addresses a stack-based buffer overflow in the steamclient_dos_to_unix_path function.

The original implementation used strcpy to copy the src string into a fixed-size stack buffer (char buffer[4096]) without bounds checking. This could lead to buffer overflow if the input string exceeds the buffer size, potentially causing crashes or unexpected behavior.

This fix replaces the unsafe strcpy call with strncpy, and ensures null-termination by explicitly setting the last byte of the buffer to \0. This change mitigates the overflow risk while preserving the original logic of the function.

Summary of changes:

  • Replaced strcpy(dst, src) with strncpy(dst, src, sizeof(buffer) - 1)
  • Added dst[sizeof(buffer) - 1] = '\0' to guarantee null-termination

@jammy3855
Copy link

Per the man page for strncpy:

If the array pointed to by s2 is a string that is shorter than n
bytes, NUL characters shall be appended to the copy in the array
pointed to by s1, until n bytes in all are written.

This will copy unnecessary NUL characters if src is shorter than sizeof(buffer) - 1
If we look at strncat:

This function appends at most ssize non-null bytes from the array
pointed to by src, followed by a null character, to the end of the
string pointed to by dst.

strncat will not copy unnecessary NUL characters.

Per the Stack Overflow link below, the fix could look like:

dst[0] = '\0';
strncat( dst, src, sizeof(buffer - 1) );

But dst is already appropriately set a few lines up:

*dst = 0;

So the final fix would look like:

strncat( dst, src, sizeof(buffer - 1) );

Nevertheless, the current change does address the issue 😄

strncpy
strncat
Copying using strncat

@Reodus
Copy link
Author

Reodus commented Jul 17, 2025

Per the man page for strncpy:

If the array pointed to by s2 is a string that is shorter than n
bytes, NUL characters shall be appended to the copy in the array
pointed to by s1, until n bytes in all are written.

This will copy unnecessary NUL characters if src is shorter than sizeof(buffer) - 1 If we look at strncat:

This function appends at most ssize non-null bytes from the array
pointed to by src, followed by a null character, to the end of the
string pointed to by dst.

strncat will not copy unnecessary NUL characters.

Per the Stack Overflow link below, the fix could look like:

dst[0] = '\0';
strncat( dst, src, sizeof(buffer - 1) );

But dst is already appropriately set a few lines up:

*dst = 0;

So the final fix would look like:

strncat( dst, src, sizeof(buffer - 1) );

Nevertheless, the current change does address the issue 😄

strncpy strncat Copying using strncat

Good point about strncat — makes total sense and yeah, definitely cleaner than strncpy in this case. I’ve updated the patch to use that instead. Appreciate the suggestion!

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