Is sprintf(buffer, "%s […]", buffer, […]) safe?

Question

I saw use of this pattern to concatenate onto a string in some code I was working on:

sprintf(buffer, "%s <input type='file' name='%s' />\r\n", buffer, id);
sprintf(buffer, "%s</td>", buffer);

and I'm fairly certain it's not safe C. You'll notice that buffer is both the output and the first input.

Apart from the obvious possibility of a buffer overflow, I believe there is no guarantee that buffer doesn't get changed between the start and the end of the function (i.e., there is no guarantee as to what the state of buffer will be during the execution of the function). The signature of sprintf additionally specifies that the target string is restricted.

I also recall a report of a speculative writing in memcpy, and I see no reason why some C library might do the same thing in a sprintf. In this case, of course, it would be writing to its source. So is this behaviour safe?

FYI, I proposed:

char *bufEnd = buffer + strlen(buffer);
/* sprintf returns the number of f'd and print'd into the s */
bufEnd += sprintf(bufEnd, " <input type='file' name='%s' />\r\n", id);

to replace this.

Answer 1

From the glibc sprintf() documentation:

The behavior of this function is undefined if copying takes place between objects that overlap—for example, if s is also given as an argument to be printed under control of the ‘%s’ conversion.

It may be safe in a particular implementation; but you could not count on it being portable.

I'm not sure that your proposal would be safe in all cases either. You could still be overlapping buffers. It's late and my wife is buggin me but I think that you could still have the case where you want to use the original string again in the concatenated string and are overwriting the null character and so the sprintf implementation might not know where the re-used string ends.

You might just want to stick with a snprint() to a temp buffer, then strncat() it onto the original buffer.

Answer 2

sprintf() is not safe. Try snprintf() instead.

Answer 3

If you want to concatenate formatted text to the end of a buffer using printf(), I'd recommend you use an integer to keep track of the end position.

int i = strlen(buffer);
i += sprintf(&buffer[i], " <input type='file' name='%s' />\r\n", id);
i += sprintf(&buffer[i], "</td>");

or:

int i = strlen(buffer);
i += sprintf(&buffer[i], " <input type='file' name='%s' />\r\n", id);
strcat(&buffer[i], "</td>");

And before people go berserk downvoting this ("This isn't safe! You can overrun the buffer!"), I'm just addressing a reasonable way to build a formatted string in C/C++.