Confirmation of thread safety with std::unique_ptr/std::shared_ptr

3
votes

My application has an IRC module that essentially is a normal client. Since this is heavily threaded, I stand the risk of a plug-in retrieving, for example, a users nickname - it is valid at the time, but the parser triggers an update, changing said nickname. Once the other thread has execution again, it's dealing with a pointer to now-invalid memory, since it'll be impossible to have the return + copy as an atomic operation.

Is my assumption about this correct, based on the code below? As such, I guess I'd have to use the usual mutex lock/unlock method, unless someone can confirm or suggest otherwise (I'd rather not have to convert and return a shared_ptr, but I guess it is a valid option, it's just I intend on SWIG'ing this and don't know if it wouldn't like them).

IrcUser.h

class IrcUser : public IrcSubject
{
private:
    ...
    std::shared_ptr<std::string>    _nickname;
    std::shared_ptr<std::string>    _ident;
    std::shared_ptr<std::string>    _hostmask;
public:
    ...
    const c8*
    Ident() const
    { return _ident.get()->c_str(); }

    const c8*
    Hostmask() const
    { return _hostmask.get()->c_str(); }

    const u16
    Modes() const
    { return _modes; }

    const c8*
    Nickname() const
    { return _nickname.get()->c_str(); }

    bool
    Update(
        const c8 *new_nickname,
        const c8 *new_ident,
        const c8 *new_hostmask,
        const mode_update *new_modes
    );
};

IrcUser.cc

bool
IrcUser::Update(
    const c8 *new_nickname,
    const c8 *new_ident,
    const c8 *new_hostmask,
    const mode_update *new_modes
)
{
    if ( new_nickname != nullptr )
    {
        if ( _nickname == nullptr )
        {
            *_nickname = std::string(new_nickname);
        }
        else
        {
            _nickname.reset();
            *_nickname = std::string(new_nickname);
        }

        Notify(SN_NicknameChange, new_nickname);
    }

    ...
}
3
c++11thread-safetyshared-ptrunique-ptr

3 Answers

8
votes

I'd suggest that locking on such finegrained levels is likely (way) overkill.

I'd suggest doing atomic updates to the IrcUser object itself, which could be lock-free depending on your library implementation and target architecture. Here is a sample that uses

  • std::atomic_is_lock_free<std::shared_ptr>
  • std::atomic_load<std::shared_ptr>
  • std::atomic_store<std::shared_ptr>

See http://en.cppreference.com/w/cpp/memory/shared_ptr/atomic for documentation.

Disclaimer I don't know how many compilers/C++ library implementations already implement this C++11 feature.

Here's what it'd would look like:

#include <atomic>
#include <memory>
#include <string>

struct IrcSubject {};
typedef char c8;
typedef uint16_t u16;
typedef u16 mode_update;

class IrcUser : public IrcSubject
{
    private:
        // ...
        std::string _nickname;
        std::string _ident;
        std::string _hostmask;
        u16         _modes;
    public:
        IrcUser(std::string nickname, std::string ident, std::string hostmask, u16 modes)
            : _nickname(nickname), _ident(ident), _hostmask(hostmask), _modes(modes) { }
        // ...
        std::string const& Ident()    const { return _ident; }
        std::string const& Hostmask() const { return _hostmask; }
        const u16          Modes()    const { return _modes; }
        std::string const& Nickname() const { return _nickname; }
};

//IrcUser.cc
bool Update(std::shared_ptr<IrcUser>& user,
    std::string new_nickname,
    std::string new_ident,
    std::string new_hostmask,
    const mode_update *new_modes
)
{
    auto new_usr = std::make_shared<IrcUser>(std::move(new_nickname), std::move(new_ident), std::move(new_hostmask), *new_modes /* ??? */);
    std::atomic_store(&user, new_usr);
    //Notify(SN_NicknameChange, new_nickname);
    return true;
}

bool Foo(IrcUser const& user)
{
    // no need for locking, user is thread safe
}

int main()
{
    auto user = std::make_shared<IrcUser>("nick", "ident", "hostmask", 0x1e);

    mode_update no_clue = 0x04;
    Update(user, "Nick", "Ident", "Hostmask", &no_clue);

    {
        auto keepref = std::atomic_load(&user);
        Foo(*keepref);
    }
}
4
votes

The code has a race condition, and therefore undefined behaviour, as there is a potential read (the ->get()) and write (the .reset() or =) on the same object (a std::shared_ptr<std::string> instance) from separate threads: access to the std::shared_ptrs must be synchronized.

Note locking a std::mutex within the getter and returning the c_str() is insufficient as the caller of the getter would be using the result of c_str() outside of the lock: the getter needs to return a shared_ptr by value.

To correct:

  • add a std::mutex to IrcUser (note this now makes the class non-copyable):

    mutable std::mutex mtx_; // Must be mutable for use within 'const'

  • lock the std::mutex in the getters and Update(), using a std::lock_guard for exception safety:

    std::shared_ptr<std::string> Nickname() const
{
    std::lock_guard<std::mutex> l(mtx_);
    return _nickname;
}

bool IrcUser::Update(const c8 *new_nickname,
                     const c8 *new_ident,
                     const c8 *new_hostmask,
                     const mode_update *new_modes)
{
    if (new_nickname)
    {
        {
            std::lock_guard<std::mutex> l(mtx_);
            _nickname.reset(new std::string(new_nickname));
        }
        // No reason to hold the lock here.
        Notify(SN_NicknameChange, new_nickname);
    }

    return true;
}

Consider just using a std::string if the copying is acceptable as the shared_ptr may be adding unnecessary complexity.

1
votes

Yes, a race condition can occur with nickname that results in your getters accessing bad memory. shared_ptrs are only thread safe with regards to their ownership symantics. You will need to add some form of synchronization for their values.