Dynamically allocating array of structs C

Question

I'm trying to make an array of structs in c, but I can't make it work. When I try to run it, the program crashes.

typedef struct{
    char name[20];
    char manufacturer[20];
    unsigned int price;
} product;

unsigned int stringToNr(char *numbers){
   unsigned int nr = 0; 
   unsigned int i; 
   for (i = 0; i < strlen(numbers); i ++)
   {
       nr  *= 10; nr += numbers[i] - '0'; 
   }
   return nr; 
}

I have a function that would print the list to a file, sometimes it reaches this function, sometimes it crashes before.

void printList(product *products, unsigned int nr){
    unsigned int i;
    FILE *f;
    f = fopen("output.txt", "w");
    for (i = 0; i < nr; i ++){
        fprintf(f, "%s ", products[i].name);
        fprintf(f, "%s ", products[i].manufacturer);
        fprintf(f, "%d\n", products[i].price);
    }
    fclose(f);
}

I have to use a separate function to read the list from file.

void readList(product **products, unsigned int *nr){
    FILE *f;
    f = fopen("input.txt", "r");
    char *row;
    row = malloc(sizeof(char) * 45);
    unsigned int rowLength;
    fgets(row, 45, f);
    rowLength = strlen(row);
    if (row[rowLength - 1] == '\n'){
        rowLength--;
        row[rowLength ] = '\0';
    }
    *nr = stringToNr(row);
    products = malloc((*nr) * sizeof(product*));
    unsigned int i;
    char *rowElement;
    for (i = 0; i < *nr; i ++){
        fgets(row, 45, f);
        rowElement = strtok(row, " ");
        strcpy((*products)[i].name, rowElement);
        rowElement = strtok(NULL, " ");
        strcpy((*products)[i].manufacturer, rowElement);
        rowElement = strtok(NULL, " ");
        rowLength = strlen(row);
        if (row[rowLength- 1] == '\n'){
            rowLength--;
            row[rowLength] = '\0';
        }
        (*products)[i].price = stringToNr(rowElement);
    }
    free(row);
    fclose(f);
}

Obviously the program has more features, but those work fine.

int main(){
    product *products;
    unsigned int nr;
    readList(&products, &nr);
    printList(products, nr);
    free(products);
    return 0;
}

My input file looks like this:

   3
   AAA FactoryA 300
   BBB FactoryC 550
   ZZZ Factory5 100

Use a debugger. It will save you (and us) alot of time if you learn to do that. At the very least it will immediately point you to the line of code that is triggering the crash. — kaylum
– kaylum, Commented Jan 5, 2017 at 22:06
Put code into the question (properly formatted) and not into the comments where it is hard to read and easy to miss. — kaylum
– kaylum, Commented Jan 5, 2017 at 22:08
products = malloc((*nr) * sizeof(product*)); -> products = malloc((*nr) * sizeof(product)); — Unimportant
– Unimportant, Commented Jan 5, 2017 at 22:10
In the readList() function, you are allocating only an array of (product* before the loop products = malloc((*nr) * sizeof(product*));. You shall also allocate for each item a product to store it ==> products[i]=malloc(sizeof(product)); — J. Piquard
– J. Piquard, Commented Jan 5, 2017 at 22:18

chux · Accepted Answer · 2017-01-05 22:32:36Z

4

Code ignores value of products.

What ever readList() receives in products is overwritten with the malloc() call.

void readList(product **products, unsigned int *nr){
    ...
    // bad
    products = malloc((*nr) * sizeof(product*));

Instead, use *products. Also allocate by the size of the referenced variable, not by the size of the type. Easier to code, review and maintain.

    *products = malloc(sizeof *(*products) * (*nr));
    if (*products == NULL) Handle_OOM();

Minor: After fgets(row, ..., ...); , following is not safe from a hacker exploit of reading an initial null character.

    rowLength = strlen(row);
    // What happens when rowLength == 0
    if (row[rowLength- 1] == '\n'){
      ...

Instead code could use below to rid the optional trailing '\n'.

    row[strcspn(row, "\n")] = '\0';

edited Jan 5, 2017 at 22:32

answered Jan 5, 2017 at 22:26

chux

158k17 gold badges160 silver badges311 bronze badges

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

8 Comments

chux Over a year ago

@user3529379 Think about what is products. It is a pointer to a pointer of product. Everywhere in readList(), code needs to use (*products). If need more info, please detail: "I can't understand" as it is too broad.

user3529379 Over a year ago

Sorry for being too vague, I don't understand why we're allocating sizeof *(*products) instead of sizeof (product) (or even sizeof *products).

chux Over a year ago

@user3529379 Simplified example: Suppose ptr is a pointer to type something and we want ptr to point to n something in memory. What is sizeof *ptr? It is the size of one something. ptr = malloc(sizeof *ptr * n) allocates memory for n something. Good so far?

raymai97 Over a year ago

I think that in this case, *products = malloc(sizeof(product) * (*nr)); will give the same result and easier to read.

chux Over a year ago

@user3529379 Now substitute *products for ptr and *nr for n --> *products = malloc(sizeof *(*products) * (*nr));. Note that extra () added for explicitness.

|

Collectives™ on Stack Overflow

Dynamically allocating array of structs C

1 Answer 1

8 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

8 Comments

Your Answer

Sign up or log in

Post as a guest

Related