2

I have a function that returns a shared pointer to an object (it is difficult to include the MyObject definition because of many large dependencies):

std::shared_ptr<MyObject> f(std::string params)
{
  return std::shared_ptr<MyObject>(new MyObject(params));
}

Does anyone know why this code works:

Case 1: no errors with valgrind -v --tool=memcheck

std::shared_ptr<MyObject> obj_ptr = f("hello");
MyObject obj = *obj_ptr;

While this code crashes:

Case 2: crashes and gives several errors with valgrind -v --tool=memcheck

MyObject obj = *f("hello");

The MyObject class has a working assignment operator and copy constructor (both verified in Case 1).

I have also tried creating a std::shared_ptr<MyObject> (via f), copying that to a pointer, copying the pointer to an object on the stack, and deleting the pointer. The final object on the stack is still fine:

Case 3: no errors with valgrind -v --tool=memcheck

std::shared_ptr<MyObject> obj_ptr = f("hello");
MyObject * obj_ptr2 = new MyObject(*obj_ptr);
MyObject obj3 = *obj_ptr2;
delete obj_ptr2;
obj3.print();

Is the error perhaps because std::shared_ptr is created as an rvalue, and frees its memory as soon as the * operator runs?

user
  • 7,123
  • 7
  • 48
  • 90
  • What version of what compiler are you using? – ildjarn May 09 '12 at 22:20
  • And what is the error when it crashes? A stack trace might help... – Rob I May 09 '12 at 22:21
  • 1
    Is MyObject copy constructor doing a deep copy? (Case 1 would still work fine with shallow copy). – bobah May 09 '12 at 22:24
  • @bobah Yes. If not, **Case 3** would fail. – user May 09 '12 at 22:24
  • 3
    Please just humor us with the definition of MyObject. – Benjamin Lindley May 09 '12 at 22:26
  • Examples 1 and 3 do nothing "odd". Example 2 dereferences a smart pointer that will be deleted right away(or has just been deleted). – mfontanini May 09 '12 at 22:29
  • @fontanini : It will be deleted at the end of the full expression, but that is not (or should not be) a problem here. – ildjarn May 09 '12 at 22:31
  • @fontanini Correct. Is this misuse of shared_ptr? Normally I would assume that my MyObject class has memory problems, but then wouldn't valgrind find some problem in **Case 1** or **Case 3**? – user May 09 '12 at 22:32
  • @Oliver: Only if you trust valgrind. I've never used it, so I don't. – Benjamin Lindley May 09 '12 at 22:33
  • @BenjaminLindley valgrind does complain reading invalid memory (during copy construction) in **Case 2** only. The code necessary to run is in a handful of files-- should I put a zip of it somewhere? – user May 09 '12 at 22:36
  • What happens when a non-shared pointer, such as unique_ptr, is used? – damienh May 09 '12 at 22:40
  • @damienh I just tried unique_ptr: same outcome for **Case 2**. – user May 09 '12 at 22:44
  • 2
    It's nearly certain that the bug is in your copy constructor. – David Schwartz May 09 '12 at 22:47
  • 1
    @Oliver: And what happens if you replace `MyObject` with a well tested class that manages memory, like `std::string`? – Benjamin Lindley May 09 '12 at 22:47
  • 1
    @Benjamin Lindley I too once did not trust valgrind's results saying I was leaking memory, and spent a few days debugging in other ways. I finally found the problem and it turned out valgrind was 100% spot on at the very beginning. I've been a believer ever since. – Mark B May 09 '12 at 22:51
  • @BenjaminLindley Just tried with `vector`, and it worked fine, so I guess the answer (below) is onto something... But I am shocked that valgrind would miss it. – user May 09 '12 at 22:54
  • @Oliver: If Mark B's answer is correct, then valgrind isn't missing anything. – Benjamin Lindley May 09 '12 at 22:54
  • @Oliver You said in the second case it *does* find a problem. `valgrind` isn't magic: If you don't execute the bad code it can't find the problem (and as I explain in my answer, the bad code almost certainly isn't being executed in those two cases). – Mark B May 09 '12 at 22:55

1 Answers1

8

The problem is (almost certainly) that you're shallow-copying one of the members of MyObject in its copy constructor. Then you either try to access the shallow data that's no longer valid, or you double delete it.

Consider the cases: In the first and third cases, the very first object from which all copies are made is still alive when you act on the stack object. In the second case the shared_ptr goes away after the copy construction, invoking the destructor of MyObject.

If you changed the third case thusly, I suspect it would crash:

MyObject * obj_ptr2 = new MyObject("hello");
MyObject obj3 = *obj_ptr2;
delete obj_ptr2;
obj3.print();
Mark B
  • 95,107
  • 10
  • 109
  • 188
  • +1 I think you're right-- I actually have callable destructors (`clear()`). I tried **Case 3** again, but destructed the original object after it was copied, and got an error. – user May 09 '12 at 22:59
  • It turned out I was doing a shallow copy somewhere. Thanks. – user May 10 '12 at 03:41