-2

I have a class Block that inherits from a class case :

class Case {
public:
    Case(sf::Vector2f const& pos, sf::IntRect const& textureRect, int type = 1);

protected:
    int type_;

    sf::Vector2f pos_;
    sf::FloatRect hitBox_;
    sf::IntRect textureRect_;
};

class Block : public Case {

public:
    Block(sf::Texture* const& texture, sf::Vector2f const& pos, sf::IntRect const& textureRect, int const& type = 2);

    sf::Sprite& getSprite();

private:
    std::shared_ptr<sf::Texture> texture_;
    sf::Sprite sprite_;

};

(Both constructor are really basic, I'm not using any new anywhere)

and I have an unordered_map of unordered map to stock my blocks :

std::unordered_map<int, std::unordered_map<int, Block>> blocks_;

But when I try to delete one :

if(blocks_[i%lenght].find((i-i%lenght)/lenght) != blocks_[i%lenght].end())
    blocks_[i%lenght].erase((i-i%lenght)/lenght);

I get thos error :

double free or corruption (out)

I tried to print the destructor, only the destructor from Block is called before I get this error.

It's been around 2 hours I'm looking for a solution so I finally ask it here, Thanks !

Guillaume Magniadas
  • 176
  • 8
  • Have you run your code through `valgrind` or tried building with address sanitization turned on? – Stephen Newell Mar 17 '19 at 18:53
  • `i-i%lenght)/lenght` -- We have no idea what this equates to. Why not print out those values before you use them, so that you know if those are valid? – PaulMcKenzie Mar 17 '19 at 18:58
  • It's just a calcul to recover the Y position on contiguous memory, I printed it do get the good key, no problem with that. – Guillaume Magniadas Mar 17 '19 at 19:00
  • 3
    Without complete and verifiable example it’s impossible to say for sure, but in your Block constructor you are passing a `sf::Texture` raw pointer, but in Block you have a `std::shared_ptr` to `sf::Texture`. I would guess you have multiple unrelated shared pointers to the same texture. – jo-art Mar 17 '19 at 19:04
  • Guess you'll have to use your debugger; There's no where near enough information here to guess what's going on - such as which object is double freed. – UKMonkey Mar 17 '19 at 19:06
  • Oh, you must be true, I'm a bit new to the shared/unique ptr etc.. maybe I did not initialize them properly – Guillaume Magniadas Mar 17 '19 at 19:07
  • @GuillaumeMagniadas you're aware that the shared_ptr takes ownership of the object? – UKMonkey Mar 17 '19 at 19:08
  • I did this : Block::Block(sf::Texture* const& texture, sf::Vector2f const& pos, sf::IntRect const& textureRect, int const& type) : Case(pos, textureRect, 2), texture_(texture), sprite_(*texture_, textureRect_) {} I think I should just use classic pointer – Guillaume Magniadas Mar 17 '19 at 19:09
  • You should not use a classic pointer, you should use only smart pointers with make_shared and make_unique. Without touching new/delete or even raw pointers (not until you are proficient with smart pointers) – Michael Veksler Mar 17 '19 at 19:14
  • @MichaelVeksler I agree with not using `new`/`delete`, but not using `raw pointers` is not correct, you can and should use raw pointers if you pass an object to a function in the cases when you don't transfere ownership, so if the object is only used for the time of the function call. – t.niese Mar 17 '19 at 19:17
  • @t.niese I think that raw pointers are relatively advanced, and should be used only after understanding ownership concepts of smart pointers. Of course we can learn 100% raw pointers, and only if proficient go to smart ones. But mixing is bad learning idea – Michael Veksler Mar 17 '19 at 19:19
  • @MichaelVeksler raw pointers belong to the concepts of owership. You should use raw pointers only as non owning pointers. If you do so and you have a constructor `Foo(Bar * const b) : x(b->x) {}` the caller already sees in the constructor signature `Foo(Bar * const b)` that `Foo` won't claim ownership on `b`. If you on the other hand write `Foo(const shared_ptr& b) : x(b->x) {}`, then the user of `Foo(const shared_ptr& b)` has to assume that `Foo` will claim ownership on `b` even so it does not. So raw pointers or references are an elementary part of the concepts of ownership. – t.niese Mar 17 '19 at 19:22
  • @MichaelVeksler as soon as [`std::experimental::observer_ptr`](https://en.cppreference.com/w/cpp/experimental/observer_ptr) are available you should stop using raw pointers to express non owning pointers. But until then you should always use references or raw pointers if you don't want to express ownership transfere. – t.niese Mar 17 '19 at 19:33
  • @t.niese you have a good point. But I'd say it is **slightly** weaker than that, since weak_ptr are also a type of non owing pointers. – Michael Veksler Mar 17 '19 at 19:48

1 Answers1

3

Your problem is with the constructor:

Block::Block(sf::Texture *const &texture, sf::Vector2f const &pos,
             sf::IntRect const &textureRect, int const &type)
    : Case(pos, textureRect, 2), texture_(texture),
      sprite_(*texture_, textureRect_) {}

While it is not necessarily wrong to write it that way, it would be wrong if you pass the same Texture to multiple Blocks:

sf::Texture *tex = new sf::Texture(/* ... params ... */);

Block b1(tex, /* ... remaining params ... */);
Block b2(tex, /* ... remaining params ... */);

Now two separate shared_ptr think that they are the only owner of tex, so as soon as one of those Blocks is deleted the Texture is deleted as well, so if both blocks are deleted then you have a double free.

As of that this is considered an anti pattern. In general if you work with smart pointers, then you should see a raw pointer as not owning pointers and you should never construct a smart pointer from a not owning pointer. (However, there is an exception to this rule, if you work with libraries that don't use smart pointers, then you need check if the raw pointer they return or receive are considered as owing pointers and if so it might be valid to convert that raw pointer to a smart pointer.)

Your code should look that way:

Block::Block(const std::shared_ptr<sf::Texture> &texture, sf::Vector2f const &pos,
             sf::IntRect const &textureRect, int const &type)
    : Case(pos, textureRect, 2), texture_(texture),
      sprite_(*texture_, textureRect_) {}

And you should construct your Blocks like that:

std::shared_ptr<Texture> tex = std::make_shared<Texture>(/* ... params ... */);

Block b1(tex, /* ... remaining params ... */);
Block b2(tex, /* ... remaining params ... */);

This does not mean that you should never use raw pointers. If you e.g. would have a function that would draw the texture to screen, then a raw pointer is perfectly fine:

void draw_texture( sf::Texture * const tex) {
   // do some drawing but don't store tex somewhere
   // so tex is only used within that draw_texture call
}

Then you would call it like that:

std::shared_ptr<Texture> tex = std::make_shared<Texture>(/* ... params ... */);

draw_texture(tex.get());
t.niese
  • 39,256
  • 9
  • 74
  • 101
  • `draw_texture( sf::Texture * const tex) ` is an anti pattern, since it makes the reader ask: is it an owing pointer? Who is responsible to free it? Will the draw function store it inside? Better is to pass a reference in such a case, unless nullptr is one of the valid arguments – Michael Veksler Mar 17 '19 at 19:56
  • @MichaelVeksler `is it an owing pointer? Who is responsible to free it?` no that is not true, if your project/library uses smart pointers then you should make clear and ensure that all raw pointers in your project/library are not owning pointers. Yes you can pass it as const reference if the operations won't modify the object, if you need to call a function on the object that is not const then you would pass it as a const pointer, and not as reference, because the reference might possibly change the object that was pass in. (passing as non const ref is seen as anti-pattern) – t.niese Mar 17 '19 at 20:02
  • @MichaelVeksler and if a const reference would not work, and you need to ensure that the pointer is not null then you would use a `not_null` as suggested in the [C++ Core Guidelines](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#i12-declare-a-pointer-that-must-not-be-null-as-not_null). An implementation of that type is e.g. available in the [Guidelines Support Library of Microsoft](https://github.com/Microsoft/GSL) – t.niese Mar 17 '19 at 20:08
  • So you are basically against answers like these: https://stackoverflow.com/a/7058373/4955498 ? Raw pointers are more dangerous than references since they can be accidentally converted to smart pointers – Michael Veksler Mar 17 '19 at 20:25
  • @MichaelVeksler As I said, `void foo(const Bar & test)` is perfectly fine and would be my first choice, but will only work if a const only usage is possible. `void foo(Bar & test)` has the problem, that if I look at `void foo(Bar & test)` then it is not self explaining that writing `foo(something)` would not change `something` so you need to assume that it might do, so someone accidentally could write `test = Bar()` in foo overwriting the `test`, and that error is less obvious then the concerting of a pointer to a smart pointer. – t.niese Mar 17 '19 at 20:50
  • @MichaelVeksler And [Google C++ Style Guide: Reference Arguments](https://google.github.io/styleguide/cppguide.html#Reference_Arguments) and also other people that do talks on the cppcon and are part of the committee will agree with that. And are convered in [C++ Core Guidelines: F.call: Parameter passing](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#fcall-parameter-passing). If you want/need to pass an owing raw pointer then you should use a GSL and `owner tex`. The answers date back to c++11, to a time when smart pointer where new. – t.niese Mar 17 '19 at 20:58
  • I never thought that Google coding guidelines are a good indication to anything. I've seen google amazing large-scale engineering from within, but coding guidelines is not their best piece. E.g. their bias against exceptions which forces them to use the anti-pattern of init functions: https://google.github.io/styleguide/cppguide.html#Exceptions – Michael Veksler Mar 17 '19 at 20:59
  • the C++ core guidelines actually gives the following as the "right way": `string& concatenate(string&, const string& suffix); `, which contradicts your point – Michael Veksler Mar 17 '19 at 21:03
  • @MichaelVeksler well exception handling has indeed its problems [CppCon 2018: Brand & Nash “What Could Possibly Go Wrong?: A Tale of Expectations and Exceptions”](https://www.youtube.com/watch?v=GC4cp4U2f2E), and a topic that is worked on. I don't think everything google write in the guidelines is valid/good, and you can argue about if raw pointer should only be used for optionals ([CppCon 2014: Herb Sutter "Back to the Basics! Essentials of Modern C++ Style"](https://www.youtube.com/watch?v=xnqTKD8uD64&feature=youtu.be&t=731)), but it is still not an anti-pattern. – t.niese Mar 17 '19 at 21:22
  • I've seen my share of accidental construction of smart-pointers out of raw pointers, so in my view using a raw pointer, when a reference would do, is an anti-pattern. It has a much higher probability of bad, and hard to debug, things to happen than accidental modification through a non-const reference. Double free is notoriously hard to debug. People used to ask me for helping them out in too many such cases. Nobody has ever asked me to help them out with a mutable reference, though – Michael Veksler Mar 17 '19 at 21:54