What is causing undefined behaviour in my use of realloc

Question

I'm trying to write a function that takes in position data, an array and a counter that stores number of elements in that array and populates that array with new position data and increments the counter. And since I need this array outside of the function, I'm dynamically allocating memory using realloc.

Code

I have a struct to store co-ordinate data

typedef struct CoOrdinates
{
  int x;
  int y;

} CoOrdinates;

and a function that draws a box at any given position

void drawBox(WINDOW*       inWindow,
             int           aAtPositionX,
             int           aAtPositionY,
             int*          aOccupiedCoordinateCount,
             CoOrdinates** aOccupiedCoordinates)
{

  int    newCount       = (*aOccupiedCoordinateCount) + 1;
  size_t sizeResized    = sizeof(**aOccupiedCoordinates) * newCount;
  *aOccupiedCoordinates = realloc(*aOccupiedCoordinates, sizeResized);

  if (*aOccupiedCoordinates == NULL)
    {

      fprintf(stderr, "Failed to allocate CoOrdinate array at %d in %s.", __LINE__, __FILE__);
      exit(-1);
    }
  else
    {
      (*aOccupiedCoordinates)[newCount].x = aAtPositionX;
      (*aOccupiedCoordinates)[newCount].y = aAtPositionY;

      *aOccupiedCoordinateCount = newCount;

      mvwprintw(inWindow, aAtPositionY, aAtPositionX, "██");
    }
}

where mvwprintw and WINDOW* are defined in ncurses.

Another function is drawTwoVerticleBoxes defined like this

void drawTwoVerticleBoxes(WINDOW*       inWindow,
                          int           aAtPositionX,
                          int           aAtPositionY,
                          int*          aOccupiedCoordinateCount,
                          CoOrdinates** aOccupiedCoordinates)
{

  drawBox(inWindow, aAtPositionX, aAtPositionY, aOccupiedCoordinateCount, aOccupiedCoordinates);
  drawBox(inWindow, aAtPositionX, aAtPositionY + 1, aOccupiedCoordinateCount, aOccupiedCoordinates);
}

and a function called drawWithColor

void drawWithColor(WINDOW*       inWindow,
                   int           aAtPositionX,
                   int           aAtPositionY,
                   int*          aOccupiedCoordinateCount,
                   CoOrdinates** aOccupiedCoordinates)
{
  init_pair(1, COLOR_GREEN, COLOR_BLACK);
  wattron(inWindow, COLOR_PAIR(1));

  drawTwoVerticleBoxes(
      inWindow, aAtPositionX, aAtPositionY, aOccupiedCoordinateCount, aOccupiedCoordinates);

  wattroff(inWindow, COLOR_PAIR(1));

Finally all of those are called in renderRoutine

void renderRoutine()
{

  ...
  ...

  int            occupiedCoordinateCount = 0;
  CoOrdinates*   occupiedCoordinates     = NULL;


  drawWithColor(windowPointer, 4, 4, &occupiedCoordinateCount, &occupiedCoordinates);

  wgetch(windowPointer);

  endwin();

  for (int i = 0; i < occupiedCoordinateCount; ++i)
    {
      printf("X: %d, Y: %d \n", occupiedCoordinates[i].x, occupiedCoordinates[i].y);
      free(occupiedCoordinates);
    }
}

Summery

The idea is, every time a box is drawn, occupiedCoordinates will store CoOrdinate of that position. occupiedCoordinateCount will store how many items occupiedCoordinates have.

Errors

Expected results

X: 4, Y: 4
X: 4, Y: 5

Actual results


X: 0, Y: 0
X: 504864784, Y: 22033

free(): double free detected in tcache 2

Tweaks

If I define a function say drawFiveHorizontalBoxes

void drawThreeHorizontalBoxes(WINDOW*       inWindow,
                              int           aAtPositionX,
                              int           aAtPositionY,
                              int*          aOccupiedCoordinateCount,
                              CoOrdinates** aOccupiedCoordinates)
{

  drawBox(inWindow, aAtPositionX, aAtPositionY, aOccupiedCoordinateCount, aOccupiedCoordinates);
  drawBox(inWindow, aAtPositionX + 1, aAtPositionY, aOccupiedCoordinateCount, aOccupiedCoordinates);
  drawBox(inWindow, aAtPositionX + 2, aAtPositionY, aOccupiedCoordinateCount, aOccupiedCoordinates);
  drawBox(inWindow, aAtPositionX + 3, aAtPositionY, aOccupiedCoordinateCount, aOccupiedCoordinates);
  drawBox(inWindow, aAtPositionX + 4, aAtPositionY, aOccupiedCoordinateCount, aOccupiedCoordinates);
}

Expected results

X: 4, Y: 4
X: 5, Y: 4
X: 6, Y: 4
X: 7, Y: 4
X: 8, Y: 4

Actual results

realloc(): Invalid next size

core dumped

Edit

          (*aOccupiedCoordinates)[*aOccupiedCoordinateCount].x = aAtPositionX;
          (*aOccupiedCoordinates)[*aOccupiedCoordinateCount].y = aAtPositionY;

or

          (*aOccupiedCoordinates)[newCount - 1].x = aAtPositionX;
          (*aOccupiedCoordinates)[newCount - 1].y = aAtPositionY;

shows

X: 4, Y: 4
X: -1300275184, Y: 21961
free(): double free detected in tcache 2

in drawThreeHorizontalBoxes.

Why are you using a double pointer for ` CoOrdinates** aOccupiedCoordinates`? It seems that you only have a 1D array of Coordinates — Rishikesh Raje, Feb 06 '20 at 08:50
@RishikeshRaje: OP is passing the pointer around so that it can be realloced. The same is with the count, the idea is to update it inside the function. I agree it's not the best idea. — vgru, Feb 06 '20 at 08:52
Atis, make your life simpler by defining a struct which represents a list of coordinates. Create it in a separate unit (c file), so that it has functions which operate on this list (add/remove elements, get count, etc). Ideally it should have a capacity which is slightly larger than the current count, and be automatically resized as needed. [This thread](https://stackoverflow.com/a/3536261/69809) shows an example of such list, you would do the same thing but with coordinates instead of integers. Then when you test it, you can pass it around your code and worry about the higher-level problems. — vgru, Feb 06 '20 at 08:54

Some programmer dude · Accepted Answer · 2020-02-06T09:05:31.030

1

One problem is this:

(*aOccupiedCoordinates)[newCount].x = aAtPositionX;
(*aOccupiedCoordinates)[newCount].y = aAtPositionY;

Here newCount is the size of the "array", which will be one out of bounds of the allocated memory.

Use the old counter *aOccupiedCoordinateCount instead (or subtract one from newCount in the index).

A second problem is that you free the memory inside the loop where you print the values:

for (int i = 0; i < occupiedCoordinateCount; ++i)
  {
    printf("X: %d, Y: %d \n", occupiedCoordinates[i].x, occupiedCoordinates[i].y);
    free(occupiedCoordinates);
  }

The first iteration will free the memory, so each other iteration will use memory that isn't owned by you anymore, and try to free it again.

The free call should be outside of the loop.

edited Feb 06 '20 at 09:05

answered Feb 06 '20 at 08:47

Some programmer dude

400,186
35
402
621

Now it works prefectly if called once, but if I call it from `drawThreeHorizontalBoxes` I get this error `X: 4, Y: 4 X: 335224848, Y: 22062 free(): double free detected in tcache 2 Aborted (core dumped) ` – atis Feb 06 '20 at 08:59
@atis Updated with the second problem – Some programmer dude Feb 06 '20 at 09:05
OH NO! I don't know how I missed that! Thanks for the help and your time. – atis Feb 06 '20 at 09:08

What is causing undefined behaviour in my use of realloc

Code

Summery

Errors

Expected results

Actual results

Tweaks

Expected results

Actual results

Edit

1 Answers1