How to properly malloc for array of struct in C

Question

I will read in two set of char* (or strings) using strtok, and since those two set of chars are related, (address : command\n) I decided to use a structure.

struct line* array = (struct line*)malloc(sizeof(file) * sizeof(struct line*));

This line mallocing space for the function gives me a segmentation fault and was wondering if you can tell me a proper way to malloc space for it. For context, here is the rest of my code:

struct line
{
    char* addr;
    char* inst;
};
while loop{
    x = strtok(line,": ");
    y = strtok(NULL,"\n");
    strcpy(array[i].addr,x); //assume that x and y are always 3characters
    strcpy(array[i].inst,++y);
    i++;
}

Solution 1

Allocating works the same for all types. If you need to allocate an array of line structs, you do that with:

struct line* array = malloc(number_of_elements * sizeof(struct line));

In your code, you were allocating an array that had the appropriate size for line pointers, not for line structs. Also note that there is no reason to cast the return value of malloc().

Note that's it's better style to use:

sizeof(*array)

instead of:

sizeof(struct line)

The reason for this is that the allocation will still work as intended in case you change the type of array. In this case this is unlikely, but it's just a general thing worth getting used to.

Also note that it's possible to avoid having to repeat the word struct over and over again, by typedefing the struct:

typedef struct line
{
    char* addr;
    char* inst;
} line;

You can then just do:

line* array = malloc(number_of_elements * sizeof(*array));

Of course don't forget to also allocate memory for array.addr and array.inst.

Solution 2

For what you have described, You do not need to allocate memory for your struct, rather, you need to allocate memory for the members char *addr;, and char *inst;. If you want to have a single copy of that structure, the first section of code illustrates how to initialize, and assign values. If you want an array, the second code example illustrates the differences.

This illustrates how to allocate memory for the members of a single struct line:

typedef struct
{
    char* addr;
    char* inst;
}LINE;
LINE line;  
int main(void)
{   
    strcpy(line.addr, "anystring"); //will fail
    line.addr = malloc(80);
    line.inst = malloc(80);
    strcpy(line.addr, "someString");//success;
    strcpy(line.inst, "someOtherString");//success;
}

For array of struct line...

typedef struct
{
    char* addr;
    char* inst;
}LINE;  //same struct definition
LINE line[10]; //but create an array of line here.
int main(void)
{   
    int i;
    for(i=0;i<10;i++)
    {
      line[i].addr = malloc(80);
      line[i].inst = malloc(80);
    }
    for(i=0;i<10;i++)
    {
        strcpy(line[i].addr, "someString");
        strcpy(line[i].inst, "someOtherString");
    }
    //when done, free memory
    for(i=0;i<10;i++)
    {
        free(line[i].addr);
        free(line[i].inst);
    }      
}

Added to address comment
Addressing the comment under this answer from @Adam Liss, the following code illustrates the following improvements using strdup(): 1) Uses only memory needed. 2) Performs memory creation and copy operations in one step, so the the following blocks:

for(i=0;i<10;i++)
{
  line[i].addr = malloc(80);
  line[i].inst = malloc(80);
}
for(i=0;i<10;i++)
{
    strcpy(line[i].addr, "someString");
    strcpy(line[i].inst, "someOtherString");
}

Become:

for(i=0;i<10;i++)
{
  line[i].addr = strdup("someString");
  line[i].inst = strdup("someOtherString");
}

One more note: Error handling was not included in examples above to avoid muddling up focus on the main concepts: But for the sake of completeness, because both malloc() and strdup() can fail, actual usage for each of these two functions, should include a test before using, eg:

Rather than

  line[i].addr = strdup("someString");
  line[i].inst = strdup("someOtherString");

The code should include

  line[i].addr = strdup("someString");
  if(!line[i].addr)
  {
      //error handling code here
  }
  line[i].inst = strdup("someOtherString");
  if(!line[i].inst)
  {
      //error handling code here
  }

Comments

[Michael Burr]

Keep in mind that allocating memory for an array of struct line doesn't allocate memory for the addr and inst strings. Depending how those pointers inside the struct line are used you may need to also allocate the memory for the string data.

[Adam Liss]

Or use strdup() and use the remainder of the 80 characters for something more useful. :-)

[Bemipefe]

If prefer writing struct every time because the keyword does its job which is remembering you that you are allocating a complex type and not a primitive one (like int , or char * ). Also I prefer sizeof(struct line) because if you write sizeof(*array) it seems like you are dereferencing the pointer before allocating it and if its not NULL initialized I bet you will get a crash.

[Nikos C.]

@Bemipefe sizeof(*array) does not derefence. sizeof is a static operator that inspects the type of its argument. Since the type of *array is line, sizeof(*array) means sizeof(line).

[Bemipefe]

Ok got it no crash because it's evaluated at compile time. Anyway this syntax is misleading if don't know such language "internals". Furthermore you won't get many benefits because if you change type inside a function you will need to change also something else in the code because probably some field name changed or some field size changed and silently allowing the compilation can lead to a whole set of problems.

[Nikos C.]

@Bemipefe sizeof(obj) instead of sizeof(type) is a very common pattern used by C programmers. Obviously it's personal preference. However, note that when the type is changed, nothing will indicate that you need to change the sizeof use as well. sizeof just evaluates to a size, regardless of what you pass to it. So everything will compile fine and without any warning, even though the size you get will be wrong. This is why so many people use sizeof like this.

[Bemipefe]

Ok you are right the code would compile anyway. Of course is just a personal opinion. I can't see that syntax in the code because it has that meaning only in that context. Maybe if you explicitly cast the pointer returned by either calloc, malloc or realloc to a specific type you will get at least a warning from the compiler. This might depend on the compiler and the option specified.