trying to make a function of 2 strings combined into a new one

Question

Is the code fine? When I use puts(nstr) in the function I do get the right result , but when on main all I get is "riends" output. using a Microsoft Visual C++ Express if it helps.

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

char* strcpy2 (char* str1, char* str2)
{
    char nstr[20];
    int i,j;
    for (i = 0; str1[i] != '\0'; i++)
    {
        nstr[i] = str1[i];
    }
    for (j = i, i = 0; str2[i]!='\0'; i++, j++)
    {
        nstr[j] = str2[i];
    }
    nstr[j] = '\0';
    return nstr;
}

void main()
{
    char str1[10] = "Hello";
    char str2[10] = ",friends";
    puts(strcpy2(str1, str2));
}

Answer 1

Summary

Welcome to SO. The main issue is that you're returning the address of a variable that has gone out of scope by the time you try to use it.

Detail

When functions are invoked, local variables are pushed onto a stack (not the data structure!). Here, your local nstr array variable is pushed and is said to be defined within the function's scope.

However, when functions return, local variables are destroyed along with the stack on which they were placed. This means that your nstr variable, has already gone out of scope by the time your function has returned.

This causes your caller, main in this case, to end up with an invalid reference to memory that is no longer in use by the program, which can trigger all sorts of bugs and crashes.

Instead, you should pass a 3rd argument to your function so that it serves as the place where the new concatenated string will go. I don't think making the variable static here is necessary or a good idea. There's no need to keep something in memory when it shouldn't be.

Updated Code

Based on this, your code should look more like this

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

void strcpy2 (char str1[], char str2[], char str3[])
{
    // ...
}

int main(void)
{
    char str1[10] = "Hello";
    char str2[10] = ",friends";
    char str3[20];
    strcpy2(str1, str2, str3);
    puts(str3);
    return 0;
}

You should be able to take it from here.

Update - Why You Should Not Use static Here

Many have recommended using the static keyword here. I advice against this for the following reasons. Using static for your local variable causes your function to be:

1. Not Thread-Safe: static variables are shared variables;
2. Not Reusable: function breaks trying to use strings longer than your toy example with a cap of 19+1 chars including null and defeats the purpose of having a function in the first place;
3. Not Memory Efficient: the static variable will remain in memory with a string that no longer needs to be used until your program exits

You'd have a better-quality function if you pass the 3rd argument as a destination that already has enough space to contain the originals to be concatenated.

You could return a pointer that has been malloc'ed, but note that this is a bit more dangerous and will require more care from everyone using your function. For example, it must be unambiguously clear to every client that it's now their responsibility to free the memory returned by your function. This can be more error-prone because the mallocs are not immediately visible to those using it. (Forgot to free your function's internal/invisible malloc? Whoops! Hello memory leak!).

Using the 3rd param should be safer in general.

You should really take the time to understand the side-effects and consequences that follow from decisions like these.

Answer 2

nstr is local to function strcpy2. Returning pointer to automatic local variable invokes undefined behavior. Change it to

static char nstr[20]; 

or use malloc to allocate memory dynamically.

Answer 3

The way you're doing this will not work. For one thing your variable nstr is locally defined and it will not be any good after you exit the function. I could show you how, but I'm guessing you're a student and need to hit the books a little harder. You can do this, it ain't that hard.

Answer 4

I think a pointer with malloc will do the job here, try this:

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

char *strcpy2 (char *str1, char *str2){
    char *result;
    size_t length = 0,i,j;

    length = strlen(str1) + strlen(str2);
    result = malloc(length + 1);

    for (i = 0; str1[i] != '\0'; i++){
        result[i] = str1[i];
    }

    for (j = i, i = 0; str2[i]!='\0'; i++, j++){
        result[j] = str2[i];
    }

    result[j] = '\0';
    return result;
}

int main(void){
    char str1[10] = "Hello";
    char str2[10] = ",friends";

    char *res = strcpy2(str1, str2);
    printf("%s\n",res);

    free(res);
    return 0;
}

Output:

Hello,friends

Answer 5

if you print it using printf("%s", strcpy2(str1, str2)); it will be fine. I dont know whats wrong with puts