CopernicaMarketingSoftware/AMQP-CPP

Race condition leading to tampering with a socket we no longer own in libboostasio.h

Opened this issue · 0 comments

devgs commented

Describe the bug
Due to an async nature of libboostasio.h we may get into a situation when Watcher outlives the call to monitor() that uninstalls the descriptor watch (flags == 0). Leading to a use of a socket that was closed by caller of monitor (TcpExtState::cleanup) and, immediately after, created by another entity (in POSIX, descriptor numbers are reused).

Specifically, problematic here is a use of _socket.release() that unsubscribes the socket's descriptor from a reactor (epoll/kqueue/...) when the file descriptor is in use by another asio socket, basically making it inoperable.

Expected behavior and actual behavior
Don't tamper with file descriptors that you no longer own.

Sample code

It's really hard to provide a sample. It's always hard to reproduce a race. And, as usual, it only happens in production :).
And obviously, to reproduce it you have to have at least two threads. In our case it's the other thread that manages to create a socket with a descriptor that was just close()-d by amqpcpp's thread, but before it was release() in a Wrapper's destructor.

It's easier to suggest a fix: either use a POSIX dup() or better: release the socket immediately, not relying for the Wrapper's destructor to be invoked:

            // the watcher does already exist, but we no longer have to watch this watcher
            iter->second.release(); // <------- HERE
            _watchers.erase(iter);

Obviously, you also have to add a release() function to Watcher.