C Access Violation error

Question

When I try to execute the following code in Visual Studio I get a data access error. I looked it up but saw that it happens when I try to modify a string literal which is a read only.

char *my_strcpy(char *dest, const char *source)
{
    int i;
    while(*dest)
    {
        *dest++ = *source++;
    }
    return dest;
}

 int main()
 {
    char str[80] = "Hello, there!";
    char *strPtr = NULL;

    my_strcpy (strPtr, str);    

    return 0;
}

error:
Unhandled exception at 0x01163C91 in PtrStrArr.exe: 0xC0000005: Access violation reading location 0x00000000.

I am unable to identify where I am modifying a string literal. I even declared the array as a constant. If I remove the declaration

  char *strPtr = NULL;

I get an error

Run-Time Check Failure #3 - The variable 'strPtr' is being used without being initialized.

Could you please point me to my mistake here?

EDIT:

Thanks to all the answers I was able to correct my code and get the ouput as below.

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

 char *my_strcpy(char *dest, const char *source)
 {
    char *retVal = dest;
    int i=0;
    while(*source != '\0')
    {
        *dest++ = *source++;
    }
    *dest++ = '\0';
    return retVal;
  }

  void print_array(char *str)
  {
    int i;
    while(*str)
    {
        printf("%c",*str++);
    }
  }

  int main()
  {
    char str[80] = "What up dude?";
    char* strPtr;
    strPtr = (char *)(malloc(sizeof str));
    print_array (str);
    printf("\n");
    my_strcpy (strPtr, str);    
    print_array (strPtr);
    return 0;
  }

I do have one question though. In all your answers you have not made the cast from void* to char* when using malloc. But when I tried it out I had to explicitly

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

I am using malloc in the right way? Thanks to every one for your answers.

Answer 1

it happens when I try to modify a string literal which is a read only

That's just one of the many ways it can happen. Another is trying to store into NULL, which is what you're doing here:

char str[80] = "Hello, there!";
char *strPtr = NULL;

my_strcpy (strPtr, str); 

You want to copy str to somewhere, but where? You haven't provided any memory to store into. Consider

char str[80] = "Hello, there!";
char str2[sizeof str];

my_strcpy (str2, str); 

or

char str[80] = "Hello, there!";
char* strptr = malloc(sizeof str);
if (!strptr)
    /* do something on out-of-memory */;

my_strcpy (strptr, str); 

Also, my_strcpy is looping until it finds the NUL in the destination, which is of course wrong ... you can't look at the destination until you've stored something there. And, you aren't storing the NUL into the destination. Consider the simple loop

while ((*dest++ = *source++) != '\0')
    ;

Finally, you probably don't want to return dest, which now points past the NUL. Either declare my_strcpy to return void and don't return anything, or if you want it to have the same semantics as strcpy, save the original value of dest and return that, e.g.,

char* retval = dest;

while ((*dest++ = *source++) != '\0')
    ;

return retval;

or

for (char* destp = dest; (*destp++ = *source++) != '\0';)
    ;

return dest;

Pick the style you prefer.

Answer 2

First:

You don't allocate memory for dest array:

char *strPtr = NULL;    <-- allocate memory here
// strPtr = malloc(lenght + 1); // +1 for \0 chaar
my_strcpy (strPtr, str);   

Second:

Also, in string copy function loop should run while *source is nul.

while(*source){

Third:

You don't add \0 at the end to dest after loop-ends, add *dest = '\0'; after while loop in string copy code.

Fourth:

You returns dest while in loop you increments it!

Try this code:

char * my_strcpy(char *dest, const char *source)
{
    dest = malloc(strlen(source) + 1);  //allocate memory 
    tdest = dest;  // store it to return 
    while(*source) // copy until source terminate at \0
    {
        *tdest++ = *source++;
    }
    *tdest = '\0'; // add nul termination in dest string 
    return dest; // return allocated memory address 
}

Note that memory allocation is done inside string copy function.

By the way, I didn't change your function prototype and function name but what are you trying to build is called strdup(). Because memory allocation is done inside function and duplicate string is returned so you don't need to pass first argument.

So as @Jim Balter also reviewed my answer semantically correct way to write function is:

char * my_strdup(const char *source)
{
    dest = malloc(strlen(source) + 1);  //allocate memory 
    tdest = dest;  // store it to return 
    while(*source) // copy until source terminate at \0
    {
        *tdest++ = *source++;
    }
    *tdest = '\0'; // add nul termination in dest string 
    return dest; // return allocated memory address 
}

call it as:

char* strPtr = my_strdup(str);    

It you want memory allocation in main function and keep string copy function simple then please read Jim Balter's answer.

Answer 3

You will have to allocate memory for strPtr before calling your copy function

strPtr=malloc(sizeof(str));