Skip to content

Commit

Permalink
[ASAN] Turn off asan instrumentation in segv handler (#46377)
Browse files Browse the repository at this point in the history 
There's some issues around asan and segv handlers. The issues
I'm aware of is:

1. If things run on the sigaltstack, we need to at some point unpoison
   the sigaltstack and it's not clear when to do that.
2. Sanitizers accidentally free stil-in-use stack frames:
   google/sanitizers#1561
3. If noreturn functions run on the sigaltstack, asan can get confused
   about what it needs to unpoison

So for now, remove asan instrumentation from the segv_handler and
the the jl_call_in_ctx functions (jl_sig_throw is already annotated).
This helps the case I was seeing (where gc_srub would cause frequent
segv_handler invocations for safe restore), but there's probably a few
other landmines remaining here.
  • Loading branch information
@Keno
Keno authored Aug 17, 2022
1 parent 0ada892 commit 4af9674
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions src/signals-unix.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ static int is_addr_on_sigstack(jl_ptls_t ptls, void *ptr)
// returns. `fptr` will execute on the signal stack, and must not return.
// jl_call_in_ctx is also currently executing on that signal stack,
// so be careful not to smash it
static void jl_call_in_ctx(jl_ptls_t ptls, void (*fptr)(void), int sig, void *_ctx)
JL_NO_ASAN static void jl_call_in_ctx(jl_ptls_t ptls, void (*fptr)(void), int sig, void *_ctx)
{
// Modifying the ucontext should work but there is concern that
// sigreturn oriented programming mitigation can work against us
Expand Down Expand Up @@ -310,7 +310,7 @@ static int jl_is_on_sigstack(jl_ptls_t ptls, void *ptr, void *context)
is_addr_on_sigstack(ptls, (void*)jl_get_rsp_from_ctx(context)));
}

static void segv_handler(int sig, siginfo_t *info, void *context)
JL_NO_ASAN static void segv_handler(int sig, siginfo_t *info, void *context)
{
if (jl_get_safe_restore()) { // restarting jl_ or profile
jl_call_in_ctx(NULL, &jl_sig_throw, sig, context);
Expand Down

6 comments on commit 4af9674

@maleadt
Copy link
Member

Choose a reason for hiding this comment

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

Some more PkgEval development, now with support for customizing the build command and e.g. running an ASAN build:

@nanosoldier runtests(["Example"], buildcommands="contrib/asan/build.sh . install", julia_binary="julia-debug")

@maleadt
Copy link
Member

Choose a reason for hiding this comment

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

Ah, botched the invocation:

@nanosoldier runtests(["Example"], configuration = (buildcommands="contrib/asan/build.sh . install", julia_binary="julia-debug"))

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your package evaluation job has completed - no issues were detected. A full report can be found here.

@maleadt
Copy link
Member

Choose a reason for hiding this comment

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

Ouch...

################################################################################
# Installation
#

Started at 2022-08-18T14:15:25.140

   Resolving package versions...
    Updating `~/.julia/environments/v1.9/Project.toml`
  [7876af07] + Example v0.5.3
    Updating `~/.julia/environments/v1.9/Manifest.toml`
  [7876af07] + Example v0.5.3

Completed after 521.8s

I don't remember ASAN builds being that slow. A result of the generated code being sanitized now, @Keno?

@Keno
Copy link
Member Author

@Keno Keno commented on 4af9674 Aug 18, 2022

Choose a reason for hiding this comment

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

Yes

@maleadt
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to introduce a flag to disable the sanitizer passes so that it is viable to run PkgEval with ASAN? I would guess that memory errors are much more likely in our runtime than they are in generated code.

Please sign in to comment.