Why I get wrong result for custom concatenation function in c?

Question

I am developing a custom concatenation function (its an assignment for my collage) and I can't get it to work, when I print result of concatenation I get strange characters.

This is my code

typedef struct {
int len;
char *s;
} string_t;
typedef string_t* string;

void set (string* s1, char *s);
void concat (string* s1, string s2);

int main(void) {

//create variables
    string *str1;
    string *str2;

    set(str1,"hello ");
    set(str2,"world!");
    printf("\nconcatenate str1 and str2\n");
    concat(str1,*str2);

    printf("concatenation result is:\n");
    //the problem is here
    printf("%p , %s",(*str1)->s,(*str1)->s);

    printf("\n------End------\n");

    return EXIT_SUCCESS;
}
void set(string* s1, char *s){

    if(s1 != NULL){
        if(s == NULL){
            (*s1)->len = -1;
        }else{
            (*s1)->s = s;
            (*s1)->len = strlen(s);
        }
    }
}

void concat (string* s1, string s2){
    int totalLen = (*s1)->len + (*s2).len;
    char rslt[totalLen+1];
    int i=0;
    for(i=0;i<(*s1)->len;i++){
        rslt[i] = (*s1)->s[i];
    }
    for(i=(*s1)->len;i<totalLen;i++){
        int j=i-(*s1)->len;
        rslt[i] = (*s2).s[j];
    }
    rslt[totalLen] = '\0';

    set(s1,rslt);
    //there is no problem when printing here
    printf("%p , %s\n",(*s1)->s,(*s1)->s);
}

This is the result i get

concatenate str1 and str2

0023FE2C , hello world!

concatenation result is:

0023FE2C , ج@@

------End------

I am using eclipse IDE.

Can anybody help me fix this out?

Answer 1

Your concat function returns pointer to local array rslt. This array is valid only during invocation of enclosing function. After concat exits str1->s contains dangling pointer. Using it will cause undefined behavior.

One way to resolve this issue is to copy this array to heap allocated memory:

void set(string* s1, const char *s){
    free(s1->s);
    s1->s = strdup(s);
    s1->len = strlen(s);
}

Answer 2

sample of fix

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

typedef struct {
    int len;
    char *s;
} string;
//typedef string_t string;//no need pointer type.

void set (string *s1, const char *s);
void concat (string *s1, const string *s2);//Unified to pointer
string *create(const char *s);

int main(void) {
    string *str1 = create(NULL);//calloc(1, sizeof(*str1));//create("");//create("hello ");
    string *str2 = create(NULL);//calloc(1, sizeof(*str2));//create("");

    set(str1, "hello ");
    set(str2, "world!");
    printf("\nconcatenate str1 and str2\n");
    concat(str1, str2);

    printf("concatenation result is:\n");
    printf("%s", str1->s);

    printf("\n------End------\n");
    free(str1->s);free(str1);
    free(str2->s);free(str2);
    return EXIT_SUCCESS;
}

string *create(const char *s){
    string *str = malloc(sizeof(*str));
    if(str){
        if(s){
            str->len = strlen(s);
            str->s = strdup(s);//strdup isn't standard
/*
            str->s = malloc(str->len + 1);
            if(!str->s){
                free(str);
                str = NULL;
            } else {
                memcpy(str->s, s, str->len + 1);
            }
*/
        } else {
            str->len = -1;
            str->s = NULL;
        }
    }
    return str;
}

void set(string *s1, const char *s){
    if(s1 != NULL){
        free(s1->s);
        if(s == NULL){
            s1->s = NULL;
            s1->len = -1;
        }else{
            s1->s = strdup(s);
            s1->len = strlen(s);
        }
    }
}

void concat (string *s1, const string *s2){
    if(!s1 || !s2 || s1->len == -1 || s2->len == -1)
        return ;

    int totalLen = s1->len + s2->len;
    char rslt[totalLen+1];//char *rslt = malloc(totalLen+1);

    strcpy(rslt, s1->s);//use standard library
    memcpy(rslt + s1->len, s2->s, s2->len + 1);

    set(s1, rslt);
    //free(rslt);
}

Answer 3

// code readability is every bit as important as the algorithm used
// when compiling, have all warnings enabled, and then fix them
// OP can add parameter checking and make the str1 and str1 be pointers
//    with appropriate changes to the rest of the code
// I changed return types from void to int
// for set() and concat() so main could free the allocated memory areas
// the 'software contract' concept says the sub functions in the file
// do not need to check their parameters for validity


#include <stdio.h>
#include <stdlib.h>
#include <string.h> // strlen()
// strdup() should have been prototyped by string.h,
// but was not on my computer
char *strdup(const char *s); 


// dont clutter the code with frivilous typedef's
struct string_t
{
    int    len;
    char * s;
};

// prototypes
int concat (struct string_t*, struct string_t* );
int set( struct string_t*, char* );

int main(void)
{
    //create variables
    // your learning C, so keep it simple
    struct string_t str1 = {0,NULL};  // good programming practice to initialize local variables
    struct string_t str2 = {0,NULL};

    if( !set( &str1, "hello " ) )
    {
        if( !set( &str2, "world!" ) )
        {
            printf("\nconcatenate str1 and str2\n");
            if( !concat(&str1,&str2) )
            {
                printf("concatenation result is:\n");
                printf("%p , %s",(void*)&(str1.s), str1.s);

                printf("\n------End------\n");
            } //  end if
        } // end if
    } // end if

    free(str1.s);
    free(str2.s);
    return EXIT_SUCCESS;
} // end function: main


// <-- pString1 must point to an instance of struct string_t
// do not free() the pNewString
// as it is a pointer to a literal,
// not a pointer to allocated memory
int set( struct string_t* pString1, char* pNewString ) // <-- use meaningful/descriptive names
{
    int returnValue = 0; // indicate success


    char * temp = strdup(pNewString);
    if( NULL == temp )
    { // then strdup failed
        perror( "strdup failed" );
        returnValue = 1; // indicate failure
    }

    else
    { // else, strdup successful
        pString1->s   = temp;
        pString1->len = strlen(pString1->s)+1;
    }
    return( returnValue );
} // end function: set


int concat (struct string_t* pString1, struct string_t* pString2)
{
    int returnValue = 0; // indicate success

    int totalLen = pString1->len + pString2->len + 1;
    //printf( "\nbefore: string1->len =%i,string2->len=%d, totalLength=%i\n",
    //            pString1->len,
    //            pString2->len,
    //            totalLen);
    //printf("\nbefore: string1:%s, string2:%s\n",
    //            pString1->s, pString2->s);

    // <-- there is no room in string1->s for any more chars so:
    char * temp;
    if( NULL == (temp = realloc(pString1->s, totalLen) ) )
    { // then realloc failed
        perror( "realloc failed" );
        returnValue = 1; // indicate failure
    }

    else
    {
        free( pString1->s);
        pString1->s = temp;
        //printf("\n after realloc: str1.len:%i, strl.s:%s\n",
        //       pString1->len, pString1->s);

        int i=0;
        for(;i<totalLen;i++)
        {
            pString1->s[strlen(pString1->s)] = pString2->s[i];
            pString1->s[strlen(pString1->s)] = '\0';
        } // end for

        pString1->len = totalLen;
        pString1->s[totalLen] = '\0';

        //printf("after: str1addr:%p , str1:%s\n",pString1->s,pString1->s);
        //printf( "\nstring1->len =%i,string2->len=%d, totalLength=%i\n",
        //        pString1->len,
        //        pString2->len,
        //        totalLen);
    } // end if

    return( returnValue );
} // end function: concat