Boost logo

Boost :

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;
>{ m_pold = s_pnext; s_pnext = &m_buf; }
>{ 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,
>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
> 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
>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
>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
>40426 If the invocation appears in any other context, the behavior is
>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


Boost list run by bdawes at, gregod at, cpdaniel at, john at