memcpy() not working correctly

Question

I am having trouble copying a 2D array in a function in c. Here is the code:

void Add(Lista* list,int** estado, char* sec, int size)
{

   if(list->last==NULL)
   {
    list->last = calloc(1,sizeof(element));
    list->last-> secuencia = sec;
    list->last->next = NULL;
    list->last->prev = NULL;
    list->last-> estado = (int**)calloc(size,sizeof(int));
    memcpy(&list->last->estado,&estado,size*sizeof(int));

    list->cantidad++;
   }
   else
   {    
    list->last-> next = calloc(1,sizeof(element));
    list->last-> next -> secuencia = sec;
    list->last->next->next = NULL;
    list->last->next->prev = list->last;
    list->last =list->last->next;
    list->last->estado = (int**)calloc(size,sizeof(int));
    memcpy(&list->last->estado,&estado,size*sizeof(int));
    list->cantidad++;
   }

}

Here are the structs Lista and element

typedef struct element
{
   char* secuencia;
   struct element* next;
   struct element* prev;
   int** estado;
}element;

typedef struct Lista
{

   int cantidad;    
   element* last;

}Lista;

The idea is to add "elements" in Lista, it is a basic list that returns the last added element. The problem is that every element stored in list returns the same "estado" (2D int array), if I modify one of the elements estado then every element estado gets the same modification. So, I dont know where is the problem, because memcpy() should copy the values and then make both arrays independent of each other, right?.

PS: Sorry if it was not well explained, I speak Spanish

EDIT

So, I change my code based on the answers to this:

void Add(Lista* list,int** estado, char* sec, int size)
{

   if(list->last==NULL)
   {
    list->last = calloc(1,sizeof(element));
    list->last-> secuencia = sec;
    list->last->next = NULL;
    list->last->prev = NULL;
    list->last-> estado = (int**)calloc(size,sizeof(int));
    memcpy(list->last->estado,estado,size*sizeof(int));

    list->cantidad++;
   }
   else
   {    
    list->last-> next = calloc(1,sizeof(element));
    list->last-> next -> secuencia = sec;
    list->last->next->next = NULL;
    list->last->next->prev = list->last;
    list->last =list->last->next;
    list->last->estado = (int**)calloc(size,sizeof(int));
    memcpy(list->last->estado,estado,size*sizeof(int));
    list->cantidad++;
   }

}

Now I haven't valgrind errors, but I keep getting the problem (when I modify one array the others get the modification)

EDIT2

So, I started checking the code and before the call to Add() there was an incorrect assignation of the array (always passed the same array to the function), that and the modification of EDIT1 solved my problem.

PS2: I will look deeply into pointer, thanks

A `int**` does not refer to a 2D array of `int`, but to a pointer to pointer of `int`. — alk, Jun 01 '16 at 15:14
Are you using an array of pointers, each pointing to arrays of `int`? Or is it all allocated in one chunk with extra math to find the right element from a row and column? — Dmitri, Jun 01 '16 at 15:23
@Dmitri it's all allocated in one chunk, except for the first one that is created number by number (I've to get it from a .txt file) — Nano Aguayo, Jun 01 '16 at 15:32
I am not using `memcpy()` correctly, because if it were'nt working correctly we would be in serious problems. — Iharob Al Asimi, Jun 01 '16 at 15:32
Can you show how you create the array, and how you access the elements? — Dmitri, Jun 01 '16 at 15:35
[Don't cast the result of `malloc` in C](http://stackoverflow.com/q/605845/995714) — phuclv, Jun 01 '16 at 15:46
@LưuVĩnhPhúc keep getting the same with or whithout the cast — Nano Aguayo, Jun 01 '16 at 15:53
I didn't say that's related to your problem. That's a suggestion — phuclv, Jun 01 '16 at 15:55
@NanoAguayo For the umpteen time, `int**` is not a contiguous 2D array. — 2501, Jun 01 '16 at 15:56
Posting code showing the `Add()` call and the origin of the variables passed to `Add()` would help. — chux - Reinstate Monica, Jun 01 '16 at 16:22

score 3 · Accepted Answer · edited May 23 '17 at 12:31

3

Your memcpy-ing onto the address of a pointer rather than the memory it's pointing to. Try this instead:

memcpy(list->last->estado,estado,size*sizeof(int));

Look in to pointers of pointers if you want to read around the subject.

Also, as pointed out by some comments, a double pointer is not the same as a 2D array. Check out this answer on creating a pointer to a 2D array. You want to be doing something like that in your code. estado should be a pointer to a double array instead.

edited May 23 '17 at 12:31

Community

1
1

answered Jun 01 '16 at 15:14

noelicus

14,468
3
92
111

2

And this is one of the reasons why it is important to know the difference between a pointer-to-array and a pointer-to-pointer. – John Bollinger Jun 01 '16 at 15:23
2

This cannot be correct because `estado` points to an array of int pointers, yet this code is using `sizeof(int)`. – 2501 Jun 01 '16 at 15:24
Did it and worked better, but still get the same values in every "estado" – Nano Aguayo Jun 01 '16 at 15:55

chux - Reinstate Monica · Answer 2 · 2016-06-01T16:12:53.867

1

memcpy(list->last->estado,estado,size*sizeof(int)); is not type/size consistent.

estado is type int**
list->last->estado is type int**, the same as estado (good)
size*sizeof(int) is size being multiplied by the wrong size element.

Rather than an int, it should be the type the 2 pointers point to. (int*)

Recommend the following to avoid the wrong size element.

memcpy(list->last->estado,estado,size*sizeof *(list->last->estado));

Same problem for calloc(): wrong size. Also no need for cast.

// list->last->estado = (int**)calloc(size,sizeof(int));
list->last->estado = calloc(size, sizeof *(list->last->estado));

// For consistency
// list->last-> next = calloc(1,sizeof(element));
list->last-> next = calloc(1,sizeof *(list->last-> next));

edited Jun 01 '16 at 16:12

answered Jun 01 '16 at 16:07

chux - Reinstate Monica

143,097
13
135
256

1

I think OP will be even more confused. The code is technically correct, but I doubt copying just the pointers is the intended behavior. – 2501 Jun 01 '16 at 16:15
1

@2501 Suspect you are correct. With what OP posted, this at least takes care of allocating the right size. Should code be copying what `estado` points to (as an array) or replicating `estado` is TBD. – chux - Reinstate Monica Jun 01 '16 at 16:20

memcpy() not working correctly

2 Answers2