Creating a int * with malloc and then use realloc when full

Question

In a function I created am I trying to allocate a int array dynamically to store some index values.

First I create the int * with the malloc function and then let the loop store som values in it and increment the pointer each time. The problem I run in to starts when I try to use the realloc to increase the memory allocation.

When I do this VS tells me it runs in to undefined behaviour and breaks the program.

The code looks like this

void showAvailable(CabinHolder *holder, Booking *booking)
{
    system("cls");

    printf("Choose cabin number \n");
    printf("Start week: &d \t End week: %d", booking->rentPeriod[0], booking->rentPeriod[1]);
    printf("------------------------------------------\n");
    
    int memory = 5;
    int *indexOfCabin = (int *)malloc(sizeof(int)*memory);
    int counter = 1;
    
    for (int i = 0; i < CABINS; i++)
    {
        if (counter == memory)
        {
            memory *= 2;
            int *expanded = realloc(indexOfCabin, (memory * sizeof(int)));
            indexOfCabin = expanded;
            expanded = NULL;
        }

        if (booking->cabin->typeOfCabin == holder->arrofCabin[i].typeOfCabin)
        {
            printf("%d. \t Cabin with number %d \t cost: %d per week\n", counter, holder->arrofCabin[i].nr, holder->arrofCabin[i].cost);
            counter++;
            indexOfCabin = &i;
            indexOfCabin++;
        }
    }

    free(indexOfCabin);
    system("pause");
}

When I debugg in VS i also se that my pointer indexOfCabin seems to be undefined inside the if statement, which I don't understand. What have I missed here?

have a look at this question: https://stackoverflow.com/q/605845/812912 — Ivaylo Strandjev, Feb 14 '18 at 07:54
`indexOfCabin = &i` throws away the memory you allocated, and puts the address of `i` into the pointer. — user3386109, Feb 14 '18 at 07:56
@user3386109 actually realloc takes care of freeing the old memory if needed: http://en.cppreference.com/w/c/memory/realloc — Ivaylo Strandjev, Feb 14 '18 at 07:58
@user3386109 ah that's completely true now that you point it out. How do I instead store the index eg. 17 in indexOfCabin? — anderssinho, Feb 14 '18 at 07:59
@anderssinho please read the documentation of realloc. The statement is not true — Ivaylo Strandjev, Feb 14 '18 at 07:59
`indexOfCabin` can be used like an array, e.g. `indexOfCabin[counter] = i;`. But `counter` needs to start at 0, and should be incremented after being used. And `indexOfCabin` should not be incremented. — user3386109, Feb 14 '18 at 08:02
@user3386109 ah ofc. I'll try that and comeback if there is any problem. Thanks — anderssinho, Feb 14 '18 at 08:03
Hint (not directly related to your question but it might make your life easier): you should outsource the memory management to another function instead of mixing memory management and program logic in the same function. — Jabberwocky, Feb 14 '18 at 09:43
@MichaelWalz that's is also a good tips. Will break out that part in a function of it's own to make the coder easier to understand :) — anderssinho, Feb 14 '18 at 10:30

anderssinho · Answer 1 · 2018-02-14T09:12:17.557

0

Okay after some help in the comment section I solved the problem with this edited code segment.

void showAvailable(CabinHolder *holder, Booking *booking)
{
    system("cls");

    printf("Choose cabin number \n");
    printf("Start week: %d \t End week: %d\n", booking->rentPeriod[0], booking->rentPeriod[1]);
    printf("------------------------------------------\n");

    int memory = 5;
    int *indexOfCabin = malloc(sizeof(int)*memory);
    int counter = 1;
    int items = 0;
    int choice = 0;

    for (int i = 0; i < CABINS; i++)
    {
        if (counter-1 == memory)
        {
            memory *= 2;
            indexOfCabin = realloc(indexOfCabin, (memory * sizeof(int)));
        }

        if (booking->cabin->typeOfCabin == holder->arrofCabin[i].typeOfCabin)
        {
            printf("%d. \t Cabin with number %d \t cost: %d per week\n", counter, holder->arrofCabin[i].nr, holder->arrofCabin[i].cost);
            counter++;
            indexOfCabin[items++] = i;
        }
    }
    free(indexOfCabin);
    system("pause");
}

First: Problem was indexOfCabin = &i throws away the memory you allocated, and puts the address of i into the pointer instead of what I wanted to do. Now we store the index in i in the pointer.

Second: indexOfCabin can be used like an array, e.g. indexOfCabin[counter] = i;. But counter needs to start at 0, and should be incremented after being used. And indexOfCabin should not be incremented

edited Feb 14 '18 at 09:12

answered Feb 14 '18 at 08:18

anderssinho

298
2
7
21

4

You should *never* `realloc` the pointer itself (e.g. `indexOfCabin = realloc(indexOfCabin,...`, you should use a temporary pointer `void *tmp = realloc(indexOfCabin,...`, then check `if (tmp)` to validate the reallocation before assigning `indexOfCabin=tmp;`. Otherwise if `realloc` fails you overwrite your original pointer address with `NULL` causing a memory leak. – David C. Rankin Feb 14 '18 at 09:12
@DavidC.Rankin that's totally true, will change that in the code – anderssinho Feb 14 '18 at 09:13
2

That also means delaying `memory *= 2;` until after you validate `realloc` succeeded, so your `realloc` will look like `void *tmp = realloc (indexOfCabin, (memory * 2 * sizeof *indexOfCabin)); if (!tmp) {/* handle error */ break; }; indexOfCabin = tmp; memory *= 2;` – David C. Rankin Feb 14 '18 at 09:18
@DavidC.Rankin thanks the solution! And understand why you suggest this. It's now implemented in my code. Will update this post also with the change – anderssinho Feb 14 '18 at 10:32
@DavidC.Rankin: It really depends on what you are going to do when realloc fails. If all you are going to do is print a diagnostic and exit, then there's no point in going to the extra work of preserving the original value. Only if you have a way of actually recover from running out of memory is it useful to preserve it. In a library, you probably want to report the error back to the caller to allow them to recover and so need to keep the pointer valid. – Chris Dodd Mar 18 '23 at 00:41
@ChrisDodd - agreed. While you can always `assert(tmp)`, my goal is explaining the original pointer is left unchanged and still points to valid data, and showing recovery can be something more than crashing out of the program at that point. But, you are entirely correct, if all you want to do is exit when `realloc()` fails, then no need for the extra effort. – David C. Rankin Mar 18 '23 at 05:21

Creating a int * with malloc and then use realloc when full

1 Answers1