memcpy to an array of strings inside a struct

[wooden robin]

Hello, Im learning C and I'm trying to create a function which returns a pointer to a dynamically allocated struct which stores an array of strings.
The number of strings stored in the depends on the number of lines a file has, so I retrieve them. Nothing wrong so far, but when I try to copy the user's input into the struct I get a segfault.

typedef struct Groceries
{
        int done;
        int undone;

        char* name[32];
} Groceries

Groceries* create_groceries_list()
{
        ...
    int lines = read_lines(f); // each line of this file is a grocery
        Groceries* grocery = malloc(sizeof(*grocery) + lines * (sizeof(*grocery->name));

        while(!feof(f)) // bad practice
        {
                ....

                char g[64];
                printf("grocery name (tomatoes, carrots...\n");
                scanf("%s", g);
                memcpy(groceries->name[lines], g, sizeof(g)); // SEGFAULT
                lines++;
                ....
        }
}

I fixed it by dynamically allocating memory for the groceries before copying my buffer into it, but I would like to understand why my code above is wrong (preallocating the amount of memory I need)
any explanation would be appreciated

[warm quest]

When your question is answered use !solved or the button below to mark the question as resolved.

Remember to ask specific questions, provide necessary details, and reduce your question to its simplest form. For tips on how to ask a good question use !howto ask.

[honest jacinth]

you're allocating space for your array of char*s, but you aren't then allocating space for each of those pointers to point to, and which to store the strings in

[wooden robin]

so the only way to do things right would be to first Grocery* groceries = malloc(sizeof(*groceries));
and then when I need to store the string in my struct do :
grocery->name[n] = malloc(sizeof g / sizeof g[0]);
and then memcpy my element into the allocated space.
Cant I just create a plane allocated space with predefined size in the struct to store my strings and fill it the way you fill a list

[honest jacinth]

well technically you could point your array into the extra space at the end of your original allocation, but that's weird and overcomplicated and not what I'd recommend so someone who's learning

I'm not sure where element is coming from. You might want to base it on the length of the string in g though

[wooden robin]

yes it was an example i forgot how I named my variable, I meant g yes

okey, thank you

[wooden robin]

at the end and browse back ?
Its fine if its complicated, I want to get familiar with the way all this works

[honest jacinth]

sizeof (char) is 1 by definition, so there's no point using it in math to calculate sizes. just adds more noise to read.

[honest jacinth]

It'd be something like

Groceries* grocery = malloc(sizeof(*grocery) + some_extra_amount);
grocery->name[0] = /* the next byte after the last one in grocery->name[31] */
memcpy(grocery->name[0], ...);
grocery->name[1] = grocery->name[0] + strlen(grocery->name[0]);

you could compute that with stuff like offsetof

[wooden robin]

Okey, I will take a look at it, thank you

[warm quest]

Thank you and let us know if you have any more questions!

This thread is now set to auto-hide after an hour of inactivity

[honest jacinth]

offsetof and learning to do this probably isn't the best use of your time and effort at this point

[wooden robin]

alr, I will take a look, actually I've found a more convenient to do this, but I wanted to know if doing it that way was possible

[honest jacinth]

if you want it as one big block, and you're using the constant size of g anyways, you might just want to do

typedef struct Groceries
{
        int done;
        int undone;

        char name[32][64];
} Groceries;

[wooden robin]

and is doing
Grocery* groceries = malloc(sizeof(*groceries));
and then when I need to store the string in my struct do :
grocery->name[n] = malloc(sizeof g / sizeof g[0]);
and then memcpy my element into the allocated space.
(like I do now and it works just fine) a decent way to do it?

[honest jacinth]

if you're doing it this way, you won't need to malloc each string. they're already there as part of the array

it's an array of char[64]s, not an array of char*s

[wooden robin]

but if I make one big block and I finally need less space because I have less groceries than expected I will have extra unused memory, which is not happening if I malloc each string

speaking of groceries is frying me lol but I had no other idea

[honest jacinth]

then I'd go with this, an array of char*s

or if you really want to be fancy, a char** so you can dynamically allocate the right number of char*s, in case you have more or less than 32 items

[honest jacinth]

this part I do think would be worth learning and trying to do

[wooden robin]

I actually thought char** and the char*[] I use above were same since I see them both in the same kind of function parameters

[wooden robin]

I will try that tonight

[honest jacinth]

function parameters are a special case, since you can't actually pass an array as a parameter. it's decaying to a pointer to the first element. you're just allowed to write it as an array type it could have come from because some people say it makes it easier to read

besides, what you had is specifically char* name[32];. An array of 32 char*s. While char** is a pointer to a char*

[wooden robin]

okey I get it

thanks for the explanations, its helpful

[wooden robin]

hey, I think I managed to do it, and it looks cleaner, is it what you were speaking about? :

typedef struct Grocery
{
        int done;
        int undone;
        char** elements;
} Grocery;

Grocery* add_groceries(int number)
{
        Grocery* grocery = malloc(sizeof(*grocery));

        grocery->elements = (char**)malloc(sizeof(char*) * number); // here

        for (int i = 0; i <= number; ++i)
        {
                char buffer[64];
                printf("what is the %d grocery you'd like to add ?\n", i);
                scanf("%s", buffer);

                grocery->elements[i] = malloc(sizeof(buffer));
                memcpy(grocery->elements[i], buffer, sizeof(buffer));

                printf("moved %s to struct\n", grocery->elements[i]);
        }

        return grocery;
}

grocery->elements = (char**)malloc(sizeof(char*) * number); Im also unsure if i should malloc char* or chars, but I guess both works since Im gonna give the appropriate space to each element in my for statement, anw if you could tell me if somthing is wrong here it would help me alot

[honest jacinth]

that line is right, you're allocating an array of char*s to then store the rest of your allocations in

though the (char**) cast on malloc isn't needed

also, in this line:

grocery->elements[i] = malloc(sizeof(buffer));

consider what would happen if you used strlen rather than sizeof

[wooden robin]

I would get the size I really need instead of the buffer?

yeah its more efficient that way

malloc(strlen(buffer) + 1);