1

I am using memcpy in my program and used it as its syntax. But the function crashes. I read some post here and tried to initialize both my char arrays but I am not getting the problem here . Can anyone please look in it? 

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

int main(int argc, char *argv[]) {
    int i;
    char *str_result;
    char *str_dst = NULL;
    str_result = (char *) malloc(100);
    str_dst = (char *) malloc(100);

    str_result = "Action done";
    size_t len = strlen(str_result);
    printf("string length is = %d\n", len);
    memcpy(str_dst, str_result, len);
    str_dst [len] = '\0';
    for (i = 0; i < len; i++) {
        printf("%s\n", str_dst[i]);
    }
    free(str_dst);
    free(str_result);

    return 0;
}
Spikatrix
  • 20,225
  • 7
  • 37
  • 83
learningpal
  • 309
  • 1
  • 8
  • 17
  • Do you know which line it's crashing on? – KornMuffin Sep 24 '15 at 12:45
  • 3
    Use `strcpy` instead of assigning the pointers to string literals. And `printf ("string length is = %d\n", strlen);` --> `printf ("string length is = %zu\n", len);` – Spikatrix Sep 24 '15 at 12:46
  • No, I am actually trying to open .exe file, but that itself is not opening. – learningpal Sep 24 '15 at 12:48
  • @Cool Guy: I want to see the correct usage of memcpy, and ya its is "len". Thanks for that . :) – learningpal Sep 24 '15 at 12:51
  • 1
    @learningpal - I assume it probably *would* crash, seeing as you are `free()`ing the address of a string literal. Also, *please* don't cast the return of `malloc()`. – owacoder Sep 24 '15 at 12:51
  • @learningpal You got that part right. The bug is elsewhere. Simply change `str_result = "Action done";` to `strcpy(str_result, "Action done");` – Spikatrix Sep 24 '15 at 12:53
  • Please don't change your code after getting answers and comments as it invalidates them. – Spikatrix Sep 24 '15 at 13:28

4 Answers4

5

The error is here:

free (str_result);

After the line str_result = "Action done";, str_result no longer points to the 100-byte block of memory you reserved, but to the string (const char*) "Action done". You can't free this pointer.

r3mainer
  • 23,981
  • 3
  • 51
  • 88
4
  str_result = "Action done";

After this str_result does not point to the block which was allocated by malloc . Thus freeing memory not allocated by malloc or similar can cause error in program (one of my favourite statement).

Use strcpy also suggested in comments -

strcpy(str_result,"Action done");

And while printing str_dst -

for (i = 0; i < len; i++) {
    printf("%s\n", str_dst[i]);   // passing char where char * is expected
}

You don't need a loop to print str_dst .Simply this will work -

printf("%s",str_dst);

Corrected code (Demo)-

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

int main(int argc, char *argv[]) {
   int i;
   char *str_result;
   char *str_dst = NULL;
   str_result =  malloc(100);
   str_dst =  malloc(100);

   strcpy(str_result, "Action done");  // copy string at location where str_result points to 
   size_t len = strlen(str_result);
   printf("string length is = %zu\n", len);
   memcpy(str_dst, str_result, len);
   str_dst[len]='\0';    // manually add null character, memcpy won't add it 
   printf("%s\n", str_dst);

  free(str_dst);
  free(str_result);

  return 0;
}
ameyCU
  • 16,489
  • 2
  • 26
  • 41
  • I corrected the code as you suggested, and above is the edited version but I am still not able to open the .exe file. – learningpal Sep 24 '15 at 13:19
  • @learningpal You should not modify the original code in question . Don't do that ever . It creates confusion for others . – ameyCU Sep 24 '15 at 13:22
  • @learningpal Please see answer. I have included corrected code. – ameyCU Sep 24 '15 at 13:25
  • ya I got your point of not changing the original question. But I am bit confused that in above answers it has been mentioned that one cannot free the `str_result` but you are freeing it in your code. – learningpal Sep 24 '15 at 13:38
  • @learningpal That's because I didn't re-assign pointer . I copied string at that address using `strcpy`.Pointer still points to memory block allocated by `malloc`. – ameyCU Sep 24 '15 at 13:39
  • Yes, I got it. Thanks a lot. Quite appreciate it. – learningpal Sep 24 '15 at 13:51
3

Below your corrected code with memcpy

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

int main(int argc, char *argv[])
{
    int i;
    char *str_result;
    char *str_dst = NULL;
    str_result = (char *) malloc (100);
    str_dst = (char *) malloc (100);

    strcpy (str_result, "Action done");
    size_t len = strlen (str_result);
    printf ("string length is = %zu\n", len);

    memcpy(str_dst, str_result, len+1);

    for (i =0;i<len;i++)
    {
        printf( "%c\n", str_dst[i]);
    }

    free (str_dst);
    free (str_result);

    return 0;
}

Error in your code:

  1. There is a typo in str_result declaration: , instead of ;
  2. You must use strcpy to copy a string to a char *: strcpy (str_result, "Action done"); not str_result = "Action done";
  3. %c is the format to print chars, not %s.
  4. If you want to define len as size_t type you must use %zu format for printf.
LPs
  • 16,045
  • 8
  • 30
  • 61
2

You have the following bugs:

str_result = (char *) malloc(100);

Casting the result of malloc is pointless.

str_result = "Action done";

This is not how you assign strings in C. You must use strcpy(). Right now, you create a memory leak. Which is why free(str_result); later causes your program to crash, str_result is no longer pointing at the allocated memory.

printf("string length is = %d\n", len);

The correct format specifier for size_t is %zu.

printf("%s\n", str_dst[i]);

str_dst is a string, not an array of strings. It makes no sense to call it from a loop unless you intend to use %c to print it character by character.

Spikatrix
  • 20,225
  • 7
  • 37
  • 83
Lundin
  • 195,001
  • 40
  • 254
  • 396