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

From: John Maddock (john_maddock_at_[hidden])
Date: 2003-02-16 07:15:18

Gennadiy,

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.

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.
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; }

};

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 :-(

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.

Issue 3:
~~~~~

Your invocation of sigsetjmp is strictly speaking invoking undefined
behaviour:

    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.

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.

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 declaritor from the buffer should fix that.

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?

Hope this helps,

John Maddock
http://ourworld.compuserve.com/homepages/john_maddock/index.htm