Concatenating two strings but getting null. please pont me error in this

Question

I am trying to copy two strings to a new pointer. this is to concatenate string pointer with sub pointer. Trying to make the output in new pointer str .what is wrong in this?

void addsubstring( char* string,  char* sub)
{
    //int m = strlen(string);
    char *str=(char*)malloc(sizeof(char)*10);
    int n =0;
    while( *string != '\0')
    {
        *(str) = *(string++);
        printf("char is %c\n", str[n++]);
        str++;
    }
        while( *sub != '\0')
    {
        *(str) = *(sub++);
        printf("char is %c\n", str[n++]);
        str++;``
    }
    printf("%s", str);
}

`str` points to the end of the string, not the beginning. Also, you never added a null terminator. — Barmar, Jun 11 '21 at 15:21
Do make sure the resulting string is less than 10 characters, or out-of-range access will happen. — MikeCAT, Jun 11 '21 at 15:24
This is an excellent Proof of Concept code for a [buffer overflow](https://en.wikipedia.org/wiki/Buffer_overflow) vulnerability. Please do not use it in production, or bad things will happen. — Victor - Reinstate Monica, Jun 11 '21 at 17:16

Vlad from Moscow · Answer 1 · 2021-06-11T18:32:54.880

For starters this memory allocation using the magic number 10 makes the function unsafe.

char *str=(char*)malloc(sizeof(char)*10);

In these while loops

while( *string != '\0')
{
    *(str) = *(string++);
    printf("char is %c\n", str[n++]);
    str++;
}
    while( *sub != '\0')
{
    *(str) = *(sub++);
    printf("char is %c\n", str[n++]);
    str++;``
}

the pointer str is being changed (incremented). So after the loops it points to a garbage.

And you forgot to include in the result string the terminating zero character '\0'.

The function can be defined the following way

char * addsubstring( const char *s1,  const char *s2 )
{
    size_t n1 = strlen( s1 );
    char *result = malloc( n1 + strlen( s2 ) + 1 );

    if ( result != NULL )
    {
        memcpy( result, s1, n1 );
        strcpy( result + n1, s2 );
    }

    return result;
}

Here is a demonstrative program

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

char * addsubstring( const char *s1,  const char *s2 )
{
    size_t n1 = strlen( s1 );
    char *result = malloc( n1 + strlen( s2 ) + 1 );

    if ( result != NULL )
    {
        memcpy( result, s1, n1 );
        strcpy( result + n1, s2 );
    }

    return result;
}

int main(void) 
{
    char *p = addsubstring( "Hello ", "World!" );
    
    if ( p ) puts( p );
    
    free( p );
    
    return 0;
}

The program output is

Hello World!

If you may not use standard C string functions then the function can be defined as it is shown in the demonstrative program below

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

char * addsubstring( const char *s1,  const char *s2 )
{
    size_t n = 0;
    
    while ( s1[n] ) ++n;
    while ( s2[n] ) ++n;
    
    char *result = malloc( n + 1 );

    if ( result != NULL )
    {
        char *p = result;
        while ( *s1 ) *p++ = *s1++;
        
        while ( ( *p++ = *s2++ ) );
    }

    return result;
}

int main(void) 
{
    char *p = addsubstring( "Hello ", "World!" );
    
    if ( p ) puts( p );
    
    free( p );
    
    return 0;
}

The program output is the same as shown above

Hello World!

Aside: Curious that code uses `memcpy( result, s1, n1 ); strcpy( result + n1, s2 );`. I would have expected 2 `strcpy()`, or better 2 `memcpy()`. Alignment optimization? — chux - Reinstate Monica, Jun 11 '21 at 17:39
@chux-ReinstateMonica strcpy is less efficient than memcpy. It could be written using two memcpy. U used the approach in the answer because I wanted only to know the offset in the destination string after the first copying. — Vlad from Moscow, Jun 11 '21 at 17:45

score 1 · Answer 2 · answered Jun 11 '21 at 15:21

The problem is the pointer str is slided. You should create another pointer to use as the origin. Also you will have to terminate the stirng by adding null-character before passing that for %s.

Also don't forget to free the allocated buffer, or a memory leak occurs.

Another point is that casting results of malloc() family is considered as a bad practice.

Fixed code:

void addsubstring( char* string,  char* sub)
{
    //int m = strlen(string);
    char *str=malloc(sizeof(char)*10);
    char *str_start = str;
    int n =0;
    while( *string != '\0')
    {
        *(str) = *(string++);
        printf("char is %c\n", str_start[n++]);
        str++;
    }
        while( *sub != '\0')
    {
        *(str) = *(sub++);
        printf("char is %c\n", str_start[n++]);
        str++;
    }
    *(str) = '\0';
    printf("%s", str_start);
    free(str_start);
}

could you please let me know why str is not working and need of bew pointer — , Jun 11 '21 at 15:50

Concatenating two strings but getting null. please pont me error in this

2 Answers2