Segmentation fault occuring with zero-length array

Question

I have this linked list node struct that's using a zero-length array for storing memory:

typedef struct s_list
{
    size_t          *list_size;
    struct s_list   *prev;
    struct s_list   *next;
    size_t          size;
    char            data[0];
}   t_list;

(list_size is a pointer containing the size of the total list)

And I'm using this function to allocate a new node:

static t_list   *lst_new_element(void *data, size_t size)
{
    t_list  *new_element;

    new_element = malloc(sizeof(t_list) + size);
    if (!new_element)
        return (NULL);
    new_element->size = size;
    memcpy(new_element->data, data, size); // <--- Segfault occurs here
    return (new_element);
}

So the segmentation fault occurs in the memcpy, but I don't understand why because I allocate sizeof(t_list) + size bytes so this should be enough to do a memcpy(size) on data. The segfault occured with this call: lst_new_element((void *)atoll(argv[1]), sizeof(long long)) (argv[1] is 5)

Thanks for the help.

Does 5 (argv[1]) represents a valid address to read in your environment? (For example, user binaries in xv6 will allow that) — MikeCAT
– MikeCAT, Commented May 20, 2021 at 12:41
Not sure but... for flexible array members, I write [] - not [0] — 4386427
– 4386427, Commented May 20, 2021 at 12:46
@BitTickler Rather, back in the days, people were using something like char arr[1]; as last member, then wildly converting that into some pointer or accessing it out of bounds. This was known as "the struct hack" and was a common source of strange bugs and broken code. Afaik gcc invented zero-length arrays as a cure against "the struct hack", somewhere in the mid-90s. — Lundin
– Lundin, Commented May 20, 2021 at 12:58

dbush · Accepted Answer · 2021-05-20 12:48:21Z

3

You're passing a long long value to your function as if it's a valid void *. Your function then attempts to dereference that pointer (which in invalid) in an attempt to copy what it points to. This triggers undefined behavior leading to a crash.

You need to assign the return value of atoll to a local variable, then pass the address of that variable to the function.

long long val = atoll(argv[1]);
t_list *l = lst_new_element((&val, sizeof(long long));

Also, using a 0 length array as the last member of a struct is an extension many compilers use to implement a flexible array member. The standard-compliant way of doing this is to leave the size blank.

typedef struct s_list
{
    size_t          *list_size;
    struct s_list   *prev;
    struct s_list   *next;
    size_t          size;
    char            data[];
}   t_list;

answered May 20, 2021 at 12:48

dbush

233k27 gold badges261 silver badges334 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Fayeure Over a year ago

Oh yes, I don't know why but I thought it would copy the address of the pointer, I forgot how my own code worked x), thanks

Lundin · Accepted Answer · 2021-05-20 12:45:01Z

2

(void *)atoll You are converting a long long value to a pointer which is of course plain wrong. Instead store the results in a temporary variable and pass that one (by value or reference).

Also please note that ato... functions are semi-obsolete and dangerous, you should be using strtoll instead, which has better error handling.

In addition (not related to the crash), zero-length arrays is an obsolete non-standard feature of gcc since well over 20 years. You should be using standard C flexible array members instead. They work exactly the same, just change the code to: char data[];.

answered May 20, 2021 at 12:45

Lundin

220k47 gold badges281 silver badges446 bronze badges

Comments

Paul Ogilvie · Accepted Answer · 2021-05-20 12:47:08Z

2

For your function call you need an intemediate variable to store the converted value, for example:

long long llval = atoll(argv[1]);
lst_new_element(&llval, sizeof(long long));

answered May 20, 2021 at 12:47

Paul Ogilvie

25.3k4 gold badges25 silver badges43 bronze badges

Comments

MikeCAT · Accepted Answer · 2021-05-20 12:51:22Z

1

You can use compound literals to allocate a temporally array to have the value be in the memory instead of using temporally variable as othe answers suggests.

lst_new_element((long long[]){ atoll(argv[1]) }, sizeof(long long));

answered May 20, 2021 at 12:51

MikeCAT

75.1k11 gold badges50 silver badges71 bronze badges

2 Comments

Paul Ogilvie Over a year ago

Clever, but since the compiler will allocate an invisible variable, nothing is gained, except obfuscation (downvote not by me).

chux Over a year ago

@PaulOgilvie Compound literals are simply another approach presented here.

Collectives™ on Stack Overflow

Segmentation fault occuring with zero-length array

4 Answers 4

1 Comment

Comments

Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

1 Comment

Comments

Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related