How can I use strncat without buffer overflow concerns?

Question

I have a buffer, I am doing lot of strncat. I want to make sure I never overflow the buffer size.

char buff[64];

strcpy(buff, "String 1");

strncat(buff, "String 2", sizeof(buff));

strncat(buff, "String 3", sizeof(buff));

Instead of sizeof(buff), I want to say something buff - xxx. I want to make sure I never override the buffer

score 21 · Accepted Answer · answered Aug 01 '11 at 20:26

21

Take into consideration the size of the existing string and the null terminator

#define BUFFER_SIZE 64
char buff[BUFFER_SIZE];

//Use strncpy
strncpy(buff, "String 1", BUFFER_SIZE - 1);
buff[BUFFER_SIZE - 1] = '\0';

strncat(buff, "String 2", BUFFER_SIZE - strlen(buff) - 1);

strncat(buff, "String 3", BUFFER_SIZE - strlen(buff) - 1);

answered Aug 01 '11 at 20:26

Joe

56,979
9
128
135

4

buff[BUFFER_SIZE - 1] = '\0'; is not needed as strncat always zero terminates – fliX Dec 13 '14 at 15:03
4

@fliX strncpy doesn’t. If count is reached before the entire string src was copied, the resulting character array is not null-terminated. But for this specific example, where BUFFER_SIZE=64 and "String 1" is shorter, you’re right, it’s unneeded because strncpy will also copy the null: http://en.cppreference.com/w/cpp/string/byte/strncpy – Soonts Jul 18 '16 at 01:01

score 14 · Answer 2 · answered Aug 01 '11 at 21:11

14

Why not use snprintf? Unlike strncat it expects the size of the buffer, but more importantly, there's no hidden O(n).

Strcat needs to find the null-terminator on each string it concatenates, and each time run through the whole buffer to find the end. Each time the string gets longer, strcat slows down. Sprintf, on the other hand can keep track of the end. you'll find that

snprintf(buf, sizeof buf, "%s%s%s", "String1", "String2", "String3");

Is frequently a faster, and more readable soluton.

answered Aug 01 '11 at 21:11

Dave

10,964
3
32
54

That sounds good. However, I have several buffers/strings to be written to one global buffer. That also means to wait until I have all the strings/buffers available in order to use snprintf, else the buffer would be overwritten. – jscode Aug 03 '11 at 00:16
3

If you can't wait, `snprintf` returns the number of characters written, so you can store the buffer offset, allowing `offset+=snprintf(buf+offset, (sizeof buf)-offset, "%s", "String2")` – Dave Aug 03 '11 at 02:02
There is strength in this answer. strcat has an implicit search for the NULL terminator. – Xofo Dec 12 '16 at 13:10

AnT stands with Russia · Answer 3 · 2011-08-01T21:14:54.520

The way you use the strncat function in your orignal code would actually be appropriate for another function: strlcat (note l instead of n). The strlcat function is not standard, yet it is a popular implementation-provided replacement for strncat. strlcat expects the total size of the entire destination buffer as its last argument.

Meanwhile, strncat expects the size of the remaining unused portion of the target buffer as its third argument. For this reason, your original code is incorrect.

I would suggest that instead of doing that horrible abuse of strncpy and making explicit rescans with those strlen calls (both issues present in Joe's answer), you either use an implementation-provided strlcat or implement one yourself (if your implementation provides no strlcat).

http://en.wikipedia.org/wiki/Strlcpy

Hogan · Answer 4 · 2011-08-01T23:11:02.170

6

This is the best way to do it. sizeof() just gives you size of the pointer to the data if you don't allocate it locally (you did allocate locally in this case but better to do it this way and it will work if the code is re-factored).

#define MAXBUFFSIZE 64

char buff[MAXBUFFSIZE];

buff[0] = 0;  // or some string

strncat(buff, "String x",MAXBUFFSIZE - strlen(buff) - 1);

edited Aug 01 '11 at 23:11

answered Aug 01 '11 at 20:24

Hogan

69,564
10
76
117

In that case, `sizeof` will give him the size of the entire buffer since it's an array, not a dynamically allocated block of memory. – yan Aug 01 '11 at 20:33
@Hogan: Not true. When `sizeof` is applied to an array object, it evaluates to the total size of the array object. There's no "pointer to the data" of any kind in the OP's code. – AnT stands with Russia Aug 01 '11 at 21:13
I like that I got down voted and the accepted answer was stolen from mine since it was posted at least a minute after. – Hogan Aug 01 '11 at 23:10
@yan -- true points for a local buffer -- I modified to point out my true point. If it was re-factored to be allocated there would be ugly bugs. – Hogan Aug 01 '11 at 23:12
@andreyt - see above comment -- I can't notify 2 people it seems. – Hogan Aug 01 '11 at 23:12

score 1 · Answer 5 · answered Aug 01 '11 at 20:28

1

Hogan has answered the question sufficently; however, if you are worried about buffer overflows in strcat(...) you should equally be worried about buffer overflows in all the other string functions.

Use strnlen(...) and strncpy(...) to really make sure you stay within your buffer. If you don't have a strnlen(...) function, write it.

answered Aug 01 '11 at 20:28

Edwin Buck

69,361
7
100
138

1

Both `strnlen` and `strncpy` are functions that work with fixed-width strings. They have nothing to do with null-terminated strings. Meanwhile, the OP is interested in null-terminated strings specifically, as follows from the question. It is true, that one can often see `strncpy` being *misused* withr null-terminated strings, all right. But what is `strnlen` doing here is totally unclear to me. – AnT stands with Russia Aug 01 '11 at 21:06
Generally speaking the treatment of null terminated strings as being possibly bound by a fixed length string is what prevents a buffer overflow. When relying on null termination (by using the non-n function), you rely on the null terminator being only so far from the start of the string, which is a recipe for possibly overflowing the buffer you are copying into (if the assumption about the null terminating character doesn't hold). – Edwin Buck Aug 01 '11 at 21:22

score 0 · Answer 6 · answered Sep 29 '21 at 14:20

I'd use memccpy instead of strncat in this case - it's safer and much faster. (It's also faster than the approach with snprintf mentioned by Dave):

/**
 * Returns the number of bytes copied (not including terminating '\0').
 * Always terminates @buf with '\0'.
 */ 
int add_strings(char *buf, int len)
{
    char *p = buf;

    if (len <= 0)
        return 0;

    p[len - 1] = '\0'; /* always terminate */

    p = memccpy(buf, "String 1", '\0', len - 1);
    if (p == NULL)
        return len - 1;

    p = memccpy(p - 1, "String 2", '\0', len - 1 - (p - buf));
    if (p == NULL)
        return len - 1;

    p = memccpy(p - 1, "String 3", '\0', len - 1 - (p - buf));

    return (p == NULL ? len : p - buf) - 1;
}

How can I use strncat without buffer overflow concerns?

6 Answers6

Linked