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
2 changes: 2 additions & 0 deletions compiler-rt/lib/radsan/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ set(RADSAN_PREINIT_SOURCES
set(RADSAN_HEADERS
radsan.h
radsan_context.h
radsan_flags.h
radsdan_flags.inc
radsan_stack.h)

set(RADSAN_DEPS)
Expand Down
113 changes: 107 additions & 6 deletions compiler-rt/lib/radsan/radsan.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,26 +7,127 @@
*/

#include <radsan/radsan.h>

#include <radsan/radsan_context.h>
#include <radsan/radsan_flags.h>
#include <radsan/radsan_interceptors.h>
#include <unistd.h>

#include "sanitizer_common/sanitizer_atomic.h"
#include "sanitizer_common/sanitizer_common.h"
#include "sanitizer_common/sanitizer_flag_parser.h"
#include "sanitizer_common/sanitizer_flags.h"

using namespace __sanitizer;

namespace radsan {
static __sanitizer::atomic_uint8_t radsan_inited{};

Choose a reason for hiding this comment

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

Why do we need to use the __sanitizer atomic types here rather than just std::atomic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question. I think it is possibly so these can run on systems that don't have standard libraries.

This hunch (and it's 100% a hunch, could be misinformed) was based on something I saw in MSAN, and is also in tsan and hwasan

compiler-rt/lib/hwasan/hwasan.cpp
37:// ACHTUNG! No system header includes in this file.

compiler-rt/lib/tsan/rtl/tsan_rtl.h
19://   - No system headers included in header files (*).

compiler-rt/lib/msan/msan.cpp
34:// ACHTUNG! No system header includes in this file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After fussing with this a bit, there are very easy ways for you to segfault in these sanitizers. These internal types are defined to not use any system calls that may or may not invoke an intercepted function.

For instance, I was trying to make a Meyer's singleton of a big type. Because it was statically initialized, and was large enough, the compiler attempted to put a pthread_mutex_lock around the call. This happened in such a way that we were in a recursive loop (intercepting the lock, trying to initialize the member with a lock, intercepting the lock) and it segfaulted.

It seems as if you stay in the __sanitizer special types, it prevents you from shooting yourself in the foot.

static Mutex radsan_init_mutex{};
static Flags radsan_flags{};

Flags *flags() { return &radsan_flags; }


SANITIZER_INTERFACE_WEAK_DEF(const char *, __radsan_default_options, void) {
return "";
}


static void RegisterRadsanFlags(FlagParser *parser, Flags *f) {
#define RADSAN_FLAG(Type, Name, DefaultValue, Description) \
RegisterFlag(parser, #Name, Description, &f->Name);
#include "radsan_flags.inc"
#undef RADSAN_FLAG
}

void Flags::SetDefaults() {
#define RADSAN_FLAG(Type, Name, DefaultValue, Description) Name = DefaultValue;
#include "radsan_flags.inc"
#undef RADSAN_FLAG
}


static void initializeFlags() {
SetCommonFlagsDefaults();
{
CommonFlags cf;
cf.CopyFrom(*common_flags());
cf.stack_trace_format = "DEFAULT";
cf.external_symbolizer_path = GetEnv("RADSAN_SYMBOLIZER_PATH");

Choose a reason for hiding this comment

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

Shall we move this into RADSAN_OPTIONS as well, for consistency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, we are in good company here! This one is a different env variable for each of the other sanitizers

> rg "external_symbolizer_path.*_SYMBOLIZER_PATH"
compiler-rt/lib/ubsan/ubsan_flags.cpp
53:    cf.external_symbolizer_path = GetFlag("UBSAN_SYMBOLIZER_PATH");

compiler-rt/lib/memprof/memprof_flags.cpp
52:    cf.external_symbolizer_path = GetEnv("MEMPROF_SYMBOLIZER_PATH");

compiler-rt/lib/asan/asan_flags.cpp
57:    cf.external_symbolizer_path = GetEnv("ASAN_SYMBOLIZER_PATH");

compiler-rt/lib/radsan/radsan.cpp
83:    cf.external_symbolizer_path = GetEnv("RADSAN_SYMBOLIZER_PATH");

compiler-rt/lib/tsan/rtl/tsan_flags.cpp
60:    cf.external_symbolizer_path = GetEnv("TSAN_SYMBOLIZER_PATH");

compiler-rt/lib/msan/msan.cpp
152:    cf.external_symbolizer_path = GetEnv("MSAN_SYMBOLIZER_PATH");

compiler-rt/lib/lsan/lsan.cpp
57:    cf.external_symbolizer_path = GetEnv("LSAN_SYMBOLIZER_PATH");

compiler-rt/lib/hwasan/hwasan.cpp
78:    cf.external_symbolizer_path = GetEnv("HWASAN_SYMBOLIZER_PATH");

OverrideCommonFlags(cf);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This information previously in another file can now be initialized only the once, cool!


Flags *f = flags();
f->SetDefaults();

FlagParser parser;
RegisterRadsanFlags(&parser, f);
RegisterCommonFlags(&parser);

// Override from user-specified string.
parser.ParseString(__radsan_default_options());

parser.ParseStringFromEnv("RADSAN_OPTIONS");

InitializeCommonFlags();

if (Verbosity()) ReportUnrecognizedFlags();

if (common_flags()->help)
{
parser.PrintFlagDescriptions();
}
}

} // namespace radsan

extern "C" {
RADSAN_EXPORT void radsan_init() { radsan::initialiseInterceptors(); }

RADSAN_EXPORT void radsan_realtime_enter() {
SANITIZER_INTERFACE_ATTRIBUTE void radsan_ensure_initialized() {
// Double-checked locking.
// Ensure that radsan_init() is called only once by the first thread
// that gets here.

if (radsan_is_initialized())
return;

Lock lock(&radsan::radsan_init_mutex);

if (radsan_is_initialized())
return;

radsan_init();
}

SANITIZER_INTERFACE_ATTRIBUTE bool radsan_is_initialized() {
return __sanitizer::atomic_load(&radsan::radsan_inited, memory_order_acquire) == 1;
}

SANITIZER_INTERFACE_ATTRIBUTE void radsan_init() {
using namespace radsan;

if (radsan_is_initialized()) return;

SanitizerToolName = "RealtimeSanitizer";

initializeFlags();
initialiseInterceptors();

__sanitizer::atomic_store(&radsan_inited, 1, memory_order_release);
}

SANITIZER_INTERFACE_ATTRIBUTE void radsan_realtime_enter() {
radsan::getContextForThisThread().realtimePush();
}

RADSAN_EXPORT void radsan_realtime_exit() {
SANITIZER_INTERFACE_ATTRIBUTE void radsan_realtime_exit() {
radsan::getContextForThisThread().realtimePop();
}

RADSAN_EXPORT void radsan_off() {
SANITIZER_INTERFACE_ATTRIBUTE void radsan_off() {
radsan::getContextForThisThread().bypassPush();
}

RADSAN_EXPORT void radsan_on() {
SANITIZER_INTERFACE_ATTRIBUTE void radsan_on() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a standard way to do what we were doing with RADSAN_EXPORT before, switched it to match the style

Choose a reason for hiding this comment

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

Nice!

radsan::getContextForThisThread().bypassPop();
}
}
35 changes: 25 additions & 10 deletions compiler-rt/lib/radsan/radsan.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,31 @@

#pragma once

#define RADSAN_EXPORT __attribute__((visibility("default")))
#include "sanitizer_common/sanitizer_internal_defs.h"

extern "C" {

/** Ensure RADSan is initialized.

This method ensures the first thread to call it will initialize RADSan.
Any subsequent calls will be no-ops.
*/
SANITIZER_INTERFACE_ATTRIBUTE void radsan_ensure_initialized();

/** Check if RADSan is initialized.

@return true if RADSan is initialized, false otherwise.
*/
SANITIZER_INTERFACE_ATTRIBUTE bool radsan_is_initialized();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made these methods "public". Let me know what you think about them being here


/**
Initialise radsan interceptors. A call to this method is added to the
preinit array on Linux systems.
Initialise radsan interceptors and other flags. A call to this method is added to the
preinit array on Linux systems. On non-linux systems this must be called explicitly
before other radsan methods.

@warning Do not call this method as a user.
@warning Do not call this method as a user. See radsan_ensure_initialized() instead.
*/
RADSAN_EXPORT void radsan_init();
SANITIZER_INTERFACE_ATTRIBUTE void radsan_init();

/** Enter real-time context.

Expand All @@ -28,7 +42,7 @@ RADSAN_EXPORT void radsan_init();

@warning Do not call this method as a user
*/
RADSAN_EXPORT void radsan_realtime_enter();
SANITIZER_INTERFACE_ATTRIBUTE void radsan_realtime_enter();

/** Exit the real-time context.

Expand All @@ -37,7 +51,7 @@ RADSAN_EXPORT void radsan_realtime_enter();

@warning Do not call this method as a user
*/
RADSAN_EXPORT void radsan_realtime_exit();
SANITIZER_INTERFACE_ATTRIBUTE void radsan_realtime_exit();

/** Disable all RADSan error reporting.

Expand All @@ -62,12 +76,13 @@ RADSAN_EXPORT void radsan_realtime_exit();
}

*/
RADSAN_EXPORT void radsan_off();
SANITIZER_INTERFACE_ATTRIBUTE void radsan_off();

/** Re-enable all RADSan error reporting.

The counterpart to `radsan_off`. See the description for `radsan_off` for
details about how to use this method.
*/
RADSAN_EXPORT void radsan_on();
}
SANITIZER_INTERFACE_ATTRIBUTE void radsan_on();

} // extern "C"
17 changes: 16 additions & 1 deletion compiler-rt/lib/radsan/radsan_context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
Subject to GNU General Public License (GPL) v3.0
*/

#include "radsan.h"
#include <radsan/radsan_context.h>

#include <radsan/radsan_stack.h>
Expand All @@ -25,10 +26,21 @@ static pthread_key_t key;
static pthread_once_t key_once = PTHREAD_ONCE_INIT;
void internalFree(void *ptr) { InternalFree(ptr); }

static __sanitizer::atomic_uint64_t radsan_report_count{};

void IncrementReportCount();
} // namespace detail

namespace radsan {

__sanitizer::u64 GetReportCount() {
return __sanitizer::atomic_load(&detail::radsan_report_count, memory_order_acquire);
}

void IncrementReportCount() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved these report methods into this file, as it's where they are used. It may make sense to make a radsan_report.cpp/.h, especially once we get an exit report if we do that with the continue mode.

__sanitizer::atomic_fetch_add(&detail::radsan_report_count, 1, memory_order_relaxed);
}

Context::Context() : Context(createErrorActionGetter()) {}

Context::Context(std::function<OnErrorAction()> get_error_action)
Expand All @@ -43,11 +55,12 @@ void Context::bypassPush() { bypass_depth_++; }
void Context::bypassPop() { bypass_depth_--; }

void Context::expectNotRealtime(const char *intercepted_function_name) {
CHECK(radsan_is_initialized());
if (inRealtimeContext() && !isBypassed()) {
bypassPush();
printDiagnostics(intercepted_function_name);
if (get_error_action_() == OnErrorAction::ExitWithFailure) {
exit(EXIT_FAILURE);
exit(common_flags()->exitcode);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cool! Configurable exitcode for free, just like the other sanitizers

Choose a reason for hiding this comment

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

I agree this is cool... but let's make sure we have a compelling use case for it before we take on the maintenance 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense! Happy to talk more about this on our eventual call

}
bypassPop();
}
Expand All @@ -62,6 +75,8 @@ void Context::printDiagnostics(const char *intercepted_function_name) {
"Real-time violation: intercepted call to real-time unsafe function "
"`%s` in real-time context! Stack trace:\n",
intercepted_function_name);

radsan::IncrementReportCount();
radsan::printStackTrace();
}

Expand Down
3 changes: 3 additions & 0 deletions compiler-rt/lib/radsan/radsan_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,14 @@
#pragma once

#include <radsan/radsan_user_interface.h>
#include "sanitizer_common/sanitizer_common.h"

#include <functional>

namespace radsan {

__sanitizer::u64 GetReportCount();

class Context {
public:
Context();
Expand Down
29 changes: 29 additions & 0 deletions compiler-rt/lib/radsan/radsan_flags.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
//===-- radsan_flags.h --------------------------------------------*- 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 RealtimeSanitizer.
//
//===----------------------------------------------------------------------===//
#ifndef RADSAN_FLAGS_H
#define RADSAN_FLAGS_H

namespace radsan {

struct Flags {
#define RADSAN_FLAG(Type, Name, DefaultValue, Description) Type Name;
#include "radsan_flags.inc"
#undef RADSAN_FLAG

void SetDefaults();
};

Flags *flags();

} // namespace radsan

#endif // RADSAN_FLAGS_H
19 changes: 19 additions & 0 deletions compiler-rt/lib/radsan/radsan_flags.inc
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
//===-- radsan_flags.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
//
//===----------------------------------------------------------------------===//
//
// RadSan runtime flags.
//
//===----------------------------------------------------------------------===//
#ifndef RADSAN_FLAG
# error "Define RADSAN_FLAG prior to including this file!"
#endif

// RADSAN_FLAG(Type, Name, DefaultValue, Description)
// See COMMON_FLAG in sanitizer_flags.inc for more details.

RADSAN_FLAG(const char*, error_mode, "exit", "Error mode: [continue|interactive|exit].")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Super easy to define new flags in this format, very impressed with the infrastructure around this.

3 changes: 3 additions & 0 deletions compiler-rt/lib/radsan/radsan_interceptors.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

#include "interception/interception.h"
#include "radsan/radsan_context.h"
#include "radsan/radsan.h"

#if !SANITIZER_LINUX && !SANITIZER_APPLE
#error Sorry, radsan does not yet support this platform
Expand All @@ -39,6 +40,8 @@ using namespace __sanitizer;

namespace radsan {
void expectNotRealtime(const char *intercepted_function_name) {
radsan_ensure_initialized();

getContextForThisThread().expectNotRealtime(intercepted_function_name);
}
} // namespace radsan
Expand Down
2 changes: 1 addition & 1 deletion compiler-rt/lib/radsan/radsan_preinit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@
// This code is linked into the main executable when -fsanitize=realtime is in
// the link flags. It can only use exported interface functions.

Choose a reason for hiding this comment

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

Does this definitely still work on the Linux image? The comment here suggests that the function radsan::radsan_init (which no longer has the RADSAN_EXPORT/SANITIZER_INTERFACE_ATTRIBUTE) can't be used - is the comment a mistake, maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great question. I have not run this on CI, so I'm not sure!

Perfect time to ask this question (and I'll do my own research too).

  1. What is the significance of extern C? What functions should be in a block that has that?
  2. What is the significance of the SANITIZER_INTERFACE_ATTRIBUTE and what functions should have it?

I was blindly pattern matching in radsan.h, and don't have a good mental model of these two things. It would be good for this review to be principled in those decisions as opposed to pattern matching. Wondering if you have any thoughts!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright, to put in some of my own discovered answers here (please add on or correct if I'm wrong). I'm typing this out mostly for my own education

  1. Extern C

Free functions in C++ cannot be automatically compiled and used in C++. The main reason for this is that C does NOT support function overloading.

To support function overloading in C++, the compiler adds a bunch of extra information to the name of the function to differentiate them from other overloads. This process is called "name mangling".

For instance, we the programmer may see:

void foo(int x, float y);
void foo(bool b);

The compiler/linker will see something else, for example (completely made up)

__fooIntFlo209ca
__fooBool29ri04

So, what do we do when we need to use a free function that is in our C++ code base in C? we need to make sure the name does not get mangled. This is where extern "C" comes in.

extern "C" ensures the code inside of it is compiled with the C linkage (instead of the C++ linkage) and ensures no mangling takes place. These functions can be called from either C or C++ with no issues.

The functions we should put in an extern C block are the functions that need to be called from C world.

FIXME IN THIS REVIEW: For example I think our init function MUST BE extern C, as it needs to be loaded into the preinit array.

This comment:

Does this definitely still work on the Linux image? The comment here suggests that the function radsan::radsan_init (which no longer has the RADSAN_EXPORT/SANITIZER_INTERFACE_ATTRIBUTE) can't be used - is the comment a mistake, maybe?

Seems to make sense now, and I would be surprised if this code ran as written on our linux box.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed this in the latest review, all methods called from linux land are linked in the C linkage instead of C++.

__attribute__((section(".preinit_array"), used))
void (*__local_radsan_preinit)(void) = radsan_init;
void (*__local_radsan_preinit)(void) = radsan::radsan_init;

#endif
12 changes: 0 additions & 12 deletions compiler-rt/lib/radsan/radsan_stack.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,6 @@ void BufferedStackTrace::UnwindImpl(uptr pc, uptr bp, void *context,
}
} // namespace __sanitizer

namespace {
void setGlobalStackTraceFormat() {
SetCommonFlagsDefaults();
CommonFlags cf;
cf.CopyFrom(*common_flags());
cf.stack_trace_format = "DEFAULT";
cf.external_symbolizer_path = GetEnv("RADSAN_SYMBOLIZER_PATH");
OverrideCommonFlags(cf);
}
} // namespace

namespace radsan {
void printStackTrace() {

Expand All @@ -44,7 +33,6 @@ void printStackTrace() {
GET_CURRENT_PC_BP;
stack.Unwind(pc, bp, nullptr, common_flags()->fast_unwind_on_fatal);

setGlobalStackTraceFormat();
stack.Print();
}
} // namespace radsan
Loading