Failing to alloc memory received by a pointer linux

Question

I have a function that receives by a pointer the location where will be stored. This place can have different other similar structs.The function has to read a file. This file have stored a struct, which i need to read.

typedef struct user_manage_t{
    short int user_id;
    char permission;
    long int other_id;
    long int check;
}user_manage_t;


typedef struct holder_t{
    user_manage_t *user_manage;
    user_manage_t *user_manage_backup;
    //(...)and a lot of stuff
}holder_t;

holder_t  holder;


int db_read_from_file(user_manage_t *prt){

    DEBUG_PRINT("READ_FROM file started");
    FILE *fd_read;
    char buffer[480];
    int read, bytesRead=0; 
    int num;
    const  struct user_manage_t *header;
    fd_read = fopen("/home/user/user_list","r+b");   
    if (fd_read == NULL) 
    {
        printf("Error");
    }
    else
    {
        DEBUG_PRINT("Its open!!!");
    }
    do 
    {
        read=fread(buffer, 2, 90, fd_read);
        bytesRead=bytesRead+read;   
        DEBUG_PRINT("Number of bytes lidos read=%d",bytesRead);
    }while(read!=0); //(bytesRead < 480);
    header = (struct  user_manage_t *) (buffer);
    fclose(fd_read);
    if ( NULL == ( prt = calloc( 10, sizeof(user_manage_t))))//aloca
    {
        DEBUG_PRINT("MAJOR_ERROR: couldnt allocate mem to users");
        return -1;
    }
    else
    {
        memcpy( (struct user_manage_t *) &prt, &buffer, 90); 
        DEBUG_PRINT("Users copied to main list");
        for ( short int i=0;i<4 ; i++ )
        {
            DEBUG_PRINT("i= %hd",i);
            DEBUG_PRINT("User id: %d",holder.user_manage[i].user_id );
            DEBUG_PRINT("Permission: %d",holder.user_manage[i].permission);
            DEBUG_PRINT("other_ID:%ld",holder.user_manage[i].other_id);
            DEBUG_PRINT("Check_value:%ld", holder.user_manage[i].check);
        }
        return 1;
    }
}
main(){

        db_read_from_file((struct user_manage_t *) &holer.user_manage); 
        db_read_from_file((struct user_manage_t *) &holder.user_manage_backup);
}

When I run the code I get SEGFAULT and valgrind tell me this,

Thread 2:
==2746== Invalid read of size 2
==2746== at 0x80523B4: db_read_from_file (code.c:3069)
==2746== by 0x20303333: ???
==2746== Address 0x0 is not stack'd, malloc'd or (recently) free'd

This is the line of "DEBUG_PRINT("User id: %d",holder.user_manage[i].user_id );" So obviously looks like I am not storing it in the right place. Can u help me?

Not sure if it's the cause of your exact problem, but in memcpy( (struct user_manage_t *) &prt, &buffer, 90); you copy 90 bytes to the place where the prt pointer variable is stored, not to the place where it points to. — sth
sth is right, that memcpy has to be corrupting memory, unless your pointers happen to be 90 bytes in size somehow... and perhaps consider checking prt for NULL pointers, too? — Elias Van Ootegem
correct me if I am wrong, but I think I am allocating 10x sizeof(user_manage_t) which has 12 bytes, so a total of 120 bytes — Kxp
@EliasVanOotegem: Going by what you see here on Stack Overflow, that seems to be the most common use case for casts ;) — sth

Elias Van Ootegem Elias Van Ootegem · Accepted Answer · 2014-06-19T14:18:20

Priority Nr 1
After looking at your code a bit more, I suspect you're casting as much as you do, because you kept getting compiler warnings about "incompatible [pointer] types" and the like. Those warnings exist for a reason: there's an issue, a possible source for bugs there. Don't hush it up, don't ignore it: FIX IT!

Yes, sometimes those casts are required, and sometimes compilers complain about your code when you know what you're doing. In that case, you can add a cast, but don't think of those casts as compiler-gags: they are ways to tell the compiler that you know what you're doing. You've just gone cast-crazy to get the compiler to shut up. That's bad.

Next
In both your db_read_from_file calls from main, you are passing pointer to a null pointer to your function. That means you still have to allocate memory to actually go and store that data, or you have to redefine holder to something like:

struct
{
    user_manage_t user_manage;//not pointers, actual structs
    user_manage_t user_manage_backup;
} holder;

If you keep them as pointers, just allocate all members of holder in main:

holder.user_manage = malloc(sizeof *holder.user_manage);//and so on
//preferably, though:
if (NULL == (holder.user_manage_backup = malloc(sizeof *holder.user_manage_backup))
    exit (EXIT_FAILURE);//error

The big one
As sth already mentioned:

memcpy( (struct user_manage_t *) &prt, &buffer, 90);

You are passing &prt, which reads: address of prt. This variable in itself is already a pointer, the memory address of a pointer is, again, a pointer. A pointer to a pointer (double indirection, avoid if possible...). Now as if that weren't enough: look at what you pass to your function:

db_read_from_file(&holder.user_manage);

Keep in mind that holder.user_manage already is a pointer, you are passing a pointer to a pointer! Thats double indirection. Then, you pass a pointer to this pointer to a pointer to memcpy!! Yes, you may have to read this last sentence again. But in short: you are passing a pointer, to a pointer, to a pointer to a struct, where that last bit (the pointer to a struct) could just be a null-pointer, too!

So what you have is this:

memcpy(void ***, char *, 90);//where the prt is void ***, and **prt could be NULL

Think about memcpy as a function that, basically does this:

void * memcpy( void *target, const void *src, size_t nr_of_bytes)
{
    char *dest = target;
    char *from = src;//use char, as it is guaranteed to be 1 byte in size
    int i;
    while(nr_of_bytes--)
        *dest++ = *from++;//copy byte to destination, move pointer 1 byte
    return dest;//return destination
}

Notice that the destination is being dereferenced (*dest++). If you pass a pointer to a pointer(&prt), and dereference it, you end up with a pointer, right? That's what you are writing to => *(&prt) == prt!

The cast, and the way you use it suggest you believe you're writing to whatever prt is pointing to, while in fact you are trying to write 90 bytes to whatever the pointer to prt is pointing to. And it's pointing to prt, which in turn points to a pointer. Only after this third inderiction, we find the struct... that's just crazy.
Anyway, the size of a pointer is 4 bytes on a 32 bit system, 8 on 64 bits. You are copying 90 bytes, so you probably end up in memory you shouldn't mess with.

Replace your code with this:

memcpy(*prt, buffer, sizeof *prt);//copy max the sizeof whatever prt is pointing to

And change the db_read_from_file function to this:

int db_read_from_file(user_manage_t **prt)//pointer to pointer!

And keep in mind that, whenever you want to change something of the struct that prt points to (2nd level), you have to dereference it, to get a regular pointer. For example, allocating memory:

if ( NULL == ( prt = calloc( 10, sizeof(user_manage_t))))//aloca

Has to become:

if ( NULL == ( *prt = calloc( 10, sizeof(user_manage_t))))//aloca

However, this is still wrong in many ways. What you actually need, is realloc, because prt might already be pointing at allocated memory:

*prt = realloc(*prt, 10*sizeof **prt);
if (*prt == NULL)
    //ERROR

It's cleaner, and safer.

Also check if your function isn't being passed a null pointer, do away with needless casts (they're clutter), and always check the return value of functions!

Failing to alloc memory received by a pointer linux

2 Answers