Skip to content

Darwin/ARM64: Fix throwing of synchronous memory exceptions #36625

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.

Sign up for GitHub

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

Closed
Keno wants to merge 1 commit into from
Closed

Darwin/ARM64: Fix throwing of synchronous memory exceptions #36625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 14 additions & 1 deletion src/signals-mach.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,12 @@ typedef arm_exception_state64_t host_exception_state_t;
#define THREAD_STATE_COUNT ARM_THREAD_STATE64_COUNT
#define HOST_EXCEPTION_STATE ARM_EXCEPTION_STATE64
#define HOST_EXCEPTION_STATE_COUNT ARM_EXCEPTION_STATE64_COUNT

enum aarch64_esr_layout {
EC_MASK = ((uint32_t)0b111111) << 25,
EC_DATA_ABORT = ((uint32_t)0b100100) << 25,
ISR_DA_WnR = ((uint32_t)1) << 6
};
#endif

static void jl_call_in_state(jl_ptls_t ptls2, host_thread_state_t *state,
Expand All @@ -141,11 +147,11 @@ static void jl_call_in_state(jl_ptls_t ptls2, host_thread_state_t *state,
uint64_t rsp = (uint64_t)ptls2->signal_stack + sig_stack_size;
assert(rsp % 16 == 0);

#ifdef _CPU_X86_64_
// push (null) $RIP onto the stack
rsp -= sizeof(void*);
*(void**)rsp = NULL;
Copy link
Contributor

@yuyichao yuyichao Jul 12, 2020

Choose a reason for hiding this comment

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

The corresponding aarch64 operation is setting lr to zero.

julia/src/signals-unix.c

Line 135 in a23a4ff

ctx->uc_mcontext.regs[29] = 0; // Clear link register (x29)

Copy link
Contributor

@yuyichao yuyichao Jul 12, 2020

Choose a reason for hiding this comment

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

And is

julia/src/signals-unix.c

Line 162 in a23a4ff

ucontext64_t *ctx = (ucontext64_t*)_ctx;
used?

Copy link
Member Author

@Keno Keno Jul 12, 2020

Choose a reason for hiding this comment

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

Right, I was wondering if we should set lr to NULL, but I haven't looked at the unwinder yet. I guess it can't really hurt.

Copy link
Contributor

@yuyichao yuyichao Jul 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah it shoudn't hurt. And I think at least in one case I've seen it helps the unwinder (either ours or gdb) terminates the stack trace. (on linux...)


#ifdef _CPU_X86_64_
state->__rsp = rsp; // set stack pointer
state->__rip = (uint64_t)fptr; // "call" the function
#else
Expand Down Expand Up @@ -253,8 +259,15 @@ kern_return_t catch_exception_raise(mach_port_t exception_port,
}
#endif
else {
#ifdef _CPU_X86_64_
if (!(exc_state.__err & WRITE_FAULT))
return KERN_INVALID_ARGUMENT; // rethrow the SEGV since it wasn't an error with writing to read-only memory
#else
uint32_t esr = exc_state.__esr;
if ((esr & EC_MASK) != EC_DATA_ABORT || !(esr & ISR_DA_WnR)) {
return KERN_INVALID_ARGUMENT;
}
#endif
excpt = jl_readonlymemory_exception;
}
jl_throw_in_thread(tid, thread, excpt);
Expand Down
12 changes: 12 additions & 0 deletions test/misc.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,3 +842,15 @@ end
@testset "fieldtypes Module" begin
@test fieldtypes(Module) isa Tuple
end

# Test that read fault on a prot-none region does not incorrectly give
# ReadOnlyMemoryEror, but rather crashes the program
const MAP_ANONYMOUS_PRIVATE = Sys.isbsd() ? 0x1002 : 0x22
let script = :(let ptr = Ptr{Cint}(ccall(:jl_mmap, Ptr{Cvoid},
Copy link
Contributor

@yuyichao yuyichao Jul 12, 2020

Choose a reason for hiding this comment

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

Do we actually guarantee this everywhere else?

Copy link
Member Author

@Keno Keno Jul 12, 2020

Choose a reason for hiding this comment

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

Yes, I think we have this logic on every platform.

Copy link
Contributor

@yuyichao yuyichao Jul 12, 2020

Choose a reason for hiding this comment

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

It throws readonlymmoryerror on linux for example. I don't really see why it's that important to differentiate reading PROT_NONE from writing PROT_READ. The linux one only check for permission violation and would tread violation of read/write/execution the same.

Copy link
Contributor

@yuyichao yuyichao Jul 12, 2020

Choose a reason for hiding this comment

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

I imaging what you could do is to unmap the page before issueing the unsafe_load. Assuming the code is compiled it should make sure you are loading from a hole in the mapping and that should be a crash.

Copy link
Member Author

@Keno Keno Jul 12, 2020

Choose a reason for hiding this comment

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

I imaging what you could do is to unmap the page before issueing the unsafe_load. Assuming the code is compiled it should make sure you are loading from a hole in the mapping and that should be a crash.

That doesn't exercise this code path. We explicitly check that the mapping exists first.

Copy link
Member Author

@Keno Keno Jul 12, 2020 •
edited by yuyichao
Loading

Choose a reason for hiding this comment

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

I think we may just want to fix the linux behavior here to bring it in line with mac/windows. I don't see a good reason to throw ReadOnlyMemoryError here.

Copy link
Contributor

@yuyichao yuyichao Jul 12, 2020

Choose a reason for hiding this comment

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

... Sure, that's fine. It's technically breaking but it was an error before anyway...

Copy link
Member Author

@Keno Keno Jul 12, 2020

Choose a reason for hiding this comment

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

Or it's a bug fix and brings linux in line with the other platforms ;).

Copy link
Contributor

@yuyichao yuyichao Jul 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

"Breaking" for ppl (like me) who hardly ever use julia anywhere other than linux... Not that I've ever cought any read only memory error when reading things anyway.............

Copy link
Contributor

@yuyichao yuyichao Jul 12, 2020

Choose a reason for hiding this comment

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

I do agree the name of the exception is very suggestive of what it should do...

(Ptr{Cvoid}, Csize_t, Cint, Cint, Cint, Int),
C_NULL, 16*1024, 0, $MAP_ANONYMOUS_PRIVATE, -1, 0)); try
unsafe_load(ptr)
catch e; println(e) end; end)
@test !success(`$(Base.julia_cmd()) -e $script`)
end