Skip to content

Commit

Permalink
Dump details about leaked ExchangeContexts before aborting (#34617)
Browse files Browse the repository at this point in the history
* Dump details about leaked ExchangeContexts before aborting

This is implemented via a VerifyOrDieWithObject() variant of the existing
VerifyOrDie() macro that calls a DumpToLog() method on the provided object if
it exists (otherwise this is simply a no-op).

If CHIP_CONFIG_VERBOSE_VERIFY_OR_DIE is not enabled, VerifyOrDieWithObject()
simply behaves like a plain VerifyOrDie(). DumpToLog() implementations can use
ChipLogFormatRtti to log type information about an object (usually a delegate);
if RTTI is disabled this simply outputs whether the object was null or not.

* Address review comments

* Make gcc happy and improve documentation

* Remove unused include

* Fix compile error without CHIP_CONFIG_VERBOSE_VERIFY_OR_DIE

* Avoid unused parameter warning
  • Loading branch information
ksperling-apple authored and pull[bot] committed Oct 2, 2024
1 parent c063ce2 commit ff87cf2
Show file tree
Hide file tree
Showing 9 changed files with 172 additions and 8 deletions.
6 changes: 5 additions & 1 deletion src/lib/support/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ source_set("logging_constants") {

source_set("attributes") {
sources = [
"Compiler.h",
"DLLUtil.h",
"EnforceFormat.h",
]
Expand Down Expand Up @@ -132,7 +133,10 @@ source_set("text_only_logging") {
}

source_set("verifymacros") {
sources = [ "CodeUtils.h" ]
sources = [
"CodeUtils.h",
"ObjectDump.h",
]

public_deps = [
":attributes",
Expand Down
16 changes: 16 additions & 0 deletions src/lib/support/CodeUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include <lib/core/CHIPConfig.h>
#include <lib/core/CHIPError.h>
#include <lib/core/ErrorStr.h>
#include <lib/support/ObjectDump.h>
#include <lib/support/VerificationMacrosNoLogging.h>
#include <lib/support/logging/TextOnlyLogging.h>

Expand Down Expand Up @@ -547,6 +548,21 @@ inline void chipDie(void)
#define VerifyOrDie(aCondition) VerifyOrDieWithoutLogging(aCondition)
#endif // CHIP_CONFIG_VERBOSE_VERIFY_OR_DIE

/**
* @def VerifyOrDieWithObject(aCondition, aObject)
*
* Like VerifyOrDie(), but calls DumpObjectToLog()
* on the provided object on failure before aborting
* if CHIP_CONFIG_VERBOSE_VERIFY_OR_DIE is enabled.
*/
#if CHIP_CONFIG_VERBOSE_VERIFY_OR_DIE
#define VerifyOrDieWithObject(aCondition, aObject) \
nlABORT_ACTION(aCondition, ::chip::DumpObjectToLog(aObject); \
ChipLogError(Support, "VerifyOrDie failure at %s:%d: %s", __FILE__, __LINE__, #aCondition))
#else // CHIP_CONFIG_VERBOSE_VERIFY_OR_DIE
#define VerifyOrDieWithObject(aCondition, aObject) VerifyOrDieWithoutLogging(aCondition)
#endif // CHIP_CONFIG_VERBOSE_VERIFY_OR_DIE

/**
* @def VerifyOrDieWithMsg(aCondition, aModule, aMessage, ...)
*
Expand Down
24 changes: 24 additions & 0 deletions src/lib/support/Compiler.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
* Copyright (c) 2024 Project CHIP Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

// CHIP_HAVE_RTTI: Is C++ RTTI enabled?
#if defined(__clang__)
#define CHIP_HAVE_RTTI __has_feature(cxx_rtti)
#elif defined(__GNUC__) && defined(__GXX_RTTI)
#define CHIP_HAVE_RTTI 1
#else
#define CHIP_HAVE_RTTI 0
#endif
57 changes: 57 additions & 0 deletions src/lib/support/ObjectDump.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
*
* Copyright (c) 2024 Project CHIP Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#pragma once

#include <type_traits>
#include <utility>

namespace chip {

/**
* A dumpable object that can log some useful state for debugging in fatal
* error scenarios by exposing a `void DumpToLog() const` method. The method
* should log key details about the state of object using ChipLogError().
*/
template <class, class = void>
struct IsDumpable : std::false_type
{
};
template <class T>
struct IsDumpable<T, std::void_t<decltype(std::declval<T>().DumpToLog())>> : std::true_type
{
};

struct DumpableTypeExample
{
void DumpToLog() const {};
};
static_assert(IsDumpable<DumpableTypeExample>::value);

/**
* Calls DumpToLog() on the object, if supported.
*/
template <class T>
void DumpObjectToLog([[maybe_unused]] const T * object)
{
if constexpr (IsDumpable<T>::value)
{
object->DumpToLog();
}
}

} // namespace chip
29 changes: 27 additions & 2 deletions src/lib/support/Pool.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

#include <lib/support/CHIPMem.h>
#include <lib/support/CodeUtils.h>
#include <lib/support/ObjectDump.h>
#include <system/SystemConfig.h>

#include <lib/support/Iterators.h>
Expand Down Expand Up @@ -263,7 +264,7 @@ class BitMapObjectPool : public internal::StaticAllocatorBitmap
{
public:
BitMapObjectPool() : StaticAllocatorBitmap(mData.mMemory, mUsage, N, sizeof(T)) {}
~BitMapObjectPool() { VerifyOrDie(Allocated() == 0); }
~BitMapObjectPool() { VerifyOrDieWithObject(Allocated() == 0, this); }

BitmapActiveObjectIterator<T> begin() { return BitmapActiveObjectIterator<T>(this, FirstActiveIndex()); }
BitmapActiveObjectIterator<T> end() { return BitmapActiveObjectIterator<T>(this, N); }
Expand Down Expand Up @@ -323,6 +324,18 @@ class BitMapObjectPool : public internal::StaticAllocatorBitmap
return ForEachActiveObjectInner(&proxy, &internal::LambdaProxy<T, Function>::ConstCall);
}

void DumpToLog() const
{
ChipLogError(Support, "BitMapObjectPool: %lu allocated", static_cast<unsigned long>(Allocated()));
if constexpr (IsDumpable<T>::value)
{
ForEachActiveObject([](const T * object) {
object->DumpToLog();
return Loop::Continue;
});
}
}

private:
static Loop ReleaseObject(void * context, void * object)
{
Expand Down Expand Up @@ -389,7 +402,7 @@ class HeapObjectPool : public internal::Statistics, public HeapObjectPoolExitHan
if (!sIgnoringLeaksOnExit)
{
// Verify that no live objects remain, to prevent potential use-after-free.
VerifyOrDie(Allocated() == 0);
VerifyOrDieWithObject(Allocated() == 0, this);
}
#endif // __SANITIZE_ADDRESS__
}
Expand Down Expand Up @@ -570,6 +583,18 @@ class HeapObjectPool : public internal::Statistics, public HeapObjectPoolExitHan
return mObjects.ForEachNode(&proxy, &internal::LambdaProxy<const T, Function>::ConstCall);
}

void DumpToLog() const
{
ChipLogError(Support, "HeapObjectPool: %lu allocated", static_cast<unsigned long>(Allocated()));
if constexpr (IsDumpable<T>::value)
{
ForEachActiveObject([](const T * object) {
object->DumpToLog();
return Loop::Continue;
});
}
}

private:
static Loop ReleaseObject(void * context, void * object)
{
Expand Down
20 changes: 20 additions & 0 deletions src/lib/support/logging/TextOnlyLogging.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@

#include <lib/core/CHIPConfig.h>

#include <lib/support/Compiler.h>
#include <lib/support/DLLUtil.h>
#include <lib/support/EnforceFormat.h>
#include <lib/support/VerificationMacrosNoLogging.h>
Expand All @@ -44,6 +45,7 @@
#include <inttypes.h>
#include <stdarg.h>
#include <stdint.h>
#include <typeinfo>

#if CHIP_SYSTEM_CONFIG_PLATFORM_LOG && defined(CHIP_SYSTEM_CONFIG_PLATFORM_LOG_INCLUDE)
#include CHIP_SYSTEM_CONFIG_PLATFORM_LOG_INCLUDE
Expand Down Expand Up @@ -292,6 +294,24 @@ using LogRedirectCallback_t = void (*)(const char * module, uint8_t category, co
#define ChipLogValueExchangeIdFromReceivedHeader(payloadHeader) \
ChipLogValueExchangeId((payloadHeader).GetExchangeID(), !(payloadHeader).IsInitiator())

/**
* Logging helpers for logging the dynamic type of an object, if possible.
*
* Primarily useful when logging the type of delegates or similar objects when
* performing logging for a fatal error in DumpToLog().
*
* Example:
* @code
* ChipLogError(Foo, "Delegate=" ChipLogFormatRtti, ChipLogValueRtti(mDelegate));
* @endcode
*/
#define ChipLogFormatRtti "%s"
#if CHIP_HAVE_RTTI
#define ChipLogValueRtti(ptr) ((ptr) != nullptr ? typeid(*(ptr)).name() : "null")
#else
#define ChipLogValueRtti(ptr) ((ptr) != nullptr ? "?" : "null")
#endif

/**
* Logging helpers for protocol ids. A protocol id is a (vendor-id,
* protocol-id) pair.
Expand Down
8 changes: 4 additions & 4 deletions src/messaging/ExchangeContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, cons
mDispatch(GetMessageDispatch(isEphemeralExchange, delegate)),
mSession(*this)
{
VerifyOrDie(mExchangeMgr == nullptr);
VerifyOrDieWithObject(mExchangeMgr == nullptr, this);

mExchangeMgr = em;
mExchangeId = ExchangeId;
Expand Down Expand Up @@ -334,12 +334,12 @@ ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, cons

ExchangeContext::~ExchangeContext()
{
VerifyOrDie(mExchangeMgr != nullptr && GetReferenceCount() == 0);
VerifyOrDieWithObject(mExchangeMgr != nullptr && GetReferenceCount() == 0, this);

//
// Ensure that DoClose has been called by the time we get here. If not, we have a leak somewhere.
//
VerifyOrDie(mFlags.Has(Flags::kFlagClosed));
VerifyOrDieWithObject(mFlags.Has(Flags::kFlagClosed), this);

#if CHIP_CONFIG_ENABLE_ICD_SERVER
// TODO(#33075) : Add check for group context to not a req since it serves no purpose
Expand Down Expand Up @@ -666,7 +666,7 @@ void ExchangeContext::AbortAllOtherCommunicationOnFabric()

void ExchangeContext::ExchangeSessionHolder::GrabExpiredSession(const SessionHandle & session)
{
VerifyOrDie(session->AsSecureSession()->IsPendingEviction());
VerifyOrDieWithObject(session->AsSecureSession()->IsPendingEviction(), this);
GrabUnchecked(session);
}

Expand Down
8 changes: 7 additions & 1 deletion src/messaging/ExchangeContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext,

SessionHandle GetSessionHandle() const
{
VerifyOrDie(mSession);
VerifyOrDieWithObject(mSession, this);
auto sessionHandle = mSession.Get();
return std::move(sessionHandle.Value());
}
Expand Down Expand Up @@ -238,6 +238,12 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext,
void ClearInjectedFailures() { mInjectedFailures.ClearAll(); }
#endif

void DumpToLog() const
{
ChipLogError(ExchangeManager, "ExchangeContext: " ChipLogFormatExchangeId " delegate=" ChipLogFormatRtti,
ChipLogValueExchangeId(GetExchangeId(), IsInitiator()), ChipLogValueRtti(mDelegate));
}

private:
#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
BitFlags<InjectedFailureType> mInjectedFailures;
Expand Down
12 changes: 12 additions & 0 deletions src/messaging/tests/TestExchange.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <lib/core/StringBuilderAdapters.h>
#include <lib/support/CHIPMem.h>
#include <lib/support/CodeUtils.h>
#include <lib/support/Pool.h>
#include <messaging/ExchangeContext.h>
#include <messaging/ExchangeMgr.h>
#include <messaging/Flags.h>
Expand Down Expand Up @@ -219,4 +220,15 @@ TEST_F(TestExchange, CheckBasicExchangeMessageDispatch)
});
}
}

// A crude test to exercise VerifyOrDieWithObject() in ObjectPool and
// the resulting DumpToLog() call on the ExchangeContext.
// TODO: Find a way to automate this test without killing the process.
// TEST_F(TestExchange, DumpExchangePoolToLog)
// {
// MockExchangeDelegate delegate;
// ObjectPool<ExchangeContext, CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS> pool;
// pool.CreateObject(&GetExchangeManager(), static_cast<uint16_t>(1234), GetSessionAliceToBob(), true, &delegate);
// }

} // namespace

0 comments on commit ff87cf2

Please sign in to comment.