C strings concatenation - strange characters

Question

Hi there.

I'm writing a C program which takes n strings and concatenates them using strcat. First I alloc'd the target string at sizeof(char)* the strlen of every string + 1 (for the null character). Then with a for I use strncat to create the final string. At the and, I am appending the null character.

Everything goes fine, but sometimes, at the beginning of the target string, there are some strange characters (for example, a '?'). This happens when, during the execution of the program, the final string is shorter than before (during the same execution).

Is there something I am missing?

This is the code:

size = 0;
for(i = 0; i < n; i++) {
    size += sizeof(char)*(strlen(strings[i]));
}

size++;

target = malloc(size);

if(!target) { /** Error handling... */ }

for(i = 0; i < n; i++) {
    target = strncat(target, strings[i], strlen(strings[i]));
}

target[size] = '\0';

Thanks,

—Donovan

Answer 1

You should first initialize the target string to an empty string right after allocating it.

target[0] = '\0';

On the first call to strncat, the first string will be appended to the target. If your target is not initialized, it may not be empty, this resulting in the garbage characters you see.

(Another solution is to copy the first of your strings with strncpy into the target, and then append the following strings to it.)

Answer 2

target[size] = '\0';

An array of size n has valid indices 0 to n - 1. You might want to change that to:

target[size-1] = '\0';

(Although this does not cause you starting garbage chars. For that, see Didier Trosset's answer.)

Some more tips for your code:

• sizeof (char) == 1 by definition, so it's save to omit.
• Since you know for sure the strings will fit, you can use plain strcat here. This will save you n calls to strlen.
• The final target[size-1] = '\0' is actually unneeded because str(n)cat always null-terminates its target.

Answer 3

By the way, each call to strncat() will have to traverse the string just to find where it will make the insertion. This makes your function O(n²). It's fairly simple to do better than that (changes to your code are in bold):

size = 0;
for(i = 0; i < n; i++) {
    size += strlen(strings[i]);
}

size++;

target = malloc(size);

if(!target) { /** Error handling... */ }

char *current_ptr = target;

for(i = 0; i < n; i++) {
    size_t len = strlen(strings[i]);
    memcpy(current_ptr, strings[i], len);
    current_ptr += len;
}

*current_ptr = 0;

Answer 4

Also you don't need to collect the return value of strncat back into target.