Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alter FailFast behaviour of memcpy #526

Merged
merged 5 commits into from
May 19, 2022
Merged
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
103 changes: 48 additions & 55 deletions src/snmalloc/global/bounds_checks.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,16 @@ namespace snmalloc
* error. With it set to false we print a helpful error message and then crash
* the process. The process may be in an undefined state by the time the
* check fails, so there are potentially security implications to turning this
* off. It defaults to true for debug builds, false for release builds and
* can be overridden by defining the macro `SNMALLOC_FAIL_FAST` to true or
* false.
* off. It defaults to false and can be overridden by defining the macro
* `SNMALLOC_FAIL_FAST` to true.
*
* Current default to true will help with adoption experience.
*/
static constexpr bool FailFast =
#ifdef SNMALLOC_FAIL_FAST
SNMALLOC_FAIL_FAST
#else
!DEBUG
false
#endif
;

Expand All @@ -40,71 +41,63 @@ namespace snmalloc
* `p` is the input pointer and `len` is the offset from this pointer of the
* bounds. `msg` is the message that will be reported along with the
* start and end of the real object's bounds.
*
* Note that this function never returns. We do not mark it [[NoReturn]]
* so as to generate better code, because [[NoReturn]] prevents tailcails
* in GCC and Clang.
*
* The function claims to return a FakeReturn, this is so it can be tail
* called where the bound checked function returns a value, for instance, in
* memcpy it is specialised to void*.
*/
SNMALLOC_SLOW_PATH SNMALLOC_UNUSED_FUNCTION inline void
report_fatal_bounds_error [[noreturn]] (
void* p, size_t len, const char* msg, decltype(ThreadAlloc::get())& alloc)
{
report_fatal_error(
"{}: {} is in allocation {}--{}, offset {} is past the end\n",
msg,
p,
alloc.template external_pointer<Start>(p),
alloc.template external_pointer<OnePastEnd>(p),
len);
}

/**
* The direction for a bounds check.
*/
enum class CheckDirection
template<typename FakeReturn = void*>
SNMALLOC_SLOW_PATH SNMALLOC_UNUSED_FUNCTION inline FakeReturn
report_fatal_bounds_error(const void* ptr, size_t len, const char* msg)
{
/**
* A read bounds check, performed only when read checks are enabled.
*/
Read,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like that this has gone away because I was using it in other functions. The goal for this header was to provide a set of tools that could be used for wrappers around other libc functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have moved the if constexpr back inside, so you can now do

check_bounds<CheckReads>(...)

Is this acceptable? Could you elaborate on where you feel the CheckDirection approach would be clearer?
It seems you have to pass two parameters

check_bounds<CheckDirection::Read, CheckReads>(...)

I have updated the comments and error message changes to not be memcpy specific, and have added a FakeReturn, so that different things could be tail called in case the function needs a different return type.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The check needs two pieces of information:

  • Is this a read or a write?
  • Are we checking reads or writes?

I'm not sure how you encode that in a single template parameter.

I am using ifunc resolvers to instantiate the outer functions multiple times so that I get two versions:

  • No checks
  • Checks on writes
  • Checks on reads and writes

If it's a wrapper, then I use the underlying function for the no-checks version. I can then switch between them based on an environment variable at process-launch time. As long as I can still do that concisely, I'm happy.

Making check_bounds take two arguments meant that I didn't need any if constexpr in the outer function, which simplifies my wrappers considerably because they just unconditionally forward their arguments. My ifunc resolver is, itself, generated from a templated function that takes a template template parameter and instantiates it with the wrapper version to generate (read or write checks).

Most of the libc function wrappers can be a simple template that calls check_bounds<> on any read/write pointer arguments and then forwards to the underlying implementation.

Copy link
Member Author

@mjp41 mjp41 May 19, 2022

Choose a reason for hiding this comment

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

So with a single template parameter I can do the following, which has no if constexpr in the caller.

    // Check the bounds of the arguments.
    if (SNMALLOC_UNLIKELY(!check_bounds<Checked && ReadsChecked>(src, len)))
      return report_fatal_bounds_error(
        src, len, "memcpy with source out of bounds of heap allocation");
    if (SNMALLOC_UNLIKELY(!check_bounds<Checked>(dst, len)))
      return report_fatal_bounds_error(
        dst, len, "memcpy with destination out of bounds of heap allocation");

I can push the SNMALLOC_UNLIKELY inside the call, but it doesn't seem to bias the branches correctly.

It has to be two calls to get the good codegen, when we have error messages as we need to conditionally tail call the error message.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough. I can always wrap the two calls in a macro.

Copy link
Member Author

Choose a reason for hiding this comment

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

So a macro would work, but you will probably need two versions one for a void returning function, and another that takes a type parameter for a not void function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

One of my favourite small parts of C++ is that you can return an expression that evaluates to a void type from a function with a void return, so this should be possible to make generic.

if constexpr (FailFast)
{
UNUSED(ptr, len, msg);
SNMALLOC_FAST_FAIL();
}
else
{
auto& alloc = ThreadAlloc::get();
void* p = const_cast<void*>(ptr);

/**
* A write bounds check, performed unconditionally.
*/
Write
};
auto range_end = pointer_offset(p, len);
auto object_end = alloc.template external_pointer<OnePastEnd>(p);
report_fatal_error(
"Fatal Error!\n{}: \n\trange [{}, {})\n\tallocation [{}, "
"{})\nrange goes beyond allocation by {} bytes \n",
msg,
p,
range_end,
alloc.template external_pointer<Start>(p),
object_end,
pointer_diff(object_end, range_end));
}
}

/**
* Check whether a pointer + length is in the same object as the pointer.
* Fail with the error message from the third argument if not.
*
* The template parameter indicates whether this is a read. If so, this
* function is a no-op when `CheckReads` is false.
* Returns true if the checks succeeds.
*
* The template parameter indicates whether the check should be performed. It
* defaults to true. If it is false, the check will always succeed.
*/
template<
CheckDirection Direction = CheckDirection::Write,
bool CheckBoth = CheckReads>
SNMALLOC_FAST_PATH_INLINE void
check_bounds(const void* ptr, size_t len, const char* msg = "")
template<bool PerformCheck = true>
SNMALLOC_FAST_PATH_INLINE bool check_bounds(const void* ptr, size_t len)
{
if constexpr ((Direction == CheckDirection::Write) || CheckBoth)
if constexpr (PerformCheck)
{
auto& alloc = ThreadAlloc::get();
void* p = const_cast<void*>(ptr);

if (SNMALLOC_UNLIKELY(!alloc.check_bounds(ptr, len)))
{
if constexpr (FailFast)
{
UNUSED(p, len, msg);
SNMALLOC_FAST_FAIL();
}
else
{
report_fatal_bounds_error(p, len, msg, alloc);
}
}
return alloc.check_bounds(ptr, len);
}
else
{
UNUSED(ptr, len, msg);
UNUSED(ptr, len);
return true;
}
}

} // namespace snmalloc
13 changes: 6 additions & 7 deletions src/snmalloc/global/memcpy.h
Original file line number Diff line number Diff line change
Expand Up @@ -317,14 +317,13 @@ namespace snmalloc
return dst;
}

if constexpr (Checked)
{
// Check the bounds of the arguments.
check_bounds(
dst, len, "memcpy with destination out of bounds of heap allocation");
check_bounds<CheckDirection::Read, CheckReads>(
// Check the bounds of the arguments.
if (SNMALLOC_UNLIKELY(!check_bounds<(Checked && ReadsChecked)>(src, len)))
return report_fatal_bounds_error(
src, len, "memcpy with source out of bounds of heap allocation");
}
if (SNMALLOC_UNLIKELY(!check_bounds<Checked>(dst, len)))
return report_fatal_bounds_error(
dst, len, "memcpy with destination out of bounds of heap allocation");

Arch::copy(dst, src, len);
return orig_dst;
Expand Down
7 changes: 6 additions & 1 deletion src/test/perf/memcpy/memcpy.cc
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,12 @@ void memcpy_unchecked(void* dst, const void* src, size_t size)
NOINLINE
void memcpy_platform_checked(void* dst, const void* src, size_t size)
{
check_bounds(dst, size, "");
if (SNMALLOC_UNLIKELY(!check_bounds(dst, size)))
{
report_fatal_bounds_error(dst, size, "");
return;
}

memcpy(dst, src, size);
}

Expand Down