Why does this C struct initialization code produce a bus error?

Question

When designing a game entity system in C, I attempted an "equals-free" initialization approach. I was surprised to see a linter tell me there was a memory leak at the end of my init function, and that my variable ent was never initialized in the following code. It turned out to be right because I got a "bus error":

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

typedef struct {
    int x;
    int y;
} entity_t;

void entity_init(entity_t* ent, int _x, int _y)
{
    ent = malloc(sizeof(*ent));
    ent->x = _x;
    ent->y = _y;
}

int main(void)
{
    entity_t* ent;
    entity_init(ent, 10, 24);
    printf("Entity: x%d y%d", ent->x, ent->y);
    return 0;
}

What I thought the above code would do, was take my empty ent pointer supplied as an argument, tell it to point to some newly allocated memory, and then fill in that memory and everything would be fine. I have no idea what it's really doing to cause a "bus error", am I missing something critical about pointers and malloc?

I vaguely remember seeing something very similar to this done before in some C code (equals-free struct initialization) and I would strongly prefer to use an equals-free initialization style similar to this (broken) code if such a thing is possible in C.

`ent = malloc(sizeof(*ent));` overwrites the function agument passed. End of. All that remains is a memory leak and undefined behaviour in `main`. Crazy - `entity_t* ent;` has not even been initiliased to `NULL`, so it might even do something bizzarre without breaking. — Weather Vane, Nov 29 '17 at 22:39
when changing, in a sub function, where a pointer points in the calling function, the parameter to the sub function must be the address of the pointer. In the current scenario, the call should be: `entity_init( &ent, 10, 24);` and signature should be: `void entity_init(entity_t** ent, int _x, int _y)` — user3629249, Nov 29 '17 at 23:01
when writing variable, parameter, and macro names, do NOT use a leading underscore. The leading underscore, especially when followed by a capital letter, is 'reserved' for the OS. — user3629249, Nov 29 '17 at 23:02
regarding: `ent = malloc(sizeof(*ent));` always check (!=NULL) the returned value to assure the operation was successful. — user3629249, Nov 29 '17 at 23:04
in general, it is a poor programming practice (many examples to the contrary) to define a struct in a typedef statement. Also, always use a 'tag name' as that is what most debuggers use to enable display of the individual fields within the struct. — user3629249, Nov 29 '17 at 23:06
the posted code (due to only passing the contents of `ent` (which is a pointer)) results in the parameter on the stack being updated, but not the variable declared in `main()` — user3629249, Nov 29 '17 at 23:08

Petr Skocik · Accepted Answer · 2017-11-29T22:51:06.390

Move the malloc call outside the initialization function:

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

typedef struct {
    int x;
    int y;
} entity_t;

void entity_init(entity_t* ent, int _x, int _y)
{
    ent->x = _x;
    ent->y = _y;
}

int main(void)
{
    entity_t* ent;
    if(NULL==(ent = malloc(sizeof(*ent))))
        return 1;
    entity_init(ent, 10, 24);
    printf("Entity: x%d y%d", ent->x, ent->y);
    return 0;
}

You're assigning the pointer to the allocated block to a local variable (ent). That can't affect the ent in main.

If you wanted to keep the malloc in entity_init, you should use a double pointer, but you should also change the signature to allow for a way to signal malloc failure from entity_init

int entity_init(entity_t **ent, int _x, int _y)
{
    if(NULL==(*ent = malloc(sizeof(**ent))))
        return -1;
    (*ent)->x = _x;
    (*ent)->y = _y;
}

int main(void)
{
    entity_t* ent;
    if(0>entity_init(&ent, 10, 24))
        return 1;
    printf("Entity: x%d y%d", ent->x, ent->y);
    return 0;
}

A more usual pattern for this would be:

entity_t *entity_new(int _x, int _y)
{
    entity_t *ent = malloc(sizeof(*ent));
    if (NULL==ent) 
        return NULL;
    ent->x = _x;
    ent->y = _y;
    return ent;
}

int main(void)
{
    entity_t* ent;
    if(NULL==(ent=entity_new(10,24)))
        return 1;
    printf("Entity: x%d y%d", ent->x, ent->y);
    return 0;
}

@MartinJames Incorporated that into an expanded version of the answer :) — Petr Skocik, Nov 29 '17 at 22:51
Why would ent ever be NULL, in the entity_new function, in the last example? — Accumulator, Nov 29 '17 at 23:17

ad absurdum · Answer 2 · 2017-11-29T22:57:21.863

If you must allocate within the entity_init() function, you need to either return a pointer to the allocation, or add a layer of indirection by making ent a pointer to pointer to entity_t. In the posted code, within entity_init() ent is a copy of the pointer which was passed to the function. Any changes made to this pointer, such as assigning the address of a memory allocation to the pointer, will not be visible from the calling function since this copy will cease to exist after the function returns.

Also, note that you need to check the value returned from malloc() to be sure that the allocation was successful. If successful, the function can continue with the initialization process; if not, ent can remain a null pointer, which should be checked in the calling function:

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

typedef struct {
    int x;
    int y;
} entity_t;

void entity_init(entity_t **ent, int _x, int _y)
{
    *ent = malloc(sizeof **ent);
    if (*ent) {
        (*ent)->x = _x;
        (*ent)->y = _y;
    }
}

int main(void)
{
    entity_t *ent;
    entity_init(&ent, 10, 24);

    if (ent == NULL) {
        fprintf(stderr, "Allocation failure in entity_init()\n");
        exit(EXIT_FAILURE);
    }

    printf("Entity: x%d y%d\n", ent->x, ent->y);

    return 0;
}

Program output:

Entity: x10 y24

Why does this C struct initialization code produce a bus error?

2 Answers2