Valgrind's invalid write

Question

The class:

class Tile {
public:
  long long size, joint;
};

My main has:

int main() {
  //
  double minW, minL;
  unsigned tileCap = 10, tileCount = 0;
  bool wasTransformed = false;
  auto *tiles = (Tile*)calloc(tileCap, sizeof(Tile) );

  GetInput(&minW, &minL, &tiles, &tileCount, &wasTransformed);

   //etc.
}

The problematic function here is GetInput():

   void GetInput(double *w, double *l, Tile **tiles, unsigned *tileCount, bool *needTransform) {
  //
  printf("Min dimensions:\n");

  if (scanf("%lf %lf", w, l) != 2)
    BadInput();
  if (!CorrectSize(*w) || *w == 0)
    BadInput();
  if (!CorrectSize(*l) || *w == 0)
    BadInput();

  unsigned tileCap = 10;
  *tiles = (Tile*)calloc(tileCap, sizeof(Tile) );

  printf("Tiles:\n");
  double tileSize, tileJoint;
  int argc;

  do {
    argc = scanf("%lf %lf", &tileSize, &tileJoint);
    if(argc == EOF) {
      break;
    }
    if (tileSize == 0 || !CorrectSize(tileSize) || !CorrectSize(tileJoint) || argc != 2)
      BadInput();

    if(! *needTransform) {
      *needTransform = HasFloatingPoint(tileSize) || HasFloatingPoint(tileJoint);
      if(*needTransform)
        TransformPrevious(*tiles, *tileCount);
    }
    if(*needTransform) {
      //transform this
      tileSize *= 10;
      tileJoint *= 10;
    }

    (*tiles)[*tileCount].size = (long long)tileSize + (long long)tileJoint;
    (*tiles)[*tileCount].joint = (long long)tileJoint;

    *tileCount += 1;
    if( (*tileCount) == tileCap) {
      DoubleArray(tiles, &tileCap);
    }
  } while(true);
}

And my DoubleArray():

void DoubleArray(Tile **array, unsigned *cap) {
   //
  auto *tmp = (Tile*)realloc(*array, 2 * sizeof(Tile) );

  if(tmp) {
    *array = tmp;
    *cap *= 2;
    (*cap)--;
  } else {
    printf("Error allocating memory.\n");
  }
}

Running the program seems fine, no errors shown and the results seem to be correct. For example:

360 217
0.1 0.0
0.2 0.0
0.3 0.0
0.4 0.0
0.6 0.0
0.8 0.0
1.2 0.0
2.4 0.0
4.1 0.0
8.2 0.0
12.3 0.0
16.4 0.0
24.6 0.0
32.8 0.0
49.2 0.0

Valgrind at 12.3 0 prints Invalid write of size 8. So it seems to me that I'm reallocating the memory incorrectly. But if I am, why are the values loaded normally if I print them out? In other words, all inputs load into the array correctly.

So what am I doing incorrectly? Using memset? Not freeing correctly?

Your question is tagged C++, but you make an intensive use of C library functions. `calloc` and `free` should not be used under normal use cases (unless you are writing a dedicated allocator or a `operator new` function). Anyway, you never free allocated memory before reusing the pointer, and in `DoubleArray`, you seem to allocate original size *before* doubling the array capacity. Long story short, use `std::vector`, or learn on a simpler example how to use the low level C allocation functions. — Serge Ballesta, Nov 19 '17 at 19:57
@SergeBallesta why do I need to `free` the array when I'm reusing it? I free it after I stop reusing it, until then I just set it using `memset`. Is that incorrect? And I'm using C library functions because `vector` was not allowed. And the tag has been fixed. — Sorry, Nov 19 '17 at 20:19
If it is C, then [don't cast malloc](https://stackoverflow.com/q/605845/3545273). You calloc `tiles` and main, and then calloc it again in `GetInput`: the original block is leaked. — Serge Ballesta, Nov 20 '17 at 07:15
Anyway, `class`, `bool` and `auto` are not C. Idiomatic C would be `typedef struct _Tile { long long size, joint; } Tile;`. `(*tiles)[*tileCount]` is wrong: if you have allocated `tileCount` tiles, you are one position past end of array (indexes start at 0 in C). And `DoubleArray` only allocates 2 tiles which is probably not what is expected. Trim all down to a [mcve] if you want help here or carefully control all allocated sizes and please chose one language, either C or C++. — Serge Ballesta, Nov 20 '17 at 08:24
@SergeBallesta C has bool support, `class` is basically `struct` with different default encapsulation. `tileCount` shouldn't go past the end of array since I have a condition that doubles the array if needed (my assumption). The reason I use class instead of struct is because it's shorter. The compiler used is g++ anyway. Auto is what my IDE changed. Anyway, the `realloc` was incorrect as SOMEBODY ELSE pointed out. — Sorry, Nov 20 '17 at 11:54
`class` does not exist in C full stop. And if you write a C program, the compiler should be gcc not g++. A C++ compiler normally accepts most of C code, but there are some differences. **C and C++ are different languages** — Serge Ballesta, Nov 20 '17 at 12:32

score 2 · Accepted Answer · answered Nov 20 '17 at 11:02

You are reallocating the memory incorrectly. The interfaces to std::malloc, std::calloc and std::realloc are somewhat inconsistent and confusing concerning the size to be allocated. std::malloc and std::realloc take a single size argument, which should be the number of objects times their size. std::calloc takes two arguments, one for the number of objects and one for the size of one object.

You are calling

auto *tmp = (Tile*)realloc(*array, 2 * sizeof(Tile) );

This (re)allocates space for two Tiles. You should write something like

*cap *= 2;
auto *tmp = (Tile*)realloc(*array, *cap * sizeof(Tile) );

The extra memory will be uninitialized so you may need to memset() it so 0.

score -1 · Answer 2 · edited Nov 19 '17 at 20:53

-1

void DoubleArray(Tile **array, unsigned *cap) {
  //
  auto *tmp = (Tile*)realloc(*array, *cap);

  if(tmp) {
    *array = tmp;
    *cap *= 2;
    (*cap)--;
  } else {
    printf("Error allocating memory.\n");
    free(tmp);
  }
}

tmp hasn't been reallocated (due to error), so why free() it?

edited Nov 19 '17 at 20:53

underscore_d

6,309
3
38
64

answered Nov 19 '17 at 19:38

Black cowgirl

59
3

1

That `free()` is pointless, but not dangerous. The Standard specifies that trying to `free()` a `NULL` pointer is fine and simply results in nothing happening. See: [Does free(ptr) where ptr is NULL corrupt memory?](https://stackoverflow.com/questions/1938735/does-freeptr-where-ptr-is-null-corrupt-memory) So this doesn't answer the question (unless the OP has a non-Standard-compliant compiler, but who cares about those?) and should really just be a comment, since it just asks a question rather than providing a solution. – underscore_d Nov 19 '17 at 20:55

Valgrind's invalid write

2 Answers2