0

In my project I use a class for paged memory allocation. This class uses a struct to store all of its allocations:

enum PageStatus : uint_fast8_t {    //!< possible use stati of an allocated page
    PAGE_STATUS_INVALID     = 0b00,
    PAGE_STATUS_FREE        = 0b01, //!< the page is free
    PAGE_STATUS_USED        = 0b10, //!< the page is (partially) used
};

struct PhysicalPage {    //!< represents a page that has been allocated
    char* pData;         //!< pointer to the allocation
    PageStatus status;   //!< status of the allocation
};

These PhysicalPages are stored in a vector std::vector<PhysicalPage> physicalAllocations {};. During runtime pages are added to the vector and some may be removed. During the removal the last element is popped and the memory is returned using delete page.pData. However a problem occurs when the allocator class reaches end of life and gets deallocated from the stack. When pyhsicalAllocations's vector destructor is called it tries to destruct not only the elements themself but also the reserved memory (which the vector keeps as a buffer for when the size is changed). That causes invalid memory pointers to be deleted, stopping program execution:

double free or corruption (!prev)
Signal: SIGABRT (Aborted)

It's probably also worth mentioning that allocations are done in chunks larger than pages, which means that only one in every x pointers are actually valid allocations. All other pointers are just offsets from the actual memory locations.

To prevent the error from occurring I tried:

deleting manually (this is a bit overcomplicated due to chunked allocation)

for (size_t i = physicalAllocations.size(); 0 < i; i -= 1 << allocationChunkSize) {
    delete physicalAllocations[i - (1 << allocationChunkSize)].pData;
    for (size_t a = 0; a < 1 << allocationChunkSize; a++)
        physicalAllocations.pop_back();
}

clearing the vector

physicalAllocations.clear();

swapping for a clear vector

std::vector<PhysicalPage>(0).swap(physicalAllocations);

of which none worked.

I've been working on this problem for a lot longer that I would like to admit and your help is very much appreciated. Thanks!

RedNicStone
  • 37
  • 6
  • `std::shared_ptr pData;` and its [aliasing constructor (8)](https://en.cppreference.com/w/cpp/memory/shared_ptr/shared_ptr) might help. (that might even allow to get rid of `PageStatus`). – Jarod42 Nov 29 '21 at 13:07
  • 1
    I would expect `delete []`, not `delete`. Using the wrong form has undefined behaviour. – molbdnilo Nov 29 '21 at 13:14
  • `delete` sounds like the culprit, but it's difficult to tell. Please provide a MWE / MRE. – bitmask Nov 29 '21 at 13:19
  • @molbdnilo You are right, delete[] is the correct form. Changing it however dosen't fix the issue – RedNicStone Nov 29 '21 at 13:42
  • @bitmask working on it, I will add it to the original question when done. – RedNicStone Nov 29 '21 at 13:42
  • @RedNicStone [Rule of 3 violation](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). Go to that link, and look at the **Managing resources** section. That class in that example looks familiar, doesn't it? It looks almost identical to yours. – PaulMcKenzie Nov 29 '21 at 13:43
  • @Jarod42 not entirely sure how your solution should work. Is the idea to store a vector of shared pointers holding the allocation? Given that the reason for the error is deleting invalid pointers I don't think that would resolve the issue – RedNicStone Nov 29 '21 at 13:44
  • Bottom line is that you need to define user-defined copy constructor and assignment operation (at the very least). You cannot get out of this situation with code that swaps and clears the vector or whatever other "tricks" you're trying to do. The `std::vector` will make copies, move stuff around, etc. and will be using your `PhysicalPage` copy semantics, which right now are broken. – PaulMcKenzie Nov 29 '21 at 13:49
  • @PaulMcKenzie not entirely sure what to take from that thread. I can't add a destructor to `PhysicalPage` that frees the memory since not all pages actually relate to memory. A solution to this would be to instead use two different structs with different destructors that are inheriting from the same struct but I doubt that would solve the issue. – RedNicStone Nov 29 '21 at 13:51
  • Make the struct moveable and turn off the copy operations. Right now, you cannot store `PhysicalPage` in a vector until you fix the broken copy semantics, or add move semantics and call vector functions that don't require copying to occur (the compiler will flag you anyway if a copy is occurring, once the copy ctor and assignment operator are disabled). Again, doing tricks will not work -- you will go crazy trying to make vector "work" with the object. – PaulMcKenzie Nov 29 '21 at 13:53
  • @PaulMcKenzie sorry I dident see your comment until now. I forgot vectors will perform these operations and adding copy constructors and assignment operators might help. From my understanding though the trivial copy constructor should already *just* copy the PageStatus and the pData pointer which is desired behaviour. – RedNicStone Nov 29 '21 at 13:54
  • Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/239669/discussion-between-rednicstone-and-paulmckenzie). – RedNicStone Nov 29 '21 at 13:56

1 Answers1

1

std::shared_ptr<char[]> pData and its aliasing constructor (8) might help. (that might even allow to get rid of PageStatus).

It would look something like:

constexpr std::size_t page_size = 6;

struct PhysicalPage {
    std::shared_ptr<char[]> pData;
};

int main()
{
    std::vector<PhysicalPage> pages;
    {
        std::shared_ptr<char[]> big_alloc = std::unique_ptr<char[]>(new char[42]{"hello world. 4 8 15 16 23 42"});

        for (std::size_t i = 0; i != 42 / page_size; ++i) {
            pages.push_back(PhysicalPage{std::shared_ptr<char[]>{big_alloc, big_alloc.get() + i * page_size}});
        }
    }
    pages.erase(pages.begin());
    pages.erase(pages.begin() + 2);    
    for (auto& p : pages) {
        std::cout << std::string_view(p.pData.get(), page_size) << std::endl;
    }
}

Demo

Jarod42
  • 203,559
  • 14
  • 181
  • 302
  • Testing the changes right now.... – RedNicStone Nov 29 '21 at 14:46
  • I tested the changes and beside the memory lookup time being 2x higher than previously (with might be due to this being a development build with little optimization), it still fails spectacularly when trying to remove non-valid pointers. This might be a implementation error on my side, and ill try reimplementing it again later. – RedNicStone Nov 29 '21 at 14:58
  • 1
    If you provide MCVE, we can pinpoint the error. Benchmarking erroneous code is pointless ;-) – Jarod42 Nov 29 '21 at 15:02
  • Thanks! I got it working now. Time to lookup an address still increased from 8ns to 19ns but I believe that is only due to missing optimization. – RedNicStone Nov 29 '21 at 15:05