5

I have troubles with memory. I'm using struct this way:

Package.h file

#pragma once
#include <cstdlib>

struct Package {
    char *data;
    long long int *packageNumber;
    long long int *allPackages;

    Package(const int sizeOfData);
    ~Package();
};

Package.cpp

#include "Package.h"

Package::Package(const int sizeOfData) {
    void *ptr = malloc(2 * sizeof(long long int) + sizeOfData * sizeof(char));
    packageNumber = (long long int*) ptr;
    allPackages = (long long int*) ((long long int*)ptr + sizeof(long long int));
    data = (char*)((char*)ptr + 2 * sizeof(long long int));
}

Package::~Package() {
    free(data);
    free(packageNumber);
    free(allPackages);
}

And in method:

for (int j = 0; j < this->bufforSize || i * bufforSize + j < allPackages; j++) {
            Package package(this->packageSize);
            this->file->read(package.data, this->packageSize);
            *package.allPackages = allPackages;
            *package.packageNumber = i * this->bufforSize + j;
            this->dataPacked->push_back(package);
        }

after end of brackets it throws error: "HEAP[zad2.exe]: Invalid address specified to RtlValidateHeap( 00000056FEFE0000, 00000056FEFF3B20 )" I have no idea what I'm doing wrong. Please help, Michael.

EDIT: Now it is working for first iterate of loop. Helps that I changed destructor to this:

Package::~Package() {
    free(packageNumber);
}

But now destructor is executed two time on same struct object in 2'nd iterate of loop.

PianistaMichal
  • 305
  • 1
  • 2
  • 14
  • Why tag this as `C` when it is obvious that you're using C++. And if this *is* C++, why not use `new [ ]` and `delete[ ]` or just a container such as `std::vector`? – PaulMcKenzie Nov 09 '15 at 16:21
  • Why are you calling `free` 3 times? There is only one call to `malloc` being done. – PaulMcKenzie Nov 09 '15 at 16:30
  • Becouse I have to do it in continuous memory fragment. Whole struct have to be in one place. – PianistaMichal Nov 09 '15 at 16:32
  • There is no magic in `malloc` as opposed to using `new char [ ]` or `std::vector`. They all do basically the same thing, which is allocate a contiguous chunk of memory in bytes. The difference, at least with `malloc` and `std::vector` is that you no longer have to manually manage the allocation and deallocation, which is why you have an error now. – PaulMcKenzie Nov 09 '15 at 16:35
  • @PaulMcKenzie Thanks, but I want to allocate memory in C-way. It looks better for me to allocate n-size array of char + m-size int for example. – PianistaMichal Nov 09 '15 at 17:03
  • *but I want to allocate memory in C-way* And the new problem you're having now with the destructor is because you are doing things the `C` way and trying to mix that with C++. If you're going to use `std::vector` to store your objects, then you have to decide whether you want to use standard containers for your data members, or use raw memory *and adhere to the "rule of 3"*, What you want is the latter, but your class fails to implement the rule of 3. – PaulMcKenzie Nov 09 '15 at 17:06
  • http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three – PaulMcKenzie Nov 09 '15 at 17:12

2 Answers2

8

Read the description of free:

The behavior is undefined if the value of ptr does not equal a value returned earlier by std::malloc(), std::calloc(), or std::realloc().

Then take a look at your code, pay attention to the comments I added.

void *ptr = malloc(2 * sizeof(long long int) + sizeOfData * sizeof(char));
packageNumber = (long long int*) ptr; // you got this from malloc
allPackages = (long long int*) ((long long int*)ptr + sizeof(long long int)); // the value of this pointer is not equal to anything returned by malloc
data = (char*)((char*)ptr + 2 * sizeof(long long int)); // the value of this pointer is not equal to anything returned by malloc either

And finally in the destructor:

free(data); // was not allocated with malloc -> undefined behaviour
free(packageNumber); // was allocated with malloc -> OK
free(allPackages); // was not allocated with malloc -> undefined behaviour

You try to delete pointers that you didn't get from malloc. This results in undefined behaviour. The error is due to the undefined behaviour. Do realize that free(packageNumber) frees the entire block of memory allocated with malloc. That includes the memory pointed by data and allPackages.

There is an easy rule of thumb: Call free exactly once per every call to malloc/calloc. Same applies to delete+new and delete[]+new[].

eerorika
  • 232,697
  • 12
  • 197
  • 326
  • It's great explanation. It helps, but there is related to this one more error now. It appers after this. Why my program executed second time destructor in second iterate of loop? – PianistaMichal Nov 09 '15 at 16:48
  • @PianistaMichal You're copying the object into a vector. I'd be willing to bet that you forgot to follow the rule of three and didn't implement a correct copy constructor. – eerorika Nov 09 '15 at 16:56
  • A vector makes copies of your object. Since your `Package` struct cannot be safely copied, by placing it in a vector, all of the bugs associated with copying `Package` objects become exposed. If you want to store `Package` objects in a container such as `std::vector`, you need to ensure it is safely copyable by implementing the rule of 3 (since you have members that are pointers to dynamically allocated memory). – PaulMcKenzie Nov 09 '15 at 17:04
0
allPackages = (long long int*) ((long long int*)ptr + sizeof(long long int));

When you use a long long int pointer(in our case it's ptr after being casted) and you want to advance sizeof(long long int) bytes, you just need to do ptr++;

But I suggest you to rewrite your code and use 3 mallocs instead of one.

Brahim
  • 808
  • 1
  • 8
  • 17
  • 1
    I suggest not writing any `malloc` calls. Just change `data` to `std::vector`, and you get the same results, all without managing any memory. – PaulMcKenzie Nov 09 '15 at 16:29