Function not saving data to pointer to structure after realloc

Question

This function is supposed to save data to a library.books_count instance of a dynamic array of pointers to structures. Yet it does not. A similar function addexistingBooks() does it flawlessly. What is the problem in realloc()?

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

typedef struct
{
    char book_name[32];
    char book_genre[32];
    char author[32];
    int page_count;
    float price;

}Sbook;

typedef struct
{
    char library_name[32];
    Sbook * bookp;
    int books_count;
}Slib;

void menu(char String[50]);
void addexistingBooks(Slib library, int i);
void addBook(Slib library, int i);

int main()
{
    Slib library;
    int i=0;
    char Choice[30];
    printf("Enter amount of books inside the library: ");
    scanf("%d", &(library.books_count));
    library.bookp = (Sbook *)calloc(library.books_count,sizeof (Sbook));
    fflush(stdin);
    addexistingBooks(library, i);

    menu(Choice);
    if(strcmp(Choice,"add")==0)
    {
        addBook(library, i);
    }
    free(library.bookp);
    return 0;
}

void menu(char String[30])
{
    printf("Welcome to the library. If you read about heresy, prepare to be purged \n");
    printf("Please choose a command, by writing the appropriate command: \n");
    printf("1. Write 'add' to add a book. \n");
    printf("2. Write 'remove' to remove a book. \n");
    printf("3. Write 'redact' to redact a book. \n");
    printf("4. Write 'Sort by criteria' to sort the books, where criteria can stand for: 1.bookname, 2.author, 3.genre, 4.price. \n");
    printf("Enter your command: ");
    gets(String);
}

void addexistingBooks(Slib library, int i)
{
    for(i=0;i<library.books_count;i++)
    {
        printf("Enter the name of the book: \n");
        fgets(library.bookp[i].book_name,32,stdin);
        printf("Enter the genre of the book: \n");
        fgets(library.bookp[i].book_genre,32,stdin);
        printf("Enter the author of the book: \n");
        fgets(library.bookp[i].author,32,stdin);
        printf("Enter the page count of the book: \n");
        scanf("%d", &(library.bookp[i].page_count));
        printf("Enter the price of the book: \n");
        scanf("%f", &(library.bookp[i].price));
        fflush(stdin);
    }
}

void addBook(Slib library, int i)
{
        (library.books_count)++;
        realloc(library.bookp,library.books_count);
        fflush(stdin);
        if(library.bookp==NULL)
        {
            exit(1);
        }
        printf("Enter the name of the book: \n");
        fgets(library.bookp[i].book_name,32,stdin);
        printf("Enter the genre of the book: \n");
        fgets(library.bookp[i].book_genre,32,stdin);
        printf("Enter the author of the book: \n");
        fgets(library.bookp[i].author,32,stdin);
        printf("Enter the page count of the book: \n");
        scanf("%d", &(library.bookp[i].page_count));
        printf("Enter the price of the book: \n");
        scanf("%f", &(library.bookp[i].price));
        fflush(stdin);
}

You should avoid `fflush(stdin);`. It is undefined behavior. — Roberto Caboni, Apr 27 '20 at 20:28
See the docs for [realloc](https://en.cppreference.com/w/c/memory/realloc), in particular the return value which you now discard. — dxiv, Apr 27 '20 at 20:29
There are actually many errors in this code. The use of return value of `realloc` is only one of them.. — Roberto Caboni, Apr 27 '20 at 20:40
@pm100 In the first function a local variable `i` shadows the parameter, in the second one it is actually passed 0, and it overwrites the first inserted book. Furthermore it is redundant, since book_count could be used. I'll update my answer. — Roberto Caboni, Apr 27 '20 at 21:24

score 0 · Answer 1 · answered Apr 27 '20 at 20:41

The definition of realloc function,

 void *realloc(void *ptr, size_t size);

So, your realloc function:

realloc(library.bookp,library.books_count);

should change to:

library.bookp = realloc(library.bookp,sizeof(Sbook)*library.books_count);

OT, i see in your menu function, you use gets. It's dangerous, you should use fgets from stdin in stead. See in this link Why is the gets function so dangerous that it should not be used?

Roberto Caboni · Accepted Answer · 2020-04-28T07:59:24.837

Your code contains several mistakes.

Lets start from the "non-blocking" mistakes. Those that, even if they can be really critical and have to be corrected, are not the real cause of the crash you experience.

The flush of standard input, fflush(stdin); is something that is not defined by the standard, so using it leads to undefined behavior: in some environments it could work, in some other environment it could not work, and there can be environments (the worst ones) in which it seems to work but it is actually harmful. It is recommended to avoid it.
Function gets() is dangerous because it doesn't provide any control on the size of the string inserted by the user, and its use should be avoided.

The issues in the `void addBook()` function

You try increasing the available space using realloc:

void *realloc(void *ptr, size_t size);

It needs the original pointer and the new size. But you pass library.books_count that is just the number of books. Meaning that, if the library used to contain 4 books, you try to allocate 5 bytes only.

You instead need to allocate library.books_count * sizeof(Sbook) bytes.

Furthermore it returns a new pointer. You need to assign it to the book pointer:

library.bookp = realloc(library.bookp, library.books_count * sizeof(Sbook));

In main() you initialize a i variable, but you never update it, since you store the numbers of books directly in library.books_count.

Then you pass it to addexistingBooks(), and it is redundant because you could use library.books_count itself for the loop, as you actually do. You can use it as the loop variable, but you don't need to have that parameter. Just

void addexistingBooks(Slib library, )
{
    int i; /* If your C version is C99 or later, you can declare it in the loop itself */

    for(i=0;i<library.books_count;i++)
    {
        /* Omissis */
    }
}

Finally you pass it to addBook(), and not only it is redundant (as you can simply store the new book at index library.books_count-1, but it is actively harmful because you are always updating index 0 (because the value of parameter i is 0).

Although it is possible to pass structures to functions as values, it is not recommended. The first reason is that you will overload the stack of the process (the whole struct will be allocated in the stack area, that is quite big in PC applications but quite limited in embedded systems). The second reason will give you functional problems.

In fact, parameters passed by value are copies of the variables passed to the function. This means that any change made on them will not be reflected to original structures. In your case, the update of library.bookp pointer will not be available outside the function, causing (1) the original structure pointing to an invalid memory address location (becoming a dangling pointer), (2) the leak of the newly allocated memory, that nobody will be able to free().

Pass structures by address, instead, using pointers to structures. addBook() function, considering the i parameter removal, would become as follows

void addBook(Slib *library)
{
    int i = library->books_count;

    (library->books_count)++;
    library->bookp = realloc(library->bookp, library->books_count * sizeof(Sbook));

    /* and so on... the value of 'i' is now library->books_count-1 */
}

/* Call from main */
int main()
{
    Slib library;

    /* Omissis */

    menu(Choice);
    if(strcmp(Choice,"add")==0)
    {
        addBook(&library, i);
    }
    free(library.bookp);
    return 0;
}

Function not saving data to pointer to structure after realloc

2 Answers2

The issues in the void addBook() function

The issues in the `void addBook()` function