Malloc corrupting already malloc'd memory in C

Question

I'm currently helping a friend debug a program of his, which includes linked lists. His list structure is pretty simple:

typedef struct nodo{
    int cantUnos;
    char* numBin;
    struct nodo* sig;
}Nodo;

We've got the following code snippet:

void insNodo(Nodo** lista, char* auxBin, int auxCantUnos){
   printf("*******Insertando\n");
int i;
   if (*lista) printf("DecInt*%p->%p\n", *lista, (*lista)->sig);
Nodo* insert = (Nodo*)malloc(sizeof(Nodo*));
   if (*lista) printf("Malloc*%p->%p\n", *lista, (*lista)->sig);
insert->cantUnos = auxCantUnos;
insert->numBin = (char*)malloc(strlen(auxBin)*sizeof(char));
for(i=0 ; i<strlen(auxBin) ; i++)
    insert->numBin[i] = auxBin[i];
insert->numBin[i] = '\0';
insert->sig = NULL;
Nodo* aux;
/* [etc] */

(The lines with extra indentation were my addition for debug purposes)

This yields me the following:

*******Insertando
DecInt*00341098->00000000
Malloc*00341098->2832B6EE

(*lista)->sig is previously and deliberately set as NULL, which checks out until here, and fixed a potential buffer overflow (he'd forgotten to copy the NULL-terminator in insert->numBin).

I can't think of a single reason why'd that happen, nor I've got any idea on what else should I provide as further info. (Compiling on latest stable MinGW under fully-patched Windows 7, friend's using MinGW under Windows XP. On my machine, at least, in only happens when GDB's not attached.)

Any ideas? Suggestions? Possible exorcism techniques? (Current hack is copying the sig pointer to a temp variable and restore it after malloc. It breaks anyways. Turns out the 2nd malloc corrupts it too. Interestingly enough, it resets sig to the exact same value as the first one).

UPDATE: Thanks for the answers. Regarding the Node* thing, it's fixed, but no change. At least prevents potential problems afterwards. String copying isn't the issue, as I already fixed all missing \0s myself. (Note the insertBin[i] = '\0' after the for)

Answer 1

One problem is this line:

Nodo* insert = (Nodo*)malloc(sizeof(Nodo*)); 

it should be

Nodo* insert = (Nodo*)malloc(sizeof(Nodo)); 

(Rule of thumb: you should have one less '*' in the sizeof() )

You need to allocate space for the Node structure, NOT space for a pointer to the Node structure (which incidently, will be 4 bytes on 32bit systems)

A similiar problem exists with not allocating enough room for the string (char array); don't forget the space for the terminating zero '\0'

Answer 2

When you allocate memory for a string (char *) make sure it is of length strlen + 1 for the \0 at the end.

insert->numBin = (char*)malloc(strlen(auxBin)*sizeof(char));

needs to be

insert->numBin = (char*)malloc(strlen(auxBin) + 1);

Also there is no need to say * sizeof(char) which is 1.

One more thing John is right about how you allocate the structure, it must not be sizeof the pointer but sizeof the struct.

Answer 3

on this line:

Nodo* insert = (Nodo*)malloc(sizeof(Nodo*));

You're only allocating enough memory for a pointer to Nodo, not a whole Nodo. You want:

Nodo* insert = (Nodo*)malloc(sizeof(Nodo));

Also, you may have at least one other allocation error:

insert->numBin = (char*)malloc(strlen(auxBin)*sizeof(char));
for(i=0 ; i<strlen(auxBin) ; i++)
    insert->numBin[i] = auxBin[i];

It looks like you're duplicating a string. You'll want to allocate enough for the string plus one to get the terminating \0. You can simplify with this standard library call:

insert->numBin = strdup(auxBin);

EDIT: just noticed you're on Windows, so strdup() might not be available (it's a POSIX routine) so you can cover string duplication this way. Note the +1 on the length for the terminator:

insert->numBin = (char *)malloc( strlen(auxBin)+1 );
strcpy( insert->numBin, auxBin );