Skip to content

Commit

Permalink
WIP: SystemLayerImplSelect: guard against unbalanced use of Request/C…
Browse files Browse the repository at this point in the history 
…learCallback

As pointed out in project-chip#21135, the previous version was not safe when calls to RequestCallbackXX and ClearCallbackXX were not balanced, and there's no reason to believe API clients would balance them.

This version does no longer try to suspend the source (because that could be problematic when done from within callbacks) but now follows the original semantics of just changing mPendingIO flags to enable/disable callback, with the exception of requesting the callback for the first time (which triggers creation of the dispatch source).

WIP: more detailed testing shows:

- TCPtest from src/transport/raw/tests never bothers to set up a dispatch queue in the first place, so fails.

- when NetworkTestHelpers.cpp is modified to setup a dispatch queue, TCPtest succeeds, crashes or fails randomly

- TestSystemWakeEvent from src/system/tests succeeds despite not setting up a dispatch queue. Apparently, the test result does not depend on the handlers set up actually returning events.

- all other tests (run on macos, `ninja -C out/host check`) pass

As making use of Request/ClearCallbackXXX without having a dispatch queue installed points to a problem (most likely only occurring in tests with only partial system init), I added a ChipLogError to make this visible.
  • Loading branch information
@plan44
plan44 committed Jul 27, 2022
1 parent fd85659 commit cc19913
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 63 deletions.
156 changes: 93 additions & 63 deletions src/system/SystemLayerImplSelect.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,12 @@ void LayerImplSelect::Shutdown()
}
}
mTimerPool.ReleaseAll();

for (auto & w : mSocketWatchPool)
{
w.DisableAndClear();
}

#else // CHIP_SYSTEM_CONFIG_USE_DISPATCH
mTimerList.Clear();
mTimerPool.ReleaseAll();
Expand Down Expand Up @@ -245,29 +251,44 @@ CHIP_ERROR LayerImplSelect::RequestCallbackOnPendingRead(SocketWatchToken token)
SocketWatch * watch = reinterpret_cast<SocketWatch *>(token);
VerifyOrReturnError(watch != nullptr, CHIP_ERROR_INVALID_ARGUMENT);

watch->mPendingIO.Set(SocketEventFlags::kRead);

#if CHIP_SYSTEM_CONFIG_USE_DISPATCH
dispatch_queue_t dispatchQueue = GetDispatchQueue();
if (watch->mWrSource)
{
dispatch_resume(watch->mRdSource);
}
else
if (!watch->mPendingIO.Has(SocketEventFlags::kRead))
{
if (dispatchQueue)
// Not yet watching
if (watch->mRdSource)
{
watch->mRdSource =
dispatch_source_create(DISPATCH_SOURCE_TYPE_READ, static_cast<uintptr_t>(watch->mFD), 0, dispatchQueue);
ReturnErrorCodeIf(watch->mRdSource == nullptr, CHIP_ERROR_NO_MEMORY);
dispatch_source_set_event_handler(watch->mRdSource, ^{
SocketEvents events;
events.Set(SocketEventFlags::kRead);
watch->mCallback(events, watch->mCallbackData);
});
dispatch_activate(watch->mRdSource);
// already used once before, but callback disabled later: just set flag again
watch->mPendingIO.Set(SocketEventFlags::kRead);
}
else
{
// first time watching for read events: install a dispatch source
// Note: if no dispatch queue is available, this is a complete NOP (does not even set the flag)
dispatch_queue_t dispatchQueue = GetDispatchQueue();
if (dispatchQueue == nullptr)
{
ChipLogError(DeviceLayer, "RequestCallbackOnPendingRead with no dispatch queue installed: callback will not work");
}
else {
watch->mRdSource =
dispatch_source_create(DISPATCH_SOURCE_TYPE_READ, static_cast<uintptr_t>(watch->mFD), 0, dispatchQueue);
ReturnErrorCodeIf(watch->mRdSource == nullptr, CHIP_ERROR_NO_MEMORY);
dispatch_source_set_event_handler(watch->mRdSource, ^{
if (watch->mPendingIO.Has(SocketEventFlags::kRead) && watch->mCallback != nullptr)
{
SocketEvents events;
events.Set(SocketEventFlags::kRead);
watch->mCallback(events, watch->mCallbackData);
}
});
// only now we are sure the source exists and can become active
watch->mPendingIO.Set(SocketEventFlags::kRead);
dispatch_activate(watch->mRdSource);
}
}
}
#else
watch->mPendingIO.Set(SocketEventFlags::kRead);
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH

return CHIP_NO_ERROR;
Expand All @@ -278,29 +299,44 @@ CHIP_ERROR LayerImplSelect::RequestCallbackOnPendingWrite(SocketWatchToken token
SocketWatch * watch = reinterpret_cast<SocketWatch *>(token);
VerifyOrReturnError(watch != nullptr, CHIP_ERROR_INVALID_ARGUMENT);

watch->mPendingIO.Set(SocketEventFlags::kWrite);

#if CHIP_SYSTEM_CONFIG_USE_DISPATCH
if (watch->mWrSource)
if (!watch->mPendingIO.Has(SocketEventFlags::kWrite))
{
dispatch_resume(watch->mWrSource);
}
else
{
dispatch_queue_t dispatchQueue = GetDispatchQueue();
if (dispatchQueue)
// Not yet watching
if (watch->mWrSource)
{
watch->mWrSource =
dispatch_source_create(DISPATCH_SOURCE_TYPE_WRITE, static_cast<uintptr_t>(watch->mFD), 0, dispatchQueue);
ReturnErrorCodeIf(watch->mWrSource == nullptr, CHIP_ERROR_NO_MEMORY);
dispatch_source_set_event_handler(watch->mWrSource, ^{
SocketEvents events;
events.Set(SocketEventFlags::kWrite);
watch->mCallback(events, watch->mCallbackData);
});
dispatch_activate(watch->mWrSource);
// already used once before, but callback disabled later: just set flag again
watch->mPendingIO.Set(SocketEventFlags::kWrite);
}
else
{
// first time watching for write events: install a dispatch source
// Note: if no dispatch queue is available, this is a complete NOP (does not even set the flag)
dispatch_queue_t dispatchQueue = GetDispatchQueue();
if (dispatchQueue == nullptr)
{
ChipLogError(DeviceLayer, "RequestCallbackOnPendingWrite with no dispatch queue installed: callback will not work");
}
else {
watch->mWrSource =
dispatch_source_create(DISPATCH_SOURCE_TYPE_WRITE, static_cast<uintptr_t>(watch->mFD), 0, dispatchQueue);
ReturnErrorCodeIf(watch->mWrSource == nullptr, CHIP_ERROR_NO_MEMORY);
dispatch_source_set_event_handler(watch->mWrSource, ^{
if (watch->mPendingIO.Has(SocketEventFlags::kWrite) && watch->mCallback != nullptr)
{
SocketEvents events;
events.Set(SocketEventFlags::kWrite);
watch->mCallback(events, watch->mCallbackData);
}
});
// only now we are sure the source exists and can become active
watch->mPendingIO.Set(SocketEventFlags::kWrite);
dispatch_activate(watch->mWrSource);
}
}
}
#else
watch->mPendingIO.Set(SocketEventFlags::kWrite);
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH

return CHIP_NO_ERROR;
Expand All @@ -310,14 +346,8 @@ CHIP_ERROR LayerImplSelect::ClearCallbackOnPendingRead(SocketWatchToken token)
{
SocketWatch * watch = reinterpret_cast<SocketWatch *>(token);
VerifyOrReturnError(watch != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
watch->mPendingIO.Clear(SocketEventFlags::kRead);

#if CHIP_SYSTEM_CONFIG_USE_DISPATCH
if (watch->mRdSource)
{
dispatch_suspend(watch->mRdSource);
}
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH
watch->mPendingIO.Clear(SocketEventFlags::kRead);

return CHIP_NO_ERROR;
}
Expand All @@ -326,14 +356,8 @@ CHIP_ERROR LayerImplSelect::ClearCallbackOnPendingWrite(SocketWatchToken token)
{
SocketWatch * watch = reinterpret_cast<SocketWatch *>(token);
VerifyOrReturnError(watch != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
watch->mPendingIO.Clear(SocketEventFlags::kWrite);

#if CHIP_SYSTEM_CONFIG_USE_DISPATCH
if (watch->mWrSource)
{
dispatch_suspend(watch->mWrSource);
}
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH
watch->mPendingIO.Clear(SocketEventFlags::kWrite);

return CHIP_NO_ERROR;
}
Expand All @@ -347,21 +371,10 @@ CHIP_ERROR LayerImplSelect::StopWatchingSocket(SocketWatchToken * tokenInOut)
VerifyOrReturnError(watch->mFD >= 0, CHIP_ERROR_INCORRECT_STATE);

#if CHIP_SYSTEM_CONFIG_USE_DISPATCH
if (watch->mRdSource)
{
dispatch_cancel(watch->mRdSource);
dispatch_release(watch->mRdSource);
}
if (watch->mWrSource)
{
dispatch_cancel(watch->mWrSource);
dispatch_release(watch->mWrSource);
}
#endif

watch->DisableAndClear();
#else
watch->Clear();

#if !CHIP_SYSTEM_CONFIG_USE_DISPATCH
// Wake the thread calling select so that it stops selecting on the socket.
Signal();
#endif
Expand Down Expand Up @@ -511,5 +524,22 @@ void LayerImplSelect::SocketWatch::Clear()
#endif
}

#if CHIP_SYSTEM_CONFIG_USE_DISPATCH
void LayerImplSelect::SocketWatch::DisableAndClear()
{
if (mRdSource)
{
dispatch_source_cancel(mRdSource);
dispatch_release(mRdSource);
}
if (mWrSource)
{
dispatch_source_cancel(mWrSource);
dispatch_release(mWrSource);
}
Clear();
}
#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH

} // namespace System
} // namespace chip
1 change: 1 addition & 0 deletions src/system/SystemLayerImplSelect.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ class LayerImplSelect : public LayerSocketsLoop
#if CHIP_SYSTEM_CONFIG_USE_DISPATCH
dispatch_source_t mRdSource;
dispatch_source_t mWrSource;
void DisableAndClear();
#endif
intptr_t mCallbackData;
};
Expand Down

0 comments on commit cc19913

Please sign in to comment.