C - Malloc and memcpy (memory management)

Question

I'm a bit new to C and I'm having trouble understanding how memory works, especially in-built functions like memcpy.

Here's a struct I'm using

 struct data_t {
    int datasize;   
    void *data; 
 };

And here's an auxiliary function that I'm using it with:

struct data_t *data_create(int size)
{
   struct data_t *dt=malloc(sizeof(struct data_t)+size);
   dt->datasize=size;   
   dt->data="1234567890a";
   return dt;
}

Now in the main function I have no problem doing this:

struct data_t *data = data_create(1024);
data->data="123456a";//just an example

But this throws a Seg Fault:

memcpy(data->data,"123456a",strlen("1234567890a")+1);

My question is why? And how do I avoid it? Please bear in mind I'm new to C so how C deals with memory is a bit new to me

Thank you.

Edit: It works! Thank you very much. Completly missed the data pointer. Now everything is working fine according to valgrind.

Solution 1

memcpy(data->data,"123456a",strlen("1234567890a")+1);

fails because data->data a void * type points to some garbage/invalid address which is not allocated. data has the address of the string literal which is stored in the readonly section (like in the .rodata of the executable and loaded in the memory, which is not writable. Also, if you did not assign such a string address to the pointer variable, then it would hold some invalid/garbage address value which is not allocated or initialized with some valid permitted location. So first allocate the buffer.

data->data = malloc (sizeof (char) * size);

The malloc will return the first location address of a block of address of atleast size * sizeof (char) bytes. Now you can copy size bytes to this memory location pointed by data->data.

Remember to free the allocated memory block when you have finished working with that memory bock with the free (addr) call.

---

I see you have tried to allocate data buffer with a very strange manner (?):

struct data_t *dt=malloc(sizeof(struct data_t)+size);

for which the extra allocated size bytes along with the struct data_t. But what ever be the case, data component still points to some place which cannot be changed. Please use:

struct data_t *dt = malloc(sizeof(struct data_t));
dt->data = malloc (sizeof (char) * size);
memcpy (data->data, "whatever", sizeof ("whatever")+1);
return dt;

to free first do:

free (dt->data);

then

free (dt);

Solution 2

Your first mistake is this:

struct data_t *dt=malloc(sizeof(struct data_t)+size);

This will create a chunk of memory of size struct data_t + size. I think what you expected was that your data field inside data_t could use this memory but it cannot because data does not hold an address to this memory.

Your second mistake was to assume that you where copying the value of the following string into "data":

data->data="123456a";

What in fact happened here is that there is a string in memory "123456a" that exists for the entire life of your program. When you assign "123456a" to data->data what is actually happening is that you are taking the address of this string "123456a" and putting it in data->data you are not copying the value ("123456a") but the location or address (0x23822...) of "123456a" .

Your final mistake was this:

memcpy(data->data,"123456a",strlen("1234567890a")+1);

You tried to copy the value "123456a" into memory pointed to by data. What is data pointing to? It's pointing to a read only area of memory containing your previously assigned string "123456a". In other words you told your program to write to the address of "123456a".

Here is a program what will do what you expect:

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

typedef struct {
    size_t datasize;   
    char *data; 
} data_t;

data_t *data_create(size_t size)
{
   data_t *dt;

   dt = malloc(sizeof(data_t));
   assert(dt != NULL);
   dt->data = malloc(size);
   assert(dt->data != NULL);
   dt->datasize = size;

   /* need to decide what to do when this happens */
   assert((strlen("1234567890a") + 1) < size);
   strcpy(dt->data, "1234567890a");

   return dt;
}

void data_destroy(data_t *dt)
{
    free(dt->data);
    free(dt);
}

int main(void)
{
    data_t *data = data_create(1024);
    /* data->data="123456a"; DONT DO THIS YOU WILL CAUSE A MEMORY LEAK */

    assert(data->datasize >= (strlen("123456a")+1));
    memcpy(data->data, "123456a", strlen("123456a")+1);

    printf("%s\n", data->data);

    data_destroy(data);

    return EXIT_SUCCESS;
}

Solution 3

Notice void *data is a pointer, in data_create, you didn't allocate space for it, you just make it point to a string constant "1234567890a" which is read only.

In main, you create another string constant "123456a", then you make void *data point to the string constant, which is read only.

So when you call memcpy to write to memory address which is not writeable(or without initialized), you got an error.

Comments

[Jonathan Leffler]

Space was allocated in the initial allocation; the pointer was simply not set to point to the space allocated.

[Mat]

The memcpy is still invalid it if tries to copy more bytes from the source than the source has, which is the case here.

[phoxis]

@JonathanLeffler: yes, the asker allocated some extra space when initially allocating, he should have initialized to the point. I will still prefer the explicit call, if there is no specific need to allocate all at once.

[VasaraBharat]

this will give an error : "invalid conversion from ‘void*’ to ‘data_t*’ [-fpermissive]" to solve " you have to explicitly cast the pointer returned by malloc".

[phoxis]

@VasaraBharat: You are correct, but that is in C++. The question was tagged C, for which an explicit conversion is not required. See C99 Section 6.3.2.3 Paragraph 1. Whereas in C++, it's not clearly mentioned (or I am not aware).