22

I am using snprintf like this to avoid a buffer overrun:

char err_msg[32] = {0};
snprintf(err_msg, sizeof(err_msg) - 1, "[ ST_ENGINE_FAILED ]");

I added the -1 to reserve space for the null terminator in case the string is more than 32 bytes long.

Am I correct in my thinking?

Platform:

  • GCC 4.4.1
  • C99
Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
ant2009
  • 27,094
  • 154
  • 411
  • 609

6 Answers6

34

As others have said, you do not need the -1 in this case. If the array is fixed size, I would use strncpy instead. It was made for copying strings - sprintf was made for doing difficult formatting. However, if the size of the array is unknown or you are trying to determine how much storage is necessary for a formatted string. This is what I really like about the Standard specified version of snprintf:

char* get_error_message(char const *msg) {
    size_t needed = snprintf(NULL, 0, "%s: %s (%d)", msg, strerror(errno), errno);
    char  *buffer = malloc(needed+1);
    sprintf(buffer, "%s: %s (%d)", msg, strerror(errno), errno);
    return buffer;
}

Combine this feature with va_copy and you can create very safe formatted string operations.  

D.Shawley
  • 58,213
  • 10
  • 98
  • 113
  • 5
    Don't use strncpy() if there string might be too big to fit into the target; strncpy() does **NOT** null-terminate what it copies if it is too long. Further, it always copies N characters - even if the source is 1 byte and the target is 1 megabyte. – Jonathan Leffler Nov 22 '09 at 19:10
  • 1
    @Jonathan: Nope. while you are right that strncpy does not terminate with \0 it *does* stop if it does find a string terminator in the source string. – Nicholaz Nov 22 '09 at 21:15
  • 1
    For fixed length buffers, I usually use `strncpy(dest, src, sizeof(dest)); dest[sizeof(dest)-1] = '\0';` That guarantees NULL termination and is just less hassle than `snprintf` not to mention that a lot of people use `snprintf(dest, sizeof(dest), src);` instead and are very surprised when their programs crash arbitrarily. – D.Shawley Nov 22 '09 at 21:22
  • @JoãoPortela - what happens if src contains formatting codes? It crashes arbitrarily. – Ian Goldby May 21 '12 at 14:11
  • @IanGoldby you are correct. `snprintf(dest, sizeof(dest), "%s", src);` would work but it makes more sense to just use `strncpy` when you don't need formatting. – João Portela May 21 '12 at 14:42
  • 2
    This code has a bug: "Upon successful completion, the snprintf() function shall return the number of bytes that would be written to s had n been sufficiently large excluding the terminating null byte." Thus you have to allocate an additional byte for the terminating null. – Jeremy Salwen Aug 31 '12 at 06:38
  • 2
    @JeremySalwen - thanks for catching that... that's what I get for writing Java =) – D.Shawley Aug 31 '12 at 16:03
  • 1
    `snprintf(buffer, needed, …` is still a bug. You only allow snprintf to write `needed` bytes, but it needs to write `buffer+1` of them including the terminating `'\0'`, therefore you are forcing it to cut the contents short and to write the `'\0'` where the last character of contents should have been. It's a shame because you reserved room for the `'\0'` in your buffer. You just forbade `snprintf` to use it. – Pascal Cuoq Jun 13 '14 at 17:36
  • 2
    You could simply use `sprintf(buffer, …`, since you have reserved `buffer` of the exact right size. That would avoid this bug. – Pascal Cuoq Jun 13 '14 at 17:37
  • 1
    @PascalCuoq very good point.... not sure how I missed this one or how it hasn't been noticed for four and a half years – D.Shawley Jun 14 '14 at 11:14
14

You don't need the -1, as the reference states:

The functions snprintf() and vsnprintf() do not write more than size bytes (including the trailing '\0').

Note the "including the trailing '\0'" part

Eli Bendersky
  • 263,248
  • 89
  • 350
  • 412
10

No need for -1. C99 snprintf always zero-terminates. Size argument specifies the size of output buffer including zero terminator. The code, thus, becomes

char err_msg[32];
int ret = snprintf(err_msg, sizeof err_msg, "[ ST_ENGINE_FAILED ]");

ret contains actual number of characters printed (excluding zero terminator).

However, do not confuse with Microsoft's _snprintf (pre-C99), which does not null-terminate, and, for that matter, has completely different behaviour (e.g. returning -1 instead of would-be printed length in case if buffer is not big enough). If using _snprintf, you should be using the same code as in your question.

Alex B
  • 82,554
  • 44
  • 203
  • 280
3

According to snprintf(3):

The functions snprintf() and vsnprintf() do not write more than size bytes (including the trailing '\0').

Andrey Vlasovskikh
  • 16,489
  • 7
  • 44
  • 62
1

For the example given, you should be doing this instead:

char err_msg[32];
strncpy(err_msg, "[ ST_ENGINE_FAILED ]", sizeof(err_msg));
err_msg[sizeof(err_msg) - 1] = '\0';

or even better:

char err_msg[32] = "[ ST_ENGINE_FAILED ]";
Matt Joiner
  • 112,946
  • 110
  • 377
  • 526
  • 1
    As noted in a comment to another answer: Don't use strncpy() if there string might be too big to fit into the target; strncpy() does NOT null-terminate what it copies if it is too long. Further, it always copies N characters - even if the source is 1 byte and the target is 1 megabyte. – Jonathan Leffler Nov 22 '09 at 19:11
  • 1
    @Jonathan Leffler, your description of how many bytes `strncpy()` is incorrect. "At most n bytes of src are copied." I've adjusted the example to fix the null-termination. – Matt Joiner Nov 22 '09 at 21:07
  • @anacrolix: your example does not guarantee NULL termination. It does guarantee that `strncpy()` will never overwrite the last character in the buffer. You have to explicitly do `err_msg[sizeof(err_msg)-1] = '\0';` to get termination. – D.Shawley Dec 11 '09 at 12:20
-1

sizeof will return the number of bytes the datatype will use in memory, not the length of the string. E.g. sizeof(int) returns '4' bytes on a 32-bit system (well, depending on the implementation I guess). Since you use a constant in your array, you can happily pass that to the printf.

TheGrandWazoo
  • 2,879
  • 1
  • 17
  • 15
  • 1
    He's applying `sizeof` to the destination array, which is completely correct. – caf Nov 21 '09 at 13:14
  • 32 is correct. In that case he does not want the size of the string (which is given by strlen) he wants the `err_msg` buffer capacity (to guarantee it will not write past its end). – João Portela Jan 11 '11 at 18:03