Can `snprintf()` Read Out Of Bounds When A String Argument Is Not Null-terminated?

- 1 answer

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?



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 writeSize 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.