Can `snprintf()` read out of bounds when a string argument is not null-terminated?

user_5281528 0
Viewed 228

I have the following piece of code, which a colleague claims may contain an out-of-bounds read, which I do not agree with. Could you help settle this argument and explain why?

char *test_filename = malloc(Size + 1);
sprintf(test_filename, "");

if (Size > 0 && Data)
  snprintf(test_filename, Size + 1, "%s", Data);

where Data is a non-null-terminated string of type const uint8_t *Data and Size is the size of Data, i.e., number of bytes in Data, of type size_t.

It may read out-of-bounds because the format string is %s, perhaps?

printf c string
2 Answers

Your colleague is correct. Perhaps unintuitively, snprintf(test_filename, Size + 1, "%s", Data) is guaranteed to read bytes starting at Data until a 0 byte is encountered, in your case typically resulting in an out-of-bounds read.

It will only write Size of these bytes to test_filename and null terminate them, respecting the size limit of the destination; but it will continue to read on. The reason for that is a design choice which enables the caller to determine the needed destination size for dynamic allocation before anything is actually written: snprintf() returns the number of bytes which would be written if the destination had infinite space. This feature is supposed to be used with a destination size of 0 (and potentially a null pointer as the destination). This functionality is useful for arguments which are not strings: With numbers etc. the size of the output is difficult to predict (e.g. locale dependent) and best left to the function at run time.

At the same time the return value indicates whether the output was truncated: If it is greater or equal to the size parameter, not all of the input was used in the output. In your case, what was left out were the bytes starting a Data[Size] and ending with the first 0 byte, or a segmentation fault ;-).

Suggestion for a fix: First of all it is unclear why you would use the printf family to print a string; simply copy it. And then Andrew has a point in his comments that since Datais not null terminated it is not really a string (even if all bytes are printable); so don't start fiddling with strcpy and friends but simply memcpy() the bytes, and null terminate manually.

Oh, and the preceding sprintf(test_filename, ""); does not serve any discernible purpose. If you want to write a null byte to *Data, simply do so; but since you are not using strcat, which would rely on a terminated destination string to extend, it is quite unnecessary.

user_31508020

from the MAN page for snprintf()

The  functions  snprintf()  and  vsnprintf()  write  at most size bytes (including the terminating null byte ('\0')) to str.

Note the at most size bytes

This means that snprintf() will stop transferring bytes after the parameter Size bytes are transferred.

this statement;

sprintf(test_filename, "");

is completely unneeded and has no effect on the operation of the second call to snprintf()

If you want to result in a 'proper' string, suggest:

char *test_filename = calloc( sizeof( char ), Size + 1);

if (Size > 0 && Data)
    snprintf(test_filename, Size, "%s", Data);

however, the function: snprintf() keeps reading until a NUL byte is encountered. This can create problems, upto and including a seg fault event.

The function: memcpy() is made for this kind of job. Suggest replacing the call to snprintf() with

memcpy( test_filename, Data, Size );
user_36292490
Related