C - realloc with array of structs

Question

I'm doing a array of structs to dynamic allocate a list of products but this works only for a few times (3~5 times) and then I got this error.

* Error in `./test': realloc(): invalid next size: 0x000055bc0b44f260 *

Here is my code, this is part of a work for college.

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

typedef struct {
    int cod;
    float price;
} Product;

int main () {

    FILE *fp;

    fp = fopen("products.txt", "r+");

    if(fp == NULL)
        fopen("products.txt", "w+");

    int control = 1;
    int choice;
    int tam = 0;
    int i;
    Product *p1;

    p1 = malloc(sizeof(Product));

    while(fread(&p1[0], sizeof(Product), 1, fp) != NULL){
        tam++;
    }

    if(tam > 1){
        rewind(fp);

        p1 = malloc((tam) * sizeof(*p1));

        for (int i = 0; i < tam; i++){
            fread(&p1[i], sizeof(Product), 1, fp);
        }
    }

    rewind(fp);

    do {
        printf("1 - Add product\n2 - Show all products\n3 - Exit\n-> ");
        scanf(" %d", &choice);

        switch (choice) {
            case 1:
                 if (tam == 0) {
                    //p1 = malloc(sizeof(Product));
                    printf("Digit the product code: ");
                    scanf(" %d", &p1[tam].cod);
                    printf("Digit the product price: ");
                    scanf(" %f", &p1[tam].price);
                    tam++;
                } else {
                    printf("***Realloqing: %d***\n", tam * sizeof(*p1));
                    p1 = (Product*)realloc(p1, (tam) * sizeof(Product));
                    for (i = tam; i > 0; i--) {
                        p1[i].cod = p1[i-1].cod;
                        p1[i].price = p1[i-1].price;
                    }
                    printf("Digit the product code: ");
                    scanf(" %d", &p1[0].cod);
                    printf("Digit the product price: ");
                    scanf(" %f", &p1[0].price);
                    tam++;
                }
            break;
            case 2:
                for (i = 0; i < tam; i++) {
                    printf("Product code: %d\nProduct price: %f\n", p1[i].cod, p1[i].price);
                }
            break;
            case 3:
                control = 0;
            break;
        }

    } while (control);

    for (int i = 0; i < tam; i++){
        fwrite(&p1[i], sizeof(Product), 1, fp);
    }

    fclose(fp);

    free(p1);

    return 0;
}

A couple of things: First you should not assign back to the pointer you pass to `realloc`, if `realloc` fails and returns a null pointer you will lose the original pointer and have a memory leak. Then you should read [this discussion about casting `malloc` (and friends)](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc/605858). — Some programmer dude, Nov 10 '16 at 14:36
As for your problem, it seems that you are writing out of bounds of the memory you allocate. You might want to use a tool such as [Valgrind](http://valgrind.org/) to help you find such problems. — Some programmer dude, Nov 10 '16 at 14:37
`fopen("products.txt", "w+");` -> `fp = fopen("products.txt", "w+");` — Jabberwocky, Nov 10 '16 at 14:47
`tam` is int, and `sizeof` yields `size_t` (an unsigned type). Be careful when doing things like `int * size_t` — Elias Van Ootegem, Nov 10 '16 at 15:27

score 1 · Answer 1 · answered Nov 10 '16 at 14:37

1

This:

p1 = (Product*)realloc(p1, (tam) * sizeof(Product));

Violates the First Rule of Realloc, which is that you must not assign the result of realloc directly to the same variable which is passed as its first argument. When you do, if it fails, you have lost (leaked) the old pointer.

Second, you do not check the return values from various functions, such as malloc(), realloc(), and scanf(). This too is a cardinal sin.

If fixing all that still leaves you with a broken program, use valgrind.

answered Nov 10 '16 at 14:37

John Zwinck

239,568
38
324
436

3

Are you sure about the "First Rule of Realloc"? The [man page](http://pubs.opengroup.org/onlinepubs/009695399/functions/realloc.html) says: _If the new size of the memory object would require movement of the object, the space for the previous instantiation of the object is freed._ – David Ranieri Nov 10 '16 at 14:40
3

@KeineLust: Yes I am sure. Note that I said "if it fails." You are assuming it succeeds. – John Zwinck Nov 10 '16 at 14:43
Where can I read more about these First Rule etc, commandments? – 2501 Nov 10 '16 at 14:50
Here, it still fails. BTW `realloc`, `malloc` etc. returning `NULL` is very unlikely to happen in toy programs. – Jabberwocky Nov 10 '16 at 14:52
@Anaboth, oh, I just posted an answer, but yes that was the problem. – Jabberwocky Nov 10 '16 at 15:05

score 1 · Accepted Answer · answered Nov 10 '16 at 15:05

The problem is here:

    printf("***Realloqing: %d***\n", tam * sizeof(*p1));
    p1 = (Product*)realloc(p1, tam * sizeof(Product));

When tam is, say 1, then you realloc tam * sizeof(Product), but you need to realloc (tam + 1) * sizeof(Product), because now you need space for 2 products.

So this fixes the problem:

    printf("***Realloqing: %d***\n", (tam + 1) * sizeof(*p1));
    p1 = (Product*)realloc(p1, (tam + 1) * sizeof(Product));

C - realloc with array of structs

2 Answers2