Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions compiler-rt/lib/rtsan/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,22 @@ set(RTSAN_CXX_SOURCES
rtsan_flags.cpp
rtsan_interceptors.cpp
rtsan_stats.cpp
rtsan_suppressions.cpp
)

set(RTSAN_PREINIT_SOURCES
rtsan_preinit.cpp)

set(RTSAN_HEADERS
rtsan.h
rtsan_checks.inc
rtsan_assertions.h
rtsan_context.h
rtsan_diagnostics.h
rtsan_flags.h
rtsan_flags.inc
rtsan_stats.h
rtsan_suppressions.h
)

set(RTSAN_DEPS)
Expand Down
58 changes: 27 additions & 31 deletions compiler-rt/lib/rtsan/rtsan.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@
#include "rtsan/rtsan_flags.h"
#include "rtsan/rtsan_interceptors.h"
#include "rtsan/rtsan_stats.h"
#include "rtsan/rtsan_suppressions.h"

#include "sanitizer_common/sanitizer_atomic.h"
#include "sanitizer_common/sanitizer_common.h"
#include "sanitizer_common/sanitizer_mutex.h"
#include "sanitizer_common/sanitizer_stackdepot.h"
#include "sanitizer_common/sanitizer_stacktrace.h"

using namespace __rtsan;
using namespace __sanitizer;
Expand All @@ -46,38 +46,30 @@ static InitializationState GetInitializationState() {
atomic_load(&rtsan_initialized, memory_order_acquire));
}

static auto OnViolationAction(DiagnosticsInfo info) {
return [info]() {
IncrementTotalErrorCount();
static auto OnViolationAction(const BufferedStackTrace &stack,
const DiagnosticsInfo &info) {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that now, when a violation occurs, ExpectNotRealtime passes in the stack and diagnostics info for more processing.

This function is probably best shown with whitespace turned off, I mostly just extracted things out of the lambda

IncrementTotalErrorCount();

BufferedStackTrace stack;
// If in the future we interop with other sanitizers, we will
// need to make our own stackdepot
StackDepotHandle handle = StackDepotPut_WithHandle(stack);

// We use the unwind_on_fatal flag here because of precedent with other
// sanitizers, this action is not necessarily fatal if halt_on_error=false
stack.Unwind(info.pc, info.bp, nullptr,
common_flags()->fast_unwind_on_fatal);
const bool is_stack_novel = handle.use_count() == 0;

// If in the future we interop with other sanitizers, we will
// need to make our own stackdepot
StackDepotHandle handle = StackDepotPut_WithHandle(stack);
// Marked UNLIKELY as if user is runing with halt_on_error=false
// we expect a high number of duplicate stacks. We are willing
// To pay for the first insertion.
if (UNLIKELY(is_stack_novel)) {
IncrementUniqueErrorCount();

const bool is_stack_novel = handle.use_count() == 0;
PrintDiagnostics(info);
stack.Print();

// Marked UNLIKELY as if user is runing with halt_on_error=false
// we expect a high number of duplicate stacks. We are willing
// To pay for the first insertion.
if (UNLIKELY(is_stack_novel)) {
IncrementUniqueErrorCount();
handle.inc_use_count_unsafe();
}

PrintDiagnostics(info);
stack.Print();

handle.inc_use_count_unsafe();
}

if (flags().halt_on_error)
Die();
};
if (flags().halt_on_error)
Die();
}

extern "C" {
Expand All @@ -90,6 +82,8 @@ SANITIZER_INTERFACE_ATTRIBUTE void __rtsan_init() {
InitializeFlags();
InitializeInterceptors();

InitializeSuppressions();

if (flags().print_stats_on_exit)
Atexit(PrintStatisticsSummary);

Expand Down Expand Up @@ -136,19 +130,21 @@ __rtsan_notify_intercepted_call(const char *func_name) {
return;

__rtsan_ensure_initialized();

GET_CALLER_PC_BP;

ExpectNotRealtime(GetContextForThisThread(),
OnViolationAction({DiagnosticsInfoType::InterceptedCall,
func_name, pc, bp}));
{DiagnosticsInfoType::InterceptedCall, func_name, pc, bp},
OnViolationAction);
}

SANITIZER_INTERFACE_ATTRIBUTE void
__rtsan_notify_blocking_call(const char *func_name) {
__rtsan_ensure_initialized();
GET_CALLER_PC_BP;
ExpectNotRealtime(GetContextForThisThread(),
OnViolationAction({DiagnosticsInfoType::BlockingCall,
func_name, pc, bp}));
{DiagnosticsInfoType::BlockingCall, func_name, pc, bp},
OnViolationAction);
}

} // extern "C"
40 changes: 36 additions & 4 deletions compiler-rt/lib/rtsan/rtsan_assertions.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,48 @@

#include "rtsan/rtsan.h"
#include "rtsan/rtsan_context.h"
#include "rtsan/rtsan_diagnostics.h"
#include "rtsan/rtsan_suppressions.h"

#include "sanitizer_common/sanitizer_stacktrace.h"

namespace __rtsan {

class ScopedBypass {
public:
[[nodiscard]] explicit ScopedBypass(Context &context) : context_(context) {
context_.BypassPush();
}

~ScopedBypass() { context_.BypassPop(); }

private:
Context &context_;
};
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go this direction, I will split this into it's own review, likely putting this in rtsan_context.h

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do without this and use __rtsan::ScopedDisabler instead?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm not sure if we have access to that construct here, but I'll investigate

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The answer is that no compiler-rt libs include these headers, except in tests. If you do a grep such as:

> rg "ubsan_interface.h" --glob "!*test*"
llvm/utils/gn/secondary/compiler-rt/include/BUILD.gn
25:    "sanitizer/ubsan_interface.h",

compiler-rt/include/sanitizer/ubsan_interface.h
1://===-- sanitizer/ubsan_interface.h -----------------------------*- C++ -*-===//
13:#ifndef SANITIZER_UBSAN_INTERFACE_H
14:#define SANITIZER_UBSAN_INTERFACE_H
32:#endif // SANITIZER_UBSAN_INTERFACE_H

compiler-rt/include/CMakeLists.txt

This shows up blank for all interface files. Based on this (for better or worse) we are left with re-implementing anything in our "top level header" down here in the depths of the impl.

We can theorize how to improve this (perhaps for us and all sanitizers), but I'm going to keep this as is for now


template <typename OnViolationAction>
void ExpectNotRealtime(Context &context, OnViolationAction &&OnViolation) {
void ExpectNotRealtime(Context &context, const DiagnosticsInfo &info,
OnViolationAction &&OnViolation) {
using namespace __sanitizer;

CHECK(__rtsan_is_initialized());
if (context.InRealtimeContext() && !context.IsBypassed()) {
context.BypassPush();
OnViolation();
context.BypassPop();
ScopedBypass scoped_bypass{context};

if (IsInterceptorSuppressed(info.func_name))
return;

BufferedStackTrace stack;

// We use the unwind_on_fatal flag here because of precedent with other
// sanitizers, this action is not necessarily fatal if halt_on_error=false
stack.Unwind(info.pc, info.bp, nullptr,
common_flags()->fast_unwind_on_fatal);

if (IsStackTraceSuppressed(stack))
return;

OnViolation(stack, info);
}
}

Expand Down
20 changes: 20 additions & 0 deletions compiler-rt/lib/rtsan/rtsan_checks.inc
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
//===-- rtsan_checks.inc ----------------------------------------*- C++ -*-===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
//
// List of checks handled by RTSan runtime.
//
//===----------------------------------------------------------------------===//
#ifndef RTSAN_CHECK
#error "Define RTSAN_CHECK prior to including this file!"
#endif

// RTSAN_CHECK(Name, SummaryKind)
// SummaryKind should be a string literal.

RTSAN_CHECK(FunctionCall, "function-call")
RTSAN_CHECK(Interceptor, "interceptor")
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we like the names of these? What else would we call them

FunctionCall is "does this call exist anywhere in the stack"

Interceptor right now disables checks at the lowest level for a blocking function or an intercepted call by name (malloc or my_call_with_attr_blocking)

  • Do we want to have two different disabling for each of them?
    • If so, what are their names and strings?
  • Do we even want people to be able to disable [[blocking]] calls?

Copy link

@davidtrevelyan davidtrevelyan Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah good questions. Here's my initial thoughts:

Q: Should we have different disabling keys for blocking, intercepted and stack functions?

A: I believe so, yes, for all three. I like to think we can improve on the names to make them more immediately understandable, even to people without a mental model of how the sanitizer works, but I'm struggling with interceptor. Here's my first iteration on the naming, but I don't think they're perfect at all, so keen to iterate more on them with you:

  • "improve" function-call to something more like call-stack-match
  • interceptor -> intercepted
  • add blocking

Q: Do we even want people to suppress [[blocking]] calls?

A: I think it could still be helpful, yes - I can imagine a situation where you're using a third-party library or re-using multi-threaded code in a single-threaded context (an noncontended spinlock here would always immediately grab a lock - the same reasoning here as it would apply to a normal mutex imho). I also worry that not suppressing them could potentially even be more confusing than otherwise.

Another question for you:

Q: Should we allow regex for intercepted and blocking? I can imagine a user might want to just suppress all pthread_.*, for example...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having said that, regarding naming, if there is precedent for these sorts of names in other sanitizers, I appreciate there might be a good argument for matching those...

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Should we have different disabling keys for blocking, intercepted and stack functions?
A: I believe so, yes, for all three.

Sounds good, I'm in favor of this. I think the only reason to avoid any of it would be implementation/runtime complexity, but both of those are very tame. I think more atomic is better

"improve" function-call to something more like call-stack-match
interceptor -> intercepted
add blocking

I like these generally, I'll give it some thought and we can continue hashing it out on the end PR

_Q: Do we even want people to suppress [[blocking]] calls?
A: I think it could still be helpful, yes

👍

Another question for you:
Q: Should we allow regex for intercepted and blocking? I can imagine a user might want to just suppress all pthread_.*, for example...

Perhaps, but I would encourage punting on this until later on. The regex engine definitely will slow down the string matching. Before we go deeper with that I'd want to let the "basic" version prove to be inadequate for folks.

Basically "can we prove that we need it" before we implement something more complex. I think we are providing a lot of flexibility to the users already

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One correction here (of myself):

Another question for you:
Q: Should we allow regex for intercepted and blocking? I can imagine a user might want to just suppress all pthread_.*, for example...

So by default (and without me knowing) we ARE using the regex engine for function matches. This is the function we use:

bool SuppressionContext::Match(const char *str, const char *type,
                               Suppression **s) {
  can_parse_ = false;
  if (!HasSuppressionType(type))
    return false;
  for (uptr i = 0; i < suppressions_.size(); i++) {
    Suppression &cur = suppressions_[i];
    if (0 == internal_strcmp(cur.type, type) && TemplateMatch(cur.templ, str)) {
      *s = &cur;
      return true;
    }
  }
  return false;
}

So early out for strcmp, then TemplateMatch, which is "regex light". It looks like it has a runtime of O(N), so it isn't more costly than just strcmp. We do both strcmp and TemplateMatch per function specified per type, so O(N_strlen*M_num_interceptor_suppressions).

Something to be aware of, that I don't think we will be able to get around -

If the user is using suppressions, the more suppressions they use the more that run-time is affected. We need to be clear that using a ScopedDisabler is the first tool they should reach for if at all possible, which has an O(1) runtime cost.

The hierarchy of speed:

  • ScopedDisabler (easy, early out, never compare strings, never unwind the stack) - O(1)
  • Function name suppressions, blocking and interceptors - O(N_strlen * M_suppressions_in_type)
  • Stack suppressions - O(N_strlen * M_suppressions_in_type * L_stack_frame_count)

(I think)

1 change: 1 addition & 0 deletions compiler-rt/lib/rtsan/rtsan_flags.inc
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,4 @@

RTSAN_FLAG(bool, halt_on_error, true, "Exit after first reported error.")
RTSAN_FLAG(bool, print_stats_on_exit, false, "Print stats on exit.")
RTSAN_FLAG(const char *, suppressions, "", "Suppressions file name.")
102 changes: 102 additions & 0 deletions compiler-rt/lib/rtsan/rtsan_suppressions.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
//===--- rtsan_suppressions.cpp - Realtime Sanitizer ------------*- C++ -*-===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
//
// This file is a part of the RTSan runtime, providing support for suppressions
//
//===----------------------------------------------------------------------===//

#include "rtsan/rtsan_suppressions.h"

#include "rtsan/rtsan_flags.h"

#include "sanitizer_common/sanitizer_common.h"
#include "sanitizer_common/sanitizer_internal_defs.h"
#include "sanitizer_common/sanitizer_suppressions.h"
#include "sanitizer_common/sanitizer_symbolizer.h"

#include <new>

using namespace __sanitizer;
using namespace __rtsan;

namespace {
enum class ErrorType {
#define RTSAN_CHECK(Name, FSanitizeFlagName) Name,
#include "rtsan_checks.inc"
#undef RTSAN_CHECK
};
} // namespace

alignas(64) static char suppression_placeholder[sizeof(SuppressionContext)];
static SuppressionContext *suppression_ctx = nullptr;
static const char *kSuppressionTypes[] = {
#define RTSAN_CHECK(Name, FSanitizeFlagName) FSanitizeFlagName,
#include "rtsan_checks.inc"
#undef RTSAN_CHECK
};

static const char *ConvertTypeToFlagName(ErrorType Type) {
switch (Type) {
#define RTSAN_CHECK(Name, FSanitizeFlagName) \
case ErrorType::Name: \
return FSanitizeFlagName;
#include "rtsan_checks.inc"
#undef RTSAN_CHECK
}
UNREACHABLE("unknown ErrorType!");
}

void __rtsan::InitializeSuppressions() {
CHECK_EQ(nullptr, suppression_ctx);

suppression_ctx = new (suppression_placeholder)
SuppressionContext(kSuppressionTypes, ARRAY_SIZE(kSuppressionTypes));
suppression_ctx->ParseFromFile(flags().suppressions);
}

bool __rtsan::IsStackTraceSuppressed(const StackTrace &stack) {
CHECK(suppression_ctx);

const char *function_call_type =
ConvertTypeToFlagName(ErrorType::FunctionCall);
if (!suppression_ctx->HasSuppressionType(function_call_type))
return false;

Symbolizer *symbolizer = Symbolizer::GetOrInit();
Suppression *s;

for (uptr i = 0; i < stack.size && stack.trace[i]; i++) {
uptr addr = stack.trace[i];

if (suppression_ctx->HasSuppressionType(function_call_type)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like there's quite a lot of code here that looks very similar to what's in asan's suppression implementation. Do you think it's worth abstracting anything in common into shared utility functions, or are they different enough to warrant their own implementations?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I would recommend investigating this in a follow up review. I don't want to abstract too soon and muddy up this change in isolation.

This could be a good refactor follow-on.

SymbolizedStackHolder symbolized_stack(symbolizer->SymbolizePC(addr));
const SymbolizedStack *frames = symbolized_stack.get();
CHECK(frames);
for (const SymbolizedStack *cur = frames; cur; cur = cur->next) {
const char *function_name = cur->info.function;
if (!function_name)
continue;

if (suppression_ctx->Match(function_name, function_call_type, &s))
return true;
}
}
}
return false;
}

bool __rtsan::IsInterceptorSuppressed(const char *interceptor_name) {
CHECK(suppression_ctx);

const char *interceptor = ConvertTypeToFlagName(ErrorType::Interceptor);
if (!suppression_ctx->HasSuppressionType(interceptor))
return false;

Suppression *s;
return suppression_ctx->Match(interceptor_name, interceptor, &s);
}
24 changes: 24 additions & 0 deletions compiler-rt/lib/rtsan/rtsan_suppressions.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
//===--- rtsan_suppressions.h - Realtime Sanitizer --------------*- C++ -*-===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
//
// This file is a part of the RTSan runtime, providing support for suppressions
//
//===----------------------------------------------------------------------===//

#pragma once

#include "sanitizer_common/sanitizer_common.h"
#include "sanitizer_common/sanitizer_stacktrace.h"

namespace __rtsan {

void InitializeSuppressions();
bool IsStackTraceSuppressed(const __sanitizer::StackTrace &stack);
bool IsInterceptorSuppressed(const char *interceptor_name);

} // namespace __rtsan
10 changes: 8 additions & 2 deletions compiler-rt/lib/rtsan/tests/rtsan_test_assertions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,11 @@

#include "rtsan/rtsan_assertions.h"

#include "sanitizer_common/sanitizer_stacktrace.h"

#include <gmock/gmock.h>

using namespace __sanitizer;
using namespace __rtsan;

class TestRtsanAssertions : public ::testing::Test {
Expand All @@ -25,9 +28,12 @@ class TestRtsanAssertions : public ::testing::Test {

static void ExpectViolationAction(Context &context,
bool expect_violation_callback) {
::testing::MockFunction<void()> mock_on_violation;
::testing::MockFunction<void(const BufferedStackTrace &stack,
const DiagnosticsInfo &info)>
mock_on_violation;
EXPECT_CALL(mock_on_violation, Call).Times(expect_violation_callback ? 1 : 0);
ExpectNotRealtime(context, mock_on_violation.AsStdFunction());
DiagnosticsInfo info{};
ExpectNotRealtime(context, info, mock_on_violation.AsStdFunction());
}

TEST_F(TestRtsanAssertions,
Expand Down
Loading