[boost] Re: boost.test thread safe bugs (and some fixes)

From: Gennadiy Rozental (gennadiy.rozental_at_[hidden])
Date: 2003-02-16 19:26:55

> I've been looking at your signal handling implementation in
>execution_monitor.cpp, and I think I've uncovered quite a few bugs, some of
>which are really quite fatal for multithreading code.

The code never promised to work in multithreaded environment, nor even to be
thread save. It is in my to-do list. Though recent hands in several
situations may require address some of these issues sooner.

>Issue 1:
>~~~~~
>You use a singleton for the signal handling jump:
>
>inline sigjmp_buf & execution_monitor_jump_buffer()
>{
>    static sigjmp_buf unit_test_jump_buffer_;
>    return unit_test_jump_buffer_;
>}
>
>There are two issues with this:
>
>a) It is not reentrant: if the monitored procedure called by catch_signals
>calls another monitored procedure then the inner catch_signals call will
>overwrite the jump data for the outer call.  IMO this situation is quite
>common - and actually occurs in your own unit test code I believe.

No. I don't think it's common situation. You don't usually create and run
test cases inside the other test case code. I also have special precautions
to make sure it does not happened with test suites. As for my own unit tests
I will recheck whether not this is the issue.

>b) It is not thread safe: each thread will trash the main threads jump
>data - when you signal handler gets called it will jump onto some other
>threads stack - or even the stack of a terminated thread!
>
>To fix this you will need to create the sigjmp_buf data on your program
>stack (in catch_signals), then store a pointer to it in a global variable,
>you will also need to back up that pointer on entering catch_signals and
>restore on leaving: smells like a scoped object to me:
>
>class sigjmp_data
>{
>sigjmp_buf m_buf;
>sigjmp_buf* m_pold;
>static sigjmp_buf* s_pnext;
>
>public:
>sigjmp_data()
>{ m_pold = s_pnext; s_pnext = &m_buf; }
>
>~sigjmp_data()
>{ s_pnext = m_pold; }
>
>static sigjmp_data& get()
>{ return *s_pnext; }
>
>};

Sound very reasonable.

>To make this thread safe you would need to store the pointer in a thread
>local storage slot, BTW I don't think you can use boost.threads for this,
as
>it will create a dependency quagmire for status/Jamfile :-(

I thought to introduce macro BOOST_MULTITHREADED_UNIT_TEST and guard all
usage of Boost.Thread with it. It does not create extra dependency and
should allow to build multithreaded version with bjam subvariant feature.

>Issue 2:
>~~~~~
>
>In catch_signals you have:
>
>    static struct sigaction all_signals_action;
>
>except this creates a race condition during this structs initialisation:
>getting rid of the static declaritor should do the trick.

I don't even remember why it's static in a first place.

>
>Issue 3:
>~~~~~
>
>Your invocation of sigsetjmp is strictly speaking invoking undefined
>behavior:
>
>    volatile int sigtype = sigsetjmp( execution_monitor_jump_buffer(), 1 );
>
> here's what POSIX issue 6 says:
>
>"An application shall ensure that an invocation of setjmp( ) appears in one
>of the following
>40418 contexts only:
>40419 • The entire controlling expression of a selection or iteration
>statement
>40420 • One operand of a relational or equality operator with the other
>operand an integral constant
>40421 expression, with the resulting expression being the entire
controlling
>expression of a
>40422 selection or iteration statement
>40423 • The operand of a unary ’!’ operator with the resulting expression
>being the entire
>40424 controlling expression of a selection or iteration
>40425 • The entire expression of an expression statement (possibly cast to
>void)
>40426 If the invocation appears in any other context, the behavior is
>undefined."
>
>Note that although this refers to setjmp, sigsetjmp makes reference to it.

I will need to investigate this in more details. The only thing that I could
comment right away is that it seems to work in majority of the cases.

>Issue 4:
>~~~~~
>
>Your calls to sigaction are not exception safe: if your monitored function
>throws an exception (not an unlikely event), then the original signal
>contexts are not restored, using more scoped objects should fix this.

Yep. That's right.

>Issue 5:
>~~~~~
>
>Your report error_code is not thread safe:
>
>static void report_error( execution_exception::error_code ec,
>c_string_literal msg1, c_string_literal msg2 )
>{
>    static char buf[REPORT_ERROR_BUFFER_SIZE];
>    buf[0] = '\0';
>    std::strncat( buf, msg1, sizeof(buf)-1 );
>    std::strncat( buf, msg2, sizeof(buf)-1-std::strlen(buf) );
>    throw execution_exception( ec, buf );
>}
>
>removing the static declarator from the buffer should fix that.

No. It would lead to error, cause execution_exception expect C string not
std::string and accordingly will point to dead memory.
What I need here is thread specific storage. It still won't be reentrant,
but it won't be the issue IMO.

>The difficulty we have now is that there is a release coming up, and
>boost.test is pretty mission critical for that, and these aren't really
>trivial issues to fix, I'm not sure how we should handle that - ideas?

I was aware of thread safety issues. And still I don't think it is so
critical, that we need to hurry to fix it for this release. My plan was to
address it after CLA. I still hope to be able to use Boost.Thread for this.
I will try to address 1(without tss) 2 and 4 today.

>Hope this helps,

Thank you John for your time and efforts.  We should have found this during
review.

Gennadiy.