Why boost::thread could be deleted double times?

Question

I noticed something strange. I expect a segfault to be produced running the following code, but it isn't.

void DeadlineTimeOut(const boost::system::error_code& pErrorCode, boost::thread* pThread) 
{
    std::cout << "Error code: #" << pErrorCode.value() 
      << " Message: " << pErrorCode.message() << std::endl;
    std::cout << "Thread Address = " 
      << pThread << std::endl; // "sth. like 0x33aabc0"

    pThread->interrupt();
    pThread->join();

    delete pThread;
    delete pThread;

    std::cout << "Stopped execution thread #" 
      << pThread->get_id() << std::endl; // "{Not-any-thread}"
}

So, why is the double delete possible? And also calling a member? I'm a little confused at the moment.

Made some changes for better reading, sorry i missed thad out at first — Benjamin, Sep 14 '11 at 13:16
The downvotes probably stem from the fact that the question is obscured by using `boost::thread`. It would be the same with an `int*` and we could simply close it as a dup. — pmr, Sep 14 '11 at 13:18
@pmr: That makes sense. As usual, it would be nice if people didn't ignore that little orange window when they downvote something. — Josh Darnell, Sep 14 '11 at 13:27
@jadarnel27 what would be even nicer if questions like this wouldn't be voted up so I didn't have to waste reputation to vote them down again. — pmr, Sep 14 '11 at 13:29
@pmr: Perhaps if there was an explanation of the downvotes I saw, I wouldn't have voted it up, causing you to waste your time and rep =) — Josh Darnell, Sep 14 '11 at 13:40

score 13 · Accepted Answer · edited Feb 23 '15 at 08:07

13

Deleting a pointer twice is undefined behaviour. There's no guarantee of a segfault. You might get one if you're lucky; you might not. The code might pass all your testing and then blow up in your customer's face at the worst possible moment. See the C++ FAQ.

The same goes for dereferencing a pointer that's been deleted (the pThread->get_id() in your code).

A simple defensive technique is to set pointers to NULL as soon as they've been deleted, instead of letting them dangle. This may help catch some bugs of this type.

The above applies to pointers of any type, and not just boost::thread*.

edited Feb 23 '15 at 08:07

seaotternerd

6,298
2
47
58

answered Sep 14 '11 at 13:13

NPE

486,780
108
951
1,012

1

From the standard chapter 3.7.4.2 / 4: _If the argument given to a deallocation function in the standard library is a pointer that is not the null pointer value (4.10), the deallocation function shall deallocate the storage referenced by the pointer, rendering invalid all pointers referring to any part of the deallocated storage. The effect of using an invalid pointer value (including passing it to a deallocation function) is undefined._ – Stephan Sep 14 '11 at 13:19
10

Can't say that I agree with setting pointers to `NULL`. This really doesn't defend against anything; most of the time, a pointer which is deleted will be going out of scope immediately anyway, and setting this pointer to `NULL` doesn't do anything about other pointers which point to the same object (and which have also become invalid). – James Kanze Sep 14 '11 at 13:30
1

Beware! You also [might get pregnant](http://stackoverflow.com/questions/1553382/pod-freeing-memory-is-delete-equal-to-delete/1553407#1553407)! – sbi Sep 14 '11 at 13:57
@sbi ?? I would have thought that setting your pointer to null would prevent pregnancy! – Martin James Sep 14 '11 at 14:25
4

@Martin: I fully agree with James about this issue. Setting pointers to `NULL` just masks errors. The best way to deal with a pointer you don't need anymore is to let it fall out of its scope. In that, setting your pointer to `NULL` resembles praying as a contraceptive: it sounds nice, but doesn't help. – sbi Sep 14 '11 at 16:15
@sbi: And what if you can't? What if you still have another 25-30 lines to go in the function? Should the user enclose the pointer in a scope, making another indentation just so that it falls out of scope after deletion? Or are you saying that you should only delete things at the bottom of the scope of a function? Setting a pointer to NULL is, at worst harmless, and at best a good way to trap errors as early as possible. Especially if those pointers are members of an object. – Nicol Bolas Sep 14 '11 at 17:49
3

@Nic: Still *another* 25-30 lines? It seems your functions are way too long. – fredoverflow Sep 14 '11 at 19:12
4

@Nicol: I rarely ever use naked pointers, and I haven't used them for managing resources in at least a decade, except as members of a class that has the sole purpose of managing that resource. There, your `delete ptr_;` is in the dtor, right before the closing `}`. – sbi Sep 14 '11 at 19:13
@sbi: While it's nice and all to be living in the ivory tower, some of us have to do work in the real world where smart pointers aren't necessarily everywhere and Boost isn't allowed. – Nicol Bolas Sep 14 '11 at 19:18
2

@Nicol: Should I feel offended? I started to write C++ for money in 1994. I have worked on quite a few C++ projects, from being the sole developer up to collaborating on multi-million LoC projects, sitting on a decade old codebase. There's a pretty good chance you have some of my code installed (it's part of a few popular applications with several million installations) or you are using it through some server on the web (some of my code runs at google and at other web servers). ___Wherever I was, I saw a strong correlation between using naked pointers and seeing "inexplicable" crashes.___ – sbi Sep 14 '11 at 19:41
2

@Nicol: Some of us *do* real world in the real world. But not all of us try to make our jobs *harder* by sabotaging our own code. If you do, that's great for you, and I'm sure you can come up with a reason why it's a constructive use of your time. But I prefer to avoid double-free errors and memory leaks by using a few very simple techniques that have been known for a decade or two. – jalf Sep 14 '11 at 19:46
1

You don't need Boost in order to use RAII in your code. You don't even need smart pointers. Smart pointers are just a couple of convenience classes to cover certain commonly used usage patterns. I don't use a lot of smart pointers, because most of my classes are designed to clean up after themselves in the first place. – jalf Sep 14 '11 at 19:49
@jalf: **I** use RAII in my code. But not everyone is _allowed_ to do so. So non-answers like "well, just do this instead" don't actually help those people. – Nicol Bolas Sep 14 '11 at 20:09
1

@Nicol: Developers being "not allowed to use RAII" - what shop would ever do that to themselves? Who would be so stupid to _know_ RAII and then _forbid it_? That just doesn't make sense! And where have you worked, that you consider such insanity _"real world"?_ I'm sorry to say, but this all sounds just very unlikely. – sbi Sep 14 '11 at 20:16
1

@Nicol: but what about places where you are "not allowed" to set pointers to null after you delete them? I'm sorry, but how is "use RAII" more of a non-answer than "set your pointers to NULL"? – jalf Sep 14 '11 at 20:19
@sbi: You've obviously never worked in the videogame industry. Depending on the project, codebase, and managers, even `new` and `delete` may be forbidden, let alone using proper RAII usage. "C with Classes" is far too often the order of the day in those circles. – Nicol Bolas Sep 14 '11 at 20:23
1

@Nicol: I don't really see the relevance. Of course there are companies that are stuck in the 80's, and parts of the videogame industry are renowned for that. But that doesn't change the fundamental fact that *RAII safeguards against double-frees. Setting pointers to NULL does not*. I don't see how it is relevant that some poor sods are being forced to write broken code. – jalf Sep 14 '11 at 20:28
@Nicol: Banning `new` and `delete` I can understand, they do this in embedded all the time. I could even understand that `std::shared_ptr`, with its locked counter, might be disallowed in an MT environment with hard performance requirements. BTDT. But why ban some _inlined_ compiler magic taking care of what you would have to do manually?! BTW, I have been to shops where C with Classes was the norm. Heck, when I started out in 1994, my boss was suspicious about the classes in this! I fought and taught, and changed their attitude, or I ran for the hills looking for a new job, if it didn't work. – sbi Sep 14 '11 at 20:40

Why boost::thread could be deleted double times?

1 Answers1