Bug 238493
Summary: | IPC::Connection::UniqueID is not possible to use in thread safe manner | ||
---|---|---|---|
Product: | WebKit | Reporter: | Kimmo Kinnunen <kkinnunen> |
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> |
Status: | NEW | ||
Severity: | Normal | CC: | cdumez, kkinnunen, simon.fraser, webkit-bug-importer |
Priority: | P2 | Keywords: | InRadar |
Version: | Safari 15 | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=226561 | ||
Bug Depends on: | |||
Bug Blocks: | 238515 |
Kimmo Kinnunen
IPC::Connection::UniqueID is not possible to use in thread safe manner
The whole idea should probably just be removed.
IPC::Connection::UniqueID is implemented for multi-threaded purposes, but it contains id to instance map maintenance in destructor which is not sound.
Also the comments around IPC::Connection::connection are just wrong. It says the function is not used from multiple threads, but the comments were added in a comment that explicitly added functionality to use it from multiple threads.
bug 226561 caused this
Attachments | ||
---|---|---|
Add attachment proposed patch, testcase, etc. |
Chris Dumez
The functions what use Connection::UniqueID use locking. Please provide actual evidence of a thread safety bug. I wrote this code and was specifically careful about thread safety.
Kimmo Kinnunen
IPC::Connection::send(UniqueID, ..) uses a lock to ensure that the instance is not deleted.
However, the instance could already be in its destructor when the lock is taken.
consider UniqueID==1, Connection instance = 0x1234
Thread A:
template<typename T>
bool Connection::send(UniqueID connectionID, T&& message, uint64_t destinationID, OptionSet<SendOption> sendOptions, std::optional<Thread::QOS> qos)
{
Locker locker { s_connectionMapLock };
auto* connection = connectionMap().get(connectionID);
if (!connection)
return false;
return connection->send(WTFMove(message), destinationID, sendOptions, qos); // <-- THREAD A here inside this for this=0x1234
}
Thread b:
Connection::~Connection()
{
// <--Thread B HERE for this=0x1234
ASSERT(RunLoop::isMain());
ASSERT(!isValid());
{
Locker locker { s_connectionMapLock };
connectionMap().remove(m_uniqueID);
}
clearAsyncReplyHandlers(*this);
}
Chris Dumez
(In reply to Kimmo Kinnunen from comment #2)
> IPC::Connection::send(UniqueID, ..) uses a lock to ensure that the instance
> is not deleted.
>
> However, the instance could already be in its destructor when the lock is
> taken.
>
> consider UniqueID==1, Connection instance = 0x1234
>
> Thread A:
>
> template<typename T>
> bool Connection::send(UniqueID connectionID, T&& message, uint64_t
> destinationID, OptionSet<SendOption> sendOptions, std::optional<Thread::QOS>
> qos)
> {
> Locker locker { s_connectionMapLock };
> auto* connection = connectionMap().get(connectionID);
> if (!connection)
> return false;
> return connection->send(WTFMove(message), destinationID, sendOptions,
> qos); // <-- THREAD A here inside this for this=0x1234
> }
>
>
> Thread b:
>
> Connection::~Connection()
> {
> // <--Thread B HERE for this=0x1234
>
> ASSERT(RunLoop::isMain());
> ASSERT(!isValid());
>
>
> {
> Locker locker { s_connectionMapLock };
> connectionMap().remove(m_uniqueID);
> }
>
> clearAsyncReplyHandlers(*this);
> }
Yes, it could be at the very beginning of its destructor. What is the (thread-safety) bug? At the point the destructor takes the locks in the destructor, nothing has been destroyed yet and it is still safe to call Connection::send(). One thing that would be unsafe would be to ref the connection but I made sure the static functions that are use uniqueID don't do that.
Kimmo Kinnunen
(In reply to Chris Dumez from comment #3)
> Yes, it could be at the very beginning of its destructor. What is the
> (thread-safety) bug? At the point the destructor takes the locks in the
> destructor, nothing has been destroyed yet
No, this is incorrect.
C++ object lifetime ends when the destructor is called.
The object lifetime has already ended when the lock is acquired.
There is no point in acquiring a member variable lock in a destructor, since no other thread can dereference the object that has been destroyed.
One thread cannot be in the destructor of an object while other thread dereferences the same object.
This is a scenario that is for example caught by ASAN.
I fixed numerous these ASAN problems and you reviewed those.
These were for example in bugs related to destructors removing the WorkQueueMessageReceiver, ThreadMessageReceiverRefCounted, WorkQueueMessageReceiver.
In those cases IPC::Connection would deliver messages from IPC delivery work queue to the instance. This would be all the same time as main thread would remove the message receiver. These were fixed by introducing the stopListeningForIPC() member function that the main thread calls before destroying the objects.
Kimmo Kinnunen
> One thing that would be unsafe would be to ref the connection but I made sure the static functions that are use uniqueID don't do that.
Sending of the message *ALSO* creates a ref to the connection, so this is wrong too.
bool Connection::sendMessage(UniqueRef<Encoder>&& encoder, OptionSet<SendOption> sendOptions, std::optional<Thread::QOS> qos)
{
...
auto sendOutgoingMessages = [protectedThis = Ref { *this }]() mutable {
protectedThis->sendOutgoingMessages();
};
...
}
Kimmo Kinnunen
For the lifetime,
https://en.cppreference.com/w/cpp/language/lifetime
The lifetime of an object ends when:
...
if it is of a class type, the destructor call starts, or
...
I discussed this in cpp channel in our slack year ago, and I think the conclusion was this.
I also tried to get somebody explicitly define where it says
* threads externally cannot call the members
* the destructor can call member functions normally and they can call other functions normally.
The latter part devolved a bit.
Anyway, the point being, based on my understanding on the above link presented as the justification: From C++ perspective it is undefined behavior to have one thread in destructor and one accessing a member function or variable. It might work or might not, but it is a use-after-free bug.
Radar WebKit Bug Importer
<rdar://problem/91282299>