Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UB in thread_data::operator delete #1763

Closed
AntonBikineev opened this issue Sep 25, 2015 · 11 comments

Comments

Projects
None yet
4 participants
@AntonBikineev
Copy link
Contributor

commented Sep 25, 2015

These lines seem to be wrong as operator delete gets called after thread_data destruction:

    void thread_data::operator delete(void *p, std::size_t size)
    {
        HPX_ASSERT(sizeof(thread_data) == size);

        if (0 != p)
        {
            thread_data* pt = static_cast<thread_data*>(p);
            thread_pool* pool = pt->pool_;
            HPX_ASSERT(pool);

            pool->deallocate(pt);
        }

https://github.com/STEllAR-GROUP/hpx/blob/master/src/runtime/threads/thread_data.cpp#L67-L75

Hope to have time and fix it soon unless somebody wants to beat me to.

@hkaiser

This comment has been minimized.

Copy link
Member

commented Sep 25, 2015

@AntonBikineev if you know how to fix this, please go ahead.

@sithhell

This comment has been minimized.

Copy link
Member

commented Sep 25, 2015

@AntonBikineev

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2015

@sithhell Might be as a solution but will tie the use of thread_data to intrusive_pointers only

@sithhell

This comment has been minimized.

Copy link
Member

commented Sep 25, 2015

Am 25.09.2015 4:52 nachm. schrieb "Anton Bikineev" <notifications@github.com

@sithhell Might be as a solution but will tie the use of thread_data with intrusive_pointers only

As far as I'm aware, that's the case right now, it's only used with
intrusive_ptr.

@AntonBikineev

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2015

@sithhell It doesn't seem to be architecturally correct to implicitly require type to be used only with intrusive pointers (counter isn't the case). thread_data::operator new and intrusive_ptr_release should be independent on each other.

@sithhell

This comment has been minimized.

Copy link
Member

commented Sep 25, 2015

Am 25.09.2015 5:13 nachm. schrieb "Anton Bikineev" <notifications@github.com

@sithhell It doesn't seem to be architecturally correct to implicitly require type to be used only with intrusive pointers (counter isn't the case)

I kind of agree. In that case, I guess, the design flaw is in the
allocation process itself already, I'm thinking that if you factor out the
deleter, and require that to be called instead of plain delete might solve
the problem.

@sithhell

This comment has been minimized.

Copy link
Member

commented Sep 25, 2015

On 09/25/2015 05:13 PM, Anton Bikineev wrote:

@sithhell https://github.com/sithhell It doesn't seem to be
architecturally correct to implicitly require type to be used only with
intrusive pointers (counter isn't the case)

I guess the only non UB way to go is something along those lines:
http://melpon.org/wandbox/permlink/MF1CPJfAMj3BCrTe


Reply to this email directly or view it on GitHub
#1763 (comment).

@hkaiser

This comment has been minimized.

Copy link
Member

commented Sep 25, 2015

I guess the only non UB way to go is something along those lines:
http://melpon.org/wandbox/permlink/MF1CPJfAMj3BCrTe

I agree, but only if you extract the reference to the pool before calling the destructor.

@K-ballo

This comment has been minimized.

Copy link
Member

commented Sep 26, 2015

Could we please try to figure out how the code ought to behave before attempting to fix it? The whole construction looks suspicious.

Consider bool is_created_from(void* pool) const, that seems to suggest that the pool is part of the thread_data model, and a fundamental part of it as one cannot construct a thread_data without a pool. In this scenario, it seems a thread_data must be created from a pool by design, yet the design does not enforce this and turn optional hints to an allocation scheme useless. When correctly used, this would lead to situations like new (p) A(p). This should instead be written as p.new_<A>() or similar, and the custom new/delete operators should be completely removed.

On the other hand, it's possible that is_created_from crept in due to a bogus implementation of custom new/delete operators. The way these custom operators handle optional hints is by allocating extra memory to put stuff into it. This data belongs to the raw bits only, it cannot be given to the concrete type being constructed, that's orthogonal. They have to do this in a way that appears redundant, replacing 4, 5, or 6 operators due to language reasons. In that case, is_created_from and all mention of pool should be removed from thread_data, and the operators should be implemented correctly.

So which is it? Is a pool a fundamental piece of the design of a thread_data, or is it an optional hint to a custom allocation scheme?

@hkaiser

This comment has been minimized.

Copy link
Member

commented Sep 26, 2015

is_created_from is used to ensure that the thread_data is deleted by the same pool as it was allocated from. And yes, thread_data is by design supposed to be created from a pool only.

Currently, the thread_data instances are created like t = new (p) A(p,...) (which is correct but could be replaced as suggested by @K-ballo, see here) and are deleted (through an intrusive_ptr) by delete t (see here).

@hkaiser

This comment has been minimized.

Copy link
Member

commented Oct 1, 2015

This has been fixed by merging #1775: Restricting thread_data to creating only with intrusive_pointers

@hkaiser hkaiser closed this Oct 1, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.