Getting empty when using my own strcopy function

Question

void strcopy(char *src, char *dst) {
  int len = strlen(src) - 1;
  dst = (char*)malloc(len);
  while(*src){
    *dst = *src;
     src++; dst++;
  }
  *dst = '\0';
}
int main() {
  char *src = "hello";
  char *dst = "";
  strcopy(src, dst);
  printf("strcpy:%s\n", dst);
  return 0;
}

Output:
strcpy:

Can anyone please explain what's wrong in my strcopy function? But I am getting the output as correct if do malloc in main. Confused, as malloc allocates memory in heap only right?

Answer 1

In your case,

  strcopy(src, dst);

dst itself is passed by value. You cannot allocate memory inside your function and expect the same to be reflected in caller function.

If you want memory to be allocated to dst inside strcopy(), you need to either

• pass a pointer to dst.

• or you can also return the newly allocated dst from your function and collect the same back into dst in the caller.

  However, please bear in mind, as you're allocating the memory yourself, it is your duty to free() it.

Otherwise, you can allocate memory to dst inside your caller (main()) function and then pass the already-allocated dst to your function to perform the copying job. IMHO, this approach has more resemblance to the library function strcpy() as the API itself does not handle the allocation of memory for the destination.

---

Note:: That said, in the code

 int len = strlen(src) - 1;

you're facing a logical error. It should rather be

 int len = strlen(src) + 1;

to accumulate the terminating null, also.

Answer 2

There are a few problems there:

1. When you do this in your strcopy:

   dst = (char*)malloc(len);
   

   you're overwriting the pointer in the dst argument with a new pointer. This has no effect at all on the dst variable in main, because its value, not a reference to it, was passed into strcopy. So naturally when you print out the string that main's dst points to, it's unchanged, and still "".

2. You're not allocating enough memory for the copy. You've done:

   int len = strlen(src) - 1;
   dst = (char*)malloc(len);
   

   which allocates two bytes too little. You need + 1, not - 1: The number of characters in the source string (strlen(src)) plus one for the terminating null character.

3. You're allocating memory you never release.

   Remember, it was written thus:

   As Ye malloc, so shall Thee free

#2 is easily fixed by using + 1 instead of - 1.

#1 is fixed in any number of ways:

1. You could have your strcopy allocate the memory for the copy, then don't pass dst in, and have it return the pointer it allocated. Its documentation would say that it did an allocation the caller is expected to deallocate. E.g.:

   char * strcopy(char *src) {
     int len = strlen(src) + 1;
     char * dst = (char*)malloc(len);
     for (p = dst; *src; ++p, ++src) {
       *p = *src;
     }
     *p = '\0';
     return dst;
   }
   int main() {
     char *src = "hello";
     char *dst = strcopy(src);
     printf("strcopy:%s\n", dst);
     free(dst);                      // <== note
     return 0;
   }
   

2. You could write to the memory main's dst points to, but you'd need to be sure that you allocate enough. Typically in C if you're passing a buffer into a function that should write to the buffer, you also pass the size of the buffer so the function knows where to stop.

Answer 3

There are two things wrong in your code.

Firstly, your strcopy() function allocates strlen(src) - 1 characters to dst, and then copies strlen(src) + 1 characters to it. That gives undefined behaviour.

Second, your main() passes its local dst by value (i.e. the address of the first character in the string literal "". Changing the value of dst in strcopy() therefore has no effect on the value of dst in main(). The fact that the two variables (in main() and in strcopy()) have the same name does not change that.

Answer 4

Summarizing the comments and proposals of others, your code could be as follows:

void strcopy(char *src, char **_dst) {
  int len = strlen(src) + 1;
  char *dst = malloc(len);
  *_dst= dst;
  while(*src){
    *dst = *src;
     src++; dst++;
  }
  *dst = '\0';
}
int main() {
  char *src = "hello";
  char *dst;
  strcopy(src, &dst);
  printf("strcpy:%s\n", dst);
  return 0;
}

Answer 5

Here is what you are looking for:

void strcopy(char **src, char **dst) {
  int len = strlen(*src) + 1;
  *dst = (char*)malloc(len);
  int j;
  for(j=0;j<strlen(*src);j++)
  {
    dst[j] = src[j];
  }
  dst[j] = '\0';
}

int main() {
  char *src = "hello";
  char *dst = "";

  strcopy(&src, &dst);
  printf("strcpy:%s\n", dst);
  return 0;
}

Answer 6

Function parameters are local variables of the function. A function deals with copies of its arguments. So any changes of a parameter within the function do not influence on the original value of the corresponding argument.

So for example this statement in function strcopy

dst = (char*)malloc(len);

does not change the argument dst defined in main.

char *dst = "";

If you want to write indeed a function named like strcopy when the function should deal with already allocted arrays. Otherwise the function should be named like for example strdup if it itself creates a new copy of the source string.

Moreover your function is wrong because it can try to allocate a memory with a negative memory size when the source string is empty and therefore strlen(src) is equal to 0. And in any case it allocates less memory than it is required.

int len = strlen(src) - 1;
dst = (char*)malloc(len);

The function can be defined the following way

char * strcopy( char *dst, const char *src ) 
{
    char *p = dst;

    while ( ( *p++ = *src++ ) );

    return dst;
}

and called like

int main( void ) 
{
    char *src = "hello";
    char dst[6];

    printf( "strcpy:%s\n", strcopy( dst, src ) );

    return 0;
}

Take also into account that you may not change string literals in C (and C++). Any attempt vto change a string literal results in undefined behaviour of the program.

Thus you may not declare variable dst like for example

char *dst = "A string literal";

and call function strcopy passing dst as the first argument in the above program.

If you want to create a copy of a source string then the function can be defined the following way

char * strdup( const char *src ) 
{
    char *dst = malloc( ( strlen( src ) + 1 ) * sizeof( char ) );

    if ( dst )
    {
        char *p = dst;
        while ( ( *p++ = *src++ ) );
    }

    return dst;
}

and called like

int main( void ) 
{
    char *src = "hello";
    char *dst;

    dst = strdup( src ) ;
    printf( "strcpy:%s\n", dst );

    free( dst );

    return 0;
}