Confirm thread safety with std :: unique_ptr / std :: shared_ptr

Question

My application has an IRC module, which is essentially a regular client. Since this is very upside down, I risk getting a plug-in, for example, a user alias - it is valid at that time, but the parser starts the update by changing the specified alias. After another thread executes again, it deals with a pointer to now invalid memory, since it will be impossible to return return + as an atomic operation.

I assume this is correct, based on the code below? As such, I assume that I will have to use the usual mutex lock / unlock method if someone cannot confirm or suggest another (I would prefer not to convert and return shared_ptr, but I think this is a valid option, it's just me I intend on SWIG'ing this and don’t know if they will like it).

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

Answer 1

There is a race condition in the code and, therefore, the behavior is undefined, since there is a potential read ( ->get() ) and a write ( .reset() or = ) on the same object (a std::shared_ptr<std::string> instance) from separate threads: access to std::shared_ptr should be synchronized.

Pay attention to the std::mutex lock in getter and returning c_str() not enough, since the calling getter will use the result of c_str() outside the lock: the recipient needs to return the value shared_ptr by value.

To fix it:

• add std::mutex to IrcUser (note that now the class is not copied):

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

• block std::mutex in getters and Update() using 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 only using std::string if copying is acceptable, since shared_ptr can add unnecessary complexity.

Answer 2

I would suggest that blocking at such fine-grained levels is likely (way) to overkill.

I would suggest making atomic updates to the IrcUser object itself, which might be blocked depending on your library implementation and target architecture. Here is an example 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 do not know how many implementations of compiler libraries / C ++ already implements this C ++ 11 function.

Here's what it 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); } } 

Answer 3

Yes, a race condition can occur with an alias, which causes your recipients to access bad memory. shared_ptrs are only thread safe with respect to their ownership symposia. You will need to add some form of synchronization for your values.