Allocation memory error with use struct for c

Question

I wrote a code for managing a library; the compilation is done but during the simulation I obtained an Allocation error (case2) and I don't know why. The first case works correctly but if I entered more than one name in the first case, the second case doesn't work.

What did I do wrong? I hope I was clear enough.

typedef struct {
    char name[80];
    char **books;
    int books_num;
} Subscription;

int main() {
    // Variables declaration:
    int option = 0, subs_num = 0, i = 0, books_num = 0;
    Subscription *subs_library;
    char **books;
    char subs_new_name[80], book_new_name[80];

    printf("Choose an option\n");
    do {
        scanf("%d", &option);
        switch (option) {
          case 1:
            printf("Case 1: enter a new name\n");
            scanf("%s", subs_new_name);
            if (subs_num == 0) {
                subs_library = malloc(sizeof(Subscription));
            } else {
                subs_library = realloc(subs_library, sizeof(Subscription));
            }
            strcpy(subs_library[subs_num].name, subs_new_name);
            subs_library[subs_num].books_num = 0;
            subs_num++;
            printf("ADDED\n");  
            break;

          case 2:
            printf("Case 2: enter the book name\n");
            scanf("%s", book_new_name);

            if (books_num == 0) {
                books = malloc(sizeof(char*));
                books[books_num] = malloc(80 * sizeof(char));
            } else {
                books = realloc(books, sizeof(char*));
                books[books_num] = malloc(80 * sizeof(char));
            }

            if (books[books_num] == NULL) {
                printf("Allocation Error\n");
                exit(1);
            }

            strcpy(books[books_num], book_new_name);
            books_num++;
            printf("ADDED\n"); 
            break;
        }
    } while (option != 7);
    return 0;
}

Answer 1

Your code to reallocate the arrays is incorrect. You do not allocate enough room for the new array sizes. When you reallocate these arrays, you pass the size of a single element, therefore the array still has a length of 1 instead of subs_num + 1. The size passed to realloc should be the number of elements times the size of a single element in bytes.

Initialize subs_library and books to NULL and change your array reallocations:

    if (subs_num == 0) {
        subs_library = malloc(sizeof(Subscription));
    } else {
        subs_library = realloc(subs_library, sizeof(Subscription));
    }

Into this:

    subs_library = realloc(subs_library, (subs_num + 1) * sizeof(*subs_library));

And do the same for books, change:

    if (books_num == 0) {
        books = malloc(sizeof(char*));
        books[books_num] = malloc(80 * sizeof(char));
    } else {
        books = realloc(books, sizeof(char*));
        books[books_num] = malloc(80 * sizeof(char));
    }

To this:

    books = realloc(books, (books_num + 1) * sizeof(*books));
    books[books_num] = malloc(80 * sizeof(char));

Or simpler:

    books = realloc(books, (books_num + 1) * sizeof(*books));
    books[books_num] = strdup(book_new_name);

Answer 2

The problem is your reallocation calls. For example you do

realloc(books,sizeof(char*))

This reallocates the memory pointed to be books to be one pointer to character in size, which is exactly what you already have. This will lead you to index out of bounds of the allocated memory, something which is undefined behavior.

If you want to allocate more than one element you need to multiply the base type size with the number of elements you want to allocate, e.g.

realloc(books, (books_num + 1) * sizeof(char *))

Answer 3

Your reallocation realloc(books, sizeof(char *)) only allocates the size of one pointer char *, not the size of the enlarged array that you need:

    books=realloc(books,sizeof(char*));

You need to multiply the size of a pointer (char *) by the number of books you plan on storing in the array. You maintain the number of books in books_num.

As Joachim Pileborg said, with every allocation/reallocation, you want this to be one more than the current size. For the first allocation (malloc()), you want to allocate for one book, which is 1 times sizeof(char *). This happens to be equivalent to your existing code, which is fine. But the reallocation (realloc()) reallocates for the same size every time (only enough for one pointer), so you're not enlarging the allocation. You need to multiply the size required for one pointer (sizeof(char *)) by the number of pointers you want, which is books_num + 1. As in Joachim's answer, this is

    books = realloc(books, (books_num + 1)*sizeof(char *));

This will enlarge the allocation of the array books by one more pointer. Then, on the next line you correctly allocate a string of size 80.

Your subs_library has the same reallocation issue.

Less frequent reallocation

You might want to resize an allocation less frequently. In this situation, you are reallocating every time you add an entry. One simple technique to reduce the number of reallocations is to double the allocation size every time it gets full. But you have to maintain the allocation size (capacity) and check for it whenever you add something. For example:

char **buffer;     /* buffer of pointers to char */
int capacity = 1;  /* number of elements allocated for */
int size = 0;      /* number of elements actually used */

Then the initial allocation is

/* Initial allocation */
buffer = malloc(capacity*sizeof(*buffer));

And to add some char *new_item to the buffer

/* When adding an element */
if ( size == capacity ) {
    /* Double allocation every time */
    capacity *= 2;

    /* Reallocate the buffer to new capacity */
    realloc(buffer, capacity*sizeof(*buffer));
}

/* Item will fit, add to buffer */
buffer[size++] = new_item;

Notice that I have used sizeof(*buffer) instead of sizeof(char *). This makes the compiler figure out what the type and size is. That way, if I change the type of buffer for some reason, I don't have to change more places in the code. Another thing that I left out for brevity is that you should always check the return values to make sure they are not NULL.

Answer 4

I guess the problem is with scanf reading a string only until a separator, in your case - a whitespace separating multiple names entered. The characters after separator remain in the input buffer and get immediately processed by other calls to scanf.

You should consider using getline for reading name(s) and checking return values from other calls to scanf.