Bug 238493

Summary: IPC::Connection::UniqueID is not possible to use in thread safe manner
Product: WebKit Reporter: Kimmo Kinnunen <kkinnunen>
Component: WebKit2Assignee: 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
Reported 2022-03-29 01:13:38 PDT
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
Chris Dumez
Comment 1 2022-03-29 13:24:30 PDT
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
Comment 2 2022-03-30 01:01:29 PDT
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
Comment 3 2022-03-30 07:08:11 PDT
(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
Comment 4 2022-04-04 05:28:59 PDT
(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
Comment 5 2022-04-04 05:33:26 PDT
> 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
Comment 6 2022-04-04 05:52:04 PDT
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
Comment 7 2022-04-05 01:14:15 PDT
Note You need to log in before you can comment on or make changes to this bug.