Re: [boost] Boost.Xint is not thread-safe

Subject: Re: [boost] Boost.Xint is not thread-safe
From: Bryce Lelbach (blelbach_at_[hidden])
Date: 2011-06-19 19:02:16

On 2011.06.19_18.08.11, Chad Nelson wrote:
> On Sun, 19 Jun 2011 12:29:41 -0500
> Bryce Lelbach <blelbach_at_[hidden]> wrote:
>
> What global variables are those? The only non-local variables I see
> from a quick perusal of allocator_t are fixed_length and
> magnitude_datasize (allocator_t static consts) and minimum_digits (a
> global const).

List is below.
 
Problems that are immediately visible:

  * exception_handler uses an unprotected static data member to hold an exception
    handler; this variable is not once-initialized with boost::call_once.

  * get_nan() uses static variables and does not initialize them in a thread-safe
    manner.

  * nan_text() uses static variables and does not initialize them in a thread-safe
    manner.

  * magnitude_manager_t<>::zerodata() uses a static variable, which is not
    initialized in a thread safe manner.

  * TUTable (monty.hpp) uses a static std::auto_pt<> which is not initialized
    in a thread safe manner, and can lead to race-condition induced memory
    leaks (in addition to data corruption). You check if the auto_ptr is 0
    before reseting it, but there is no synchronization there, so two threads
    could read a zero, assume it's not been initialized, and allocate storage
    for it, leading to at least one excessive allocation, if not a leak. 

  * is_prime() uses a static std::vector<int> which is not initialized in a
    thread safe manner.

  * is_prime() uses an std::auto_ptr<> (to a random generator) which is not
    initialized in a thread safe manner, and may lead to memory leaks, among
    other issues.

  * Your reference counting implementation is not thread safe. The reference
    counting implementation consists of an std::size_t member in the magnitude_t
    class, which is incremented by an inc() function, and decrementing by a dec()
    function. No synchronization is performed before, during, or after
    increments or decrements.

  * magnitude_manager_t's copy constructor doesn't copy, it just increments the
    reference count of the rhs.

  * raw_integer_t's copy constructor doesn't copy, it does the same thing that
    magnitude_manager_t does. The raw_integer_t constructor for other
    instantiations of raw_integer_t does ensure unique copies.

  * copy semantics are performed lazily as needed by
    magnitude_manager_t::resize_and_uniquify/raw_integer_t::make_unique
    (and the backend, allocator_t::realloc_and_uniquify), because of the above
    three points, this leads to copies that share storage. 

  * integer_t's copy constructor only calls data.make_unique() if Threadsafe
    == false && force_thread_safety. This is not thread safe as it prevents
    resize_and_uniquify from being called, which leads to copies of the same
    integer_t sharing storage, as explained above. Copy assignment and move
    assignment operators are fine.

  * The above points mean that the "threadsafe" option isn't actually threadsafe.

Other notes:

  * You should use BOOST_STATIC_CONSTANT instead of defining static constants
    by hand.

  * Please use BOOST_ASSERT instead of assert().

> If I work on it any further (still undecided), that's already on the
> to-do list.

If you're not going to make Boost.Xint thread safe, please remove the notices
that claim that the library is thread safe.

I found a "boost.bigint" library in the Boost sandbox (under 2007 GSoC). Said
library is thread safe, albeit slightly slower. I would advise anyone else looking
for a Boost-style arbitrary precision integer library to check out bigint as well.

Chad, if you wish to use globals, please investigate a thread-safe singleton
approach. Robert Ramey has a nice one in Boost.Serialization; attached is the one
that we use at work.

Is this Xint library accepted?

-- 
Bryce Lelbach aka wash
boost-spirit.com
px.cct.lsu.edu
github.com/lll-project