Boost C++ Libraries: Ticket #8730: Race condition in once_block.hpp

Andrey Semashev — Tue, 25 Jun 2013 16:01:08 GMT

Which implementation are you referring to?

First of all, once_block_flag is not thread safe.

What makes you think so? It is POD, and all its modifications in the library are made in a thread-safe way (given that you use the multi-threaded build of the library).

keyword static will do bad things in C++03

I'm well aware of that, and that's the reason why once_block_flag is POD.

Antony Polukhin — Tue, 25 Jun 2013 19:03:30 GMT

Replying to andysem:

Which implementation are you referring to?

First of all, once_block_flag is not thread safe.

What makes you think so? It is POD, and all its modifications in the library are made in a thread-safe way (given that you use the multi-threaded build of the library).

POD *does not* guarantee atomicity. Take a look at Boost.Atomic library and note that even bools and integers are wrapped around in classes and specific processor instructions are used to gurantee atomicity. Those instructions are *not* generated by compiler when compiler sees POD type, because they work much slower than usual ones.

keyword static will do bad things in C++03

I'm well aware of that, and that's the reason why once_block_flag is POD.

Even if you rewrite it to Boost.Atomic it is not safe to use it that way. Back to the example:

if (!is_foo_inited) {
    new (place_for_foo) foo_type(some parameters);
    is_foo_inited = true;
}

Thread no1 checks for !is_foo_inited, enters the if block and starts to execute the placement new. At that time thread no2 checks for !is_foo_inited and enters the if block... That is not good, leads to memory leaks and undefined behavior!

Andrey Semashev — Tue, 25 Jun 2013 20:28:35 GMT

Replying to apolukhin:

POD *does not* guarantee atomicity. Take a look at Boost.Atomic library and note that even bools and integers are wrapped around in classes and specific processor instructions are used to gurantee atomicity. Those instructions are *not* generated by compiler when compiler sees POD type, because they work much slower than usual ones.

I repeat, all modifications to the flag are done in a thread-safe manner. There is no need for explicit atomic ops in this case, because only the fast path check is done unprotected. The secondary check and all modifications are done under the mutex lock, which ensures all the necessary memory barriers.

Do you have a particular use case that may show breakage with the current implementation of once_block? On what hardware? With references to the code, if possible.

Even if you rewrite it to Boost.Atomic it is not safe to use it that way. Back to the example:
if (!is_foo_inited) {
    new (place_for_foo) foo_type(some parameters);
    is_foo_inited = true;
}
Thread no1 checks for !is_foo_inited, enters the if block and starts to execute the placement new. At that time thread no2 checks for !is_foo_inited and enters the if block... That is not good, leads to memory leaks and undefined behavior!

That is not the code that is present in once_block.hpp/cpp, nor it is intended to be that way. I'm not using Boost.Atomic in that code for very specific reasons. Please, do not digress to problems in hypothetical code in this report.

Antony Polukhin — Tue, 25 Jun 2013 21:07:53 GMT

Replying to andysem:

Do you have a particular use case that may show breakage with the current implementation of once_block? On what hardware? With references to the code, if possible.

Let's take a look at boost/log/utility/once_block.hpp. Macro BOOST_LOG_ONCE_BLOCK() is defined as

    BOOST_LOG_ONCE_BLOCK_INTERNAL(\
        BOOST_LOG_UNIQUE_IDENTIFIER_NAME(_boost_log_once_block_flag_),\
        BOOST_LOG_UNIQUE_IDENTIFIER_NAME(_boost_log_once_block_sentry_))

That block contains BOOST_LOG_ONCE_BLOCK_INTERNAL will unwrap to something like

    static boost::log::once_block_flag flag_var = { boost::log::once_block_flag::uninitialized };

where boost::log::once_block_flag is a POD type. Note the static keyword usage. It means that it will be converted by compiler to something like this:

if (!is_once_block_flag_inited) { // Variable constructed with compiler
    new (flag_var) boost::log::once_block_flag(boost::log::once_block_flag::uninitialized);
    is_once_block_flag_inited = true;
}

Now consider the situation: Thread no1 checks for !is_foo_inited, enters the if block and starts to execute the placement new. At that time thread no2 checks for !is_foo_inited and enters the if block... That is not good, leads to memory leaks and undefined behavior!

That's only top of the iceberg. There are some more problems in once_block_sentry::executed(). What for is all the code inside once_block.cpp: you are using boost::thread but do not use boost::call_once? What is the reason?

Andrey Semashev — Tue, 25 Jun 2013 21:24:00 GMT

Replying to apolukhin:

    static boost::log::once_block_flag flag_var = { boost::log::once_block_flag::uninitialized };
where boost::log::once_block_flag is a POD type. Note the static keyword usage. It means that it will be converted by compiler to something like this:
if (!is_once_block_flag_inited) { // Variable constructed with compiler
    new (flag_var) boost::log::once_block_flag(boost::log::once_block_flag::uninitialized);
    is_once_block_flag_inited = true;
}

No, it won't. Since it's POD, its initialization is performed during the static initialization stage, which happens before any dynamic initialization. See 3.6.2 [basic.start.init] in the standard.

That's only top of the iceberg. There are some more problems in once_block_sentry::executed().

What problems in particular?

What for is all the code inside once_block.cpp: you are using boost::thread but do not use boost::call_once? What is the reason?

Once blocks offer more convenient syntax and are more lightweight than boost::call_once. BTW, boost::call_once also uses static initialization of its once_flag to achieve thread safety.

Antony Polukhin — Tue, 25 Jun 2013 21:50:11 GMT

Replying to andysem:

Replying to apolukhin:
    static boost::log::once_block_flag flag_var = { boost::log::once_block_flag::uninitialized };
where boost::log::once_block_flag is a POD type. Note the static keyword usage. It means that it will be converted by compiler to something like this:
if (!is_once_block_flag_inited) { // Variable constructed with compiler
    new (flag_var) boost::log::once_block_flag(boost::log::once_block_flag::uninitialized);
    is_once_block_flag_inited = true;
}
No, it won't. Since it's POD, its initialization is performed during the static initialization stage, which happens before any dynamic initialization. See 3.6.2 [basic.start.init] in the standard.

This is not correct for *local objects with static storage duration*, see 6.7 of C++03: A local object of POD type (3.9) with static storage duration initialized with constant-expressions is initialized before its block is first entered. An implementation is permitted to per-form early initialization of other local objects with static storage duration under the same conditions that an implementation is permitted to statically initialize an object with static storage duration in namespace scope (3.6.2). Otherwise such an object is initialized the first time control passes through its declaration; such an object is considered initialized upon the completion of its initialization.

So you can not have 100% guarantee for that.

That's only top of the iceberg. There are some more problems in once_block_sentry::executed().

What problems in particular?

Did not noticed the enter_once_block() in it, so sorry - it is OK.

What for is all the code inside once_block.cpp: you are using boost::thread but do not use boost::call_once? What is the reason?

Once blocks offer more convenient syntax and are more lightweight than boost::call_once. BTW, boost::call_once also uses static initialization of its once_flag to achieve thread safety.

Please show me the line where it uses local object with static storage duration.

Andrey Semashev — Wed, 26 Jun 2013 07:08:58 GMT

Replying to apolukhin:

Replying to andysem:

No, it won't. Since it's POD, its initialization is performed during the static initialization stage, which happens before any dynamic initialization. See 3.6.2 [basic.start.init] in the standard.

This is not correct for *local objects with static storage duration*, see 6.7 of C++03:

So you can not have 100% guarantee for that.

Ok, you have a point. But note that once_block_flag::uninitialized is 0 and zero initialization for local static objects is still performed before any other initialization. Most compilers will simply ignore the initializer for this reason. But I guess, it can be just removed from the macro definition to avoid the confusion and guarantee the correct behavior.

Once blocks offer more convenient syntax and are more lightweight than boost::call_once. BTW, boost::call_once also uses static initialization of its once_flag to achieve thread safety.

Please show me the line where it uses local object with static storage duration.

I was referring to the once_flag usage in the user's code (as a function-local static). But I guess since it is not guaranteed to be initialized statically (e.g. if BOOST_ONCE_INIT is not equivalent to zero initialization or the compiler is not smart enough to initialize the flag statically), it cannot be used that way.

To be honest, I have yet to see the compiler that does not perform static initialization for local static PODs, even with non-zero values.

Antony Polukhin — Wed, 26 Jun 2013 08:44:04 GMT

Replying to andysem:

Replying to apolukhin:

Replying to andysem:

No, it won't. Since it's POD, its initialization is performed during the static initialization stage, which happens before any dynamic initialization. See 3.6.2 [basic.start.init] in the standard.

This is not correct for *local objects with static storage duration*, see 6.7 of C++03:

So you can not have 100% guarantee for that.

Ok, you have a point. But note that once_block_flag::uninitialized is 0 and zero initialization for local static objects is still performed before any other initialization. Most compilers will simply ignore the initializer for this reason. But I guess, it can be just removed from the macro definition to avoid the confusion and guarantee the correct behavior.

OK, you've convinced me.

May I ask a few more questions. Why there is so many things 'reinvented' in Boost.Log? Foe example I can see light_rw_mutex.cpp, thread_safe_queue.cpp, thread_id.cpp, thread_specific.cpp and others... They are all already implemented in Boost and tested for a long time. Why don't you use them?

Andrey Semashev — Wed, 26 Jun 2013 09:26:18 GMT

Replying to apolukhin:

May I ask a few more questions. Why there is so many things 'reinvented' in Boost.Log? Foe example I can see light_rw_mutex.cpp, thread_safe_queue.cpp, thread_id.cpp, thread_specific.cpp and others... They are all already implemented in Boost and tested for a long time. Why don't you use them?

Because for various reasons these implementations do not fit the pressing requirements of the library.

light_rw_mutex is more lightweight in terms of compile times and probably run times (I haven't benchmarked it thoroughly) than shared_mutex.

Unlike thread::id, thread id attribute provides ids equivalent to those you would typically see in debugger or system monitoring tools (i.e. some notion of a system thread ids), which simplifies greatly debugging.

TLS, as it is implemented in Boost.Log, guarantees maximum performance the underlying system provides (unlike Boost.Thread implementation which provided O(N) complexity of every access to the thread specific value at some point; I think it's O(log(N)) now, but it is still worse than the typical O(1) of the system TLS).

There is no direct counterpart for thread_safe_queue, aside from the recently added Boost.Lockfree. Even then, Boost.Lockfree containers have restrictions on the element types that are not compatible with Boost.Log. It is possible to change the implementation to employ Boost.Lockfree, but the resulting code will be less efficient and also it adds an unnecessary dependency, so I don't think this is reasonable.

I did propose some of these custom implementations to the respective libraries but there was little interest. I may try again in the future though.

Antony Polukhin — Wed, 26 Jun 2013 09:44:44 GMT

Replying to andysem:

Replying to apolukhin:

May I ask a few more questions. Why there is so many things 'reinvented' in Boost.Log? Foe example I can see light_rw_mutex.cpp, thread_safe_queue.cpp, thread_id.cpp, thread_specific.cpp and others... They are all already implemented in Boost and tested for a long time. Why don't you use them?

Because for various reasons these implementations do not fit the pressing requirements of the library.

light_rw_mutex is more lightweight in terms of compile times and probably run times (I haven't benchmarked it thoroughly) than shared_mutex.

Unlike thread::id, thread id attribute provides ids equivalent to those you would typically see in debugger or system monitoring tools (i.e. some notion of a system thread ids), which simplifies greatly debugging.

TLS, as it is implemented in Boost.Log, guarantees maximum performance the underlying system provides (unlike Boost.Thread implementation which provided O(N) complexity of every access to the thread specific value at some point; I think it's O(log(N)) now, but it is still worse than the typical O(1) of the system TLS).

I did propose some of these custom implementations to the respective libraries but there was little interest. I may try again in the future though.

All the changes sound good. Why don't you cooperate with Vicente Botet (current maintainer of Boost.Thread) and propose your changes to him? I don not really wish to see two different thread libraries in Boost - they make resulting users code bigger and require more effort for maintaining.

Andrey Semashev — Sun, 30 Jun 2013 13:29:26 GMT

(In [84918]) Refs #8730. The once_flag doesn't have initializer now when defined by the BOOST_LOG_ONCE_BLOCK macro. This is done in order to guarantee it is initialized at static initialization stage even on (theoretical) compilers that would not do so if the initializer is present. The BOOST_LOG_ONCE_BLOCK_FLAG macro has been fixed so that it actually uses the flag variable specified in its argument. The flag itself is now a single byte internally in order to minimize the chance of reading an inaccurate value at the fast path. The test robustness improved.

status changed; resolution set

Andrey Semashev — Sat, 20 Jul 2013 18:09:34 GMT

status new → closed
resolution → fixed

(In [85093]) Merged recent changes from trunk. Fixes #8730.