strncat causes buffer overflow

user_8184482 0
Viewed 485

how can I use strncat with heap objects?

Im trying to write a simple function to concatenate 2 strings together returning the result, however, I can not run it without making the return buffer very large (adding approximately an additional 5000 to its length) for it to not overflow.

I'm probably just using the strncat function incorrectly using heap objects instead of character arrays of fixed length. but I don't know how I would write it any other way.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

char *concatStrings(char *arg1, char *arg2) {
    char *ret = malloc(strlen(arg1) + strlen(arg2));
    strncpy(ret, arg1, strlen(arg1));
    strncat(ret, arg2, strlen(arg2));
    ret[strlen(arg1)+strlen(arg2)] = '\0';
    return ret;
}

int main(int argc, char *argv[]) {
    if (argc == 3) {
        char *print = concatStrings(argv[1], argv[2]);
        printf("\n%s", print);
        free(print);
    }
    return 0;
}
function-definition c-strings concatenation c
4 Answers

The first problem is, that you don't allocate enough memory, because in C a string is terminated by a 0-byte so it should be

char *ret = malloc((strlen(arg1) + strlen(arg2))+1);

And of couse you should check if malloc worked.

if (!ret)
    // error

The second problem is, that you use strncpy with a hardcoded length here. Just use strcpy because you already allocated enough memory. You can also use strcat for the same reason. strncat doesn't provide any additionaly benefit, and in fact makes the code slower because you, again, call strlen.

This

ret[10 + strlen(arg2)] = '\0';

is not needed at all. In fact its potentially dangerous if arg1 is less then 10 characters.

user_22820110

For starters the function should be declared like

char * concatStrings( const char* arg1, const char* arg2 );

because the strings pointed to by the pointers arg1 and arg2 are not being changed within the function.

In this memory allocation

char *ret = malloc(strlen(arg1) + strlen(arg2));

you forgot to reserve memory for the null terminating character '\0'. You have to write

char *ret = malloc( strlen(arg1) + strlen(arg2) + 1 );

Using the magic number 10 in this call

strncpy(ret,arg1,10);

does not make a sense.

If instead you will write for example

strncpy(ret,arg1,strlen(arg1));

then the next call

strncat(ret,arg2,strlen(arg2));

will invoke undefined behavior because the call strncpy did not append the null terminating character '\0' to the string pointed to by the pointer ret.

It would be much better just to write at least

strcpy( ret, arg1 );

In any case your function implementation is inefficient. For example there are two times called the function strlen for the parameter arg2

char *ret = malloc(strlen(arg1) + strlen(arg2));
//...
strncat(ret,arg2,strlen(arg2));

Also the call of strncat is also inefficient because the function needs to traverse the whole target string to find its terminating zero.

The function can be defined the following way

char * concatStrings( const char* arg1, const char* arg2 )
{
    size_t n1 = strlen( arg1 );
    char *ret = malloc( n1 + strlen( arg2 ) + 1 );

    if ( ret != NULL )
    {
        strcpy( ret, arg1 );
        strcpy( ret + n1, arg2 );
    }

    return ret;
}

Here is a demonstrative program.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

char * concatStrings( const char* arg1, const char* arg2 )
{
    size_t n1 = strlen( arg1 );
    char *ret = malloc( n1 + strlen( arg2 ) + 1 );

    if ( ret != NULL )
    {
        strcpy( ret, arg1 );
        strcpy( ret + n1, arg2 );
    }

    return ret;
}

int main(void) 
{
    const char *argv1 = "Hello ";
    const char *argv2 = "World!";
    
    char *print = concatStrings( argv1, argv2 );
    
    if ( print != NULL ) puts( print );
    
    free( print );
    
    return 0;
}

The program output is

Hello World!

It would be even better to substitute the first call of strcpy for memcpy within the function. That is the function can also look like

char * concatStrings( const char* arg1, const char* arg2 )
{
    size_t n1 = strlen( arg1 );
    char *ret = malloc( n1 + strlen( arg2 ) + 1 );

    if ( ret != NULL )
    {
        memcpy( ret, arg1, n1 );
        strcpy( ret + n1, arg2 );
    }

    return ret;
}
user_28772410

I forget snprintf existed and thought I had to use either strcpy strcat or some other string or memory management function to do this

char* concatStrings(char* arg1, char*arg2){
  size_t n1 = strlen(arg1), n2 = strlen(arg2);
  char *ret = malloc(n1 + n2 + 1);
  snprintf(ret,n1 + n2 + 1, "%s%s", arg1, arg2);
  return ret;
}
user_81844820

Problems

  1. Not allocating space for the null character. Off by 1.

  2. strncpy(ret, arg1, strlen(arg1)) does not null character terminate ret, so the next strncat(ret,... is undefined behavior as ret needs to point to a string.

Some alternatives:

// Via sprintf
// Good compilers will emit efficient code 
//   and not perform the usual parsing overhead of *printf format at run time
char *concatStrings_alt1(const char *arg1, const char *arg2) {
  char *ret = malloc(strlen(arg1) + strlen(arg2) + 1);
  sprintf(ret, "%s%s", arg1, arg2);
  return ret;
}

// Via memcpy
char *concatStrings_alt2(const char *arg1, const char *arg2) {
  size_t len1 = strlen(arg1);
  size_t size2 = strlen(arg2) + 1;
  char *ret = malloc(len1 + size2);
  memcpy(ret, arg1, len1);
  memcpy(ret + len1, arg2, size2);
  return ret;
}
user_24103590
Related