Skip to content

Commit

Permalink
Sanitization squash commit (#225)
Browse files Browse the repository at this point in the history 
Changes prompted by bug hunt and asan.

- Copy .clang-format from pc-ble-driver.
- Refactor to automatic memory management in several places.
- Fix delete with incorrect type.
- Add debug logging to system tests script.
  • Loading branch information
@alwa-nordic
alwa-nordic authored Jun 17, 2019
1 parent c92a3a8 commit 9eeabd0
Show file tree
Hide file tree
Showing 10 changed files with 240 additions and 128 deletions.
108 changes: 108 additions & 0 deletions .clang-format
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
---
Language: Cpp
AccessModifierOffset: -2
AlignAfterOpenBracket: Align
AlignConsecutiveAssignments: true
AlignConsecutiveDeclarations: false
AlignEscapedNewlines: Right
AlignOperands: true
AlignTrailingComments: true
AllowAllParametersOfDeclarationOnNextLine: true
AllowShortBlocksOnASingleLine: false
AllowShortCaseLabelsOnASingleLine: false
AllowShortFunctionsOnASingleLine: None
AllowShortIfStatementsOnASingleLine: false
AllowShortLoopsOnASingleLine: false
AlwaysBreakAfterDefinitionReturnType: None
AlwaysBreakAfterReturnType: None
AlwaysBreakBeforeMultilineStrings: false
AlwaysBreakTemplateDeclarations: false
BinPackArguments: true
BinPackParameters: true
BraceWrapping:
AfterClass: true
AfterControlStatement: true
AfterEnum: false
AfterFunction: true
AfterNamespace: false
AfterObjCDeclaration: false
AfterStruct: true
AfterUnion: false
AfterExternBlock: false
BeforeCatch: true
BeforeElse: true
IndentBraces: false
SplitEmptyFunction: false
SplitEmptyRecord: false
SplitEmptyNamespace: false
BreakBeforeBinaryOperators: None
BreakBeforeBraces: Custom
BreakBeforeInheritanceComma: false
BreakBeforeTernaryOperators: true
BreakConstructorInitializersBeforeComma: true
BreakConstructorInitializers: BeforeColon
BreakAfterJavaFieldAnnotations: false
BreakStringLiterals: true
ColumnLimit: 100
CommentPragmas: '^ IWYU pragma:'
CompactNamespaces: false
ConstructorInitializerAllOnOneLineOrOnePerLine: false
ConstructorInitializerIndentWidth: 4
ContinuationIndentWidth: 4
Cpp11BracedListStyle: true
DerivePointerAlignment: false
DisableFormat: false
ExperimentalAutoDetectBinPacking: false
FixNamespaceComments: true
ForEachMacros:
- foreach
- Q_FOREACH
- BOOST_FOREACH
IncludeBlocks: Preserve
IncludeCategories:
- Regex: '^"(llvm|llvm-c|clang|clang-c)/'
Priority: 2
- Regex: '^(<|"(gtest|gmock|isl|json)/)'
Priority: 3
- Regex: '.*'
Priority: 1
IncludeIsMainRegex: '(Test)?$'
IndentCaseLabels: true
IndentPPDirectives: None
IndentWidth: 4
IndentWrappedFunctionNames: false
JavaScriptQuotes: Leave
JavaScriptWrapImports: true
KeepEmptyLinesAtTheStartOfBlocks: false
MacroBlockBegin: ''
MacroBlockEnd: ''
MaxEmptyLinesToKeep: 1
NamespaceIndentation: None
ObjCBlockIndentWidth: 2
ObjCSpaceAfterProperty: false
ObjCSpaceBeforeProtocolList: true
PenaltyBreakAssignment: 2
PenaltyBreakBeforeFirstCallParameter: 19
PenaltyBreakComment: 300
PenaltyBreakFirstLessLess: 120
PenaltyBreakString: 1000
PenaltyExcessCharacter: 1000000
PenaltyReturnTypeOnItsOwnLine: 60
PointerAlignment: Right
ReflowComments: true
SortIncludes: true
SortUsingDeclarations: true
SpaceAfterCStyleCast: false
SpaceAfterTemplateKeyword: true
SpaceBeforeAssignmentOperators: true
SpaceBeforeParens: ControlStatements
SpaceInEmptyParentheses: false
SpacesBeforeTrailingComments: 1
SpacesInAngles: false
SpacesInContainerLiterals: true
SpacesInCStyleCastParentheses: false
SpacesInParentheses: false
SpacesInSquareBrackets: false
Standard: Cpp11
TabWidth: 8
UseTab: Never
2 changes: 2 additions & 0 deletions scripts/system-tests.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ set -eux
test_pcas=(PCA10031 PCA10028 PCA10040 PCA10056)

export BLE_DRIVER_TEST_OPENCLOSE_ITERATIONS=5
export BLE_DRIVER_TEST_LOGLEVEL="trace"
export DEBUG="ble-driver:*"

for pca in "${test_pcas[@]}"; do
serial_numbers=($(npx nrf-device-lister -S "$pca"))
Expand Down
148 changes: 84 additions & 64 deletions src/adapter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,19 @@ adapter_t *Adapter::getInternalAdapter() const
return adapter;
}

extern "C" {
// This compilation unit will be linked several times. So
// log_handler must not have external linkage. Otherwise, we get
// problems like a v3 Adapter getting cast into a v2 Adapter.
// Ideally, only things specifically needing external linkage
// should have it.
namespace {
// Turns out the right way to declare a function that can be
// called trough a pointer in C code is simply to declare it
// having the type we want that was typedef-ed inside a
// extern-C block. `uv_async_cb` pretty much fits the bill,
// but.. it's pointer to callback function, while we are
// declaring the actual function. So:
std::remove_pointer<uv_async_cb>::type event_handler;
void event_handler(uv_async_t *handle)
{
auto adapter = static_cast<Adapter *>(handle->data);
Expand All @@ -103,6 +115,7 @@ extern "C" {
}
}

std::remove_pointer<uv_async_cb>::type event_interval_handler;
void event_interval_handler(uv_timer_t *handle)
{
auto adapter = static_cast<Adapter *>(handle->data);
Expand All @@ -119,16 +132,16 @@ extern "C" {
}
}

void Adapter::initEventHandling(std::unique_ptr<Nan::Callback> &callback, uint32_t interval)
void Adapter::initEventHandling(std::unique_ptr<Nan::Callback> callback, uint32_t interval)
{
eventInterval = interval;
asyncEvent = new uv_async_t();
asyncEvent = std::make_unique<uv_async_t>();

// Setup event related functionality
eventCallback = std::move(callback);
asyncEvent->data = static_cast<void *>(this);

if (uv_async_init(uv_default_loop(), asyncEvent, event_handler) != 0)
if (uv_async_init(uv_default_loop(), asyncEvent.get(), event_handler) != 0)
{
std::cerr << "Not able to create a new async event handler." << std::endl;
std::terminate();
Expand All @@ -150,26 +163,38 @@ void Adapter::initEventHandling(std::unique_ptr<Nan::Callback> &callback, uint32

if (eventIntervalTimer == nullptr)
{
eventIntervalTimer = new uv_timer_t();
eventIntervalTimer = std::make_unique<uv_timer_t>();
}

// Setup event interval functionality
eventIntervalTimer->data = static_cast<void *>(this);

if (uv_timer_init(uv_default_loop(), eventIntervalTimer) != 0)
if (uv_timer_init(uv_default_loop(), eventIntervalTimer.get()) != 0)
{
std::cerr << "Not able to create a new async event interval timer." << std::endl;
std::terminate();
}

if (uv_timer_start(eventIntervalTimer, event_interval_handler, eventInterval, eventInterval) != 0)
if (uv_timer_start(eventIntervalTimer.get(), event_interval_handler, eventInterval, eventInterval) != 0)
{
std::cerr << "Not able to create a new event interval handler." << std::endl;
std::terminate();
}
}

extern "C" {
// This compilation unit will be linked several times. So
// log_handler must not have external linkage. Otherwise, we get
// problems like a v3 Adapter getting cast into a v2 Adapter.
// Ideally, only things specifically needing external linkage
// should have it.
namespace {
// Turns out the right way to declare a function that can be
// called trough a pointer in C code is simply to declare it
// having the type we want that was typedef-ed inside a
// extern-C block. `uv_async_cb` pretty much fits the bill,
// but.. it's pointer to callback function, while we are
// declaring the actual function. So:
std::remove_pointer<uv_async_cb>::type log_handler;
void log_handler(uv_async_t *handle)
{
auto adapter = static_cast<Adapter *>(handle->data);
Expand All @@ -186,21 +211,33 @@ extern "C" {
}
}

void Adapter::initLogHandling(std::unique_ptr<Nan::Callback> &callback)
void Adapter::initLogHandling(std::unique_ptr<Nan::Callback> callback)
{
// Setup event related functionality
asyncLog = new uv_async_t();
asyncLog = std::make_unique<uv_async_t>();
logCallback = std::move(callback);
asyncLog->data = static_cast<void *>(this);

if (uv_async_init(uv_default_loop(), asyncLog, log_handler) != 0)
if (uv_async_init(uv_default_loop(), asyncLog.get(), log_handler) != 0)
{
std::cerr << "Not able to create a new event log handler." << std::endl;
std::terminate();
}
}

extern "C" {
// This compilation unit will be linked several times. So
// log_handler must not have external linkage. Otherwise, we get
// problems like a v3 Adapter getting cast into a v2 Adapter.
// Ideally, only things specifically needing external linkage
// should have it.
namespace {
// Turns out the right way to declare a function that can be
// called trough a pointer in C code is simply to declare it
// having the type we want that was typedef-ed inside a
// extern-C block. `uv_async_cb` pretty much fits the bill,
// but.. it's pointer to callback function, while we are
// declaring the actual function. So:
std::remove_pointer<uv_async_cb>::type status_handler;
void status_handler(uv_async_t *handle)
{
auto adapter = static_cast<Adapter *>(handle->data);
Expand All @@ -217,78 +254,72 @@ extern "C" {
}
}

void Adapter::initStatusHandling(std::unique_ptr<Nan::Callback> &callback)
void Adapter::initStatusHandling(std::unique_ptr<Nan::Callback> callback)
{
// Setup event related functionality
asyncStatus = new uv_async_t();
asyncStatus = std::make_unique<uv_async_t>();
statusCallback = std::move(callback);
asyncStatus->data = static_cast<void *>(this);

if (uv_async_init(uv_default_loop(), asyncStatus, status_handler) != 0)
if (uv_async_init(uv_default_loop(), asyncStatus.get(), status_handler) != 0)
{
std::cerr << "Not able to create a new status handler." << std::endl;
std::terminate();
}
}

// Helper function for cleanUpV8Resources for closing uv_*_t
// handles. It is also suitable as a Deleter (template argment
// of unique_ptr).
namespace {
template<typename T_uv_handle_t_derived>
void close_uv_handle(std::unique_ptr<T_uv_handle_t_derived> handle)
{
// Ensure the type is expected. All the other subtypes
// of uv_handle_t can be added as needed.
static_assert(std::is_same<T_uv_handle_t_derived, uv_async_t>::value
|| std::is_same<T_uv_handle_t_derived, uv_timer_t>::value
, "Not sure if T_uv_handle_t_derived is a uv_handle_t.");

// The callback from uv_close is the earliest the handle
// memory can be released according to libuv. C++ delete
// requires the pointer have the original type.
auto raw_handle = reinterpret_cast<uv_handle_t *>(handle.release());
uv_close(raw_handle, [](uv_handle_t * raw_handle){
delete reinterpret_cast<T_uv_handle_t_derived *>(raw_handle);
});
}
}

void Adapter::cleanUpV8Resources()
{
uv_mutex_lock(adapterCloseMutex);
uv_mutex_lock(&adapterCloseMutex);

if (asyncStatus != nullptr)
{
auto handle = reinterpret_cast<uv_handle_t *>(asyncStatus);
uv_close(handle, [](uv_handle_t *handle) {
delete handle;
});
close_uv_handle(std::move(asyncStatus));
this->statusCallback.reset();

asyncStatus = nullptr;
}

if (eventIntervalTimer != nullptr)
{
// Deallocate resources related to the event handling interval timer
if (uv_timer_stop(eventIntervalTimer) != 0)
{
std::terminate();
}

auto handle = reinterpret_cast<uv_handle_t *>(eventIntervalTimer);
uv_close(handle, [](uv_handle_t *handle)
{
delete handle;
});

eventIntervalTimer = nullptr;
uv_timer_stop(eventIntervalTimer.get());
close_uv_handle(std::move(eventIntervalTimer));
}

if (asyncEvent != nullptr)
{
auto handle = reinterpret_cast<uv_handle_t *>(asyncEvent);

uv_close(handle, [](uv_handle_t *handle)
{
delete handle;
});
close_uv_handle(std::move(asyncEvent));
this->eventCallback.reset();

asyncEvent = nullptr;
}

if (asyncLog != nullptr)
{
auto logHandle = reinterpret_cast<uv_handle_t *>(asyncLog);
uv_close(logHandle, [](uv_handle_t *handle)
{
delete handle;
});
close_uv_handle(std::move(asyncLog));
this->logCallback.reset();

asyncLog = nullptr;
}

uv_mutex_unlock(adapterCloseMutex);
uv_mutex_unlock(&adapterCloseMutex);
}

void Adapter::initGeneric(v8::Local<v8::FunctionTemplate> tpl)
Expand Down Expand Up @@ -384,17 +415,7 @@ Adapter::Adapter()
eventCallbackBatchEventTotalCount = 0;
eventCallbackBatchNumber = 0;

eventCallback = nullptr;

eventIntervalTimer = nullptr;

asyncEvent = nullptr;
asyncLog = nullptr;
asyncStatus = nullptr;

adapterCloseMutex = new uv_mutex_t();

if (uv_mutex_init(adapterCloseMutex) != 0)
if (uv_mutex_init(&adapterCloseMutex) != 0)
{
std::cerr << "Not able to create adapterCloseMutex! Terminating." << std::endl;
std::terminate();
Expand All @@ -411,8 +432,7 @@ Adapter::~Adapter()
// Remove callbacks and cleanup uv_handle_t instances
cleanUpV8Resources();

uv_mutex_destroy(adapterCloseMutex);
delete adapterCloseMutex;
uv_mutex_destroy(&adapterCloseMutex);
}

NAN_METHOD(Adapter::New)
Expand Down
Loading

0 comments on commit 9eeabd0

Please sign in to comment.