Skip to content

Commit

Permalink
Catch writes to protected memory in mmapped arrays (fix #3434)
Browse files Browse the repository at this point in the history
  • Loading branch information
@timholy
timholy committed May 18, 2014
1 parent 2eef453 commit a339592
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 13 deletions.
62 changes: 49 additions & 13 deletions base/mmap.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,22 @@ function mmap_stream_settings(s::IO)
flags = MAP_SHARED
return prot, flags, (prot & PROT_WRITE) > 0
end
end # @unix_only

# Mmapped-array constructor
@windows_only begin
function msync(p::Ptr, len::Integer)
status = bool(ccall(:FlushViewOfFile, stdcall, Cint, (Ptr{Void}, Csize_t), p, len))
if !status
error("could not msync")
end
end
end


## Mmapped-array constructor ##
global mmap_array
let protected_buffers = Array(Uint, 0)
@unix_only begin
function mmap_array{T,N}(::Type{T}, dims::NTuple{N,Integer}, s::IO, offset::FileOffset; grow::Bool=true)
prot, flags, iswrite = mmap_stream_settings(s)
len = prod(dims)*sizeof(T)
Expand All @@ -110,15 +124,17 @@ function mmap_array{T,N}(::Type{T}, dims::NTuple{N,Integer}, s::IO, offset::File
pmap, delta = mmap(len, prot, flags, fd(s), offset)
end
A = pointer_to_array(pointer(T, uint(pmap)+delta), dims)
finalizer(A,x->munmap(pmap,len+delta))
if !iswrite
set_protected(A)
finalizer(A,x->(munmap(pmap,len+delta); clear_protected(A)))
else
finalizer(A,x->munmap(pmap,len+delta))
end
return A
end
end # @unix_only

end

### Windows implementation ###
@windows_only begin
# Mmapped-array constructor
function mmap_array{T,N}(::Type{T}, dims::NTuple{N,Integer}, s::IO, offset::FileOffset)
shandle = _get_osfhandle(RawFD(fd(s)))
if int(shandle.handle) == -1
Expand All @@ -145,7 +161,12 @@ function mmap_array{T,N}(::Type{T}, dims::NTuple{N,Integer}, s::IO, offset::File
error("could not create mapping view")
end
A = pointer_to_array(pointer(T, viewhandle), dims)
finalizer(A, x->munmap(viewhandle, mmaphandle))
if ro
set_protected(A)
finalizer(A,x->(munmap(viewhandle, mmaphandle); clear_protected(A)))
else
finalizer(A, x->munmap(viewhandle, mmaphandle))
end
return A
end

Expand All @@ -156,15 +177,30 @@ function munmap(viewhandle::Ptr, mmaphandle::Ptr)
error("could not unmap view")
end
end

function msync(p::Ptr, len::Integer)
status = bool(ccall(:FlushViewOfFile, stdcall, Cint, (Ptr{Void}, Csize_t), p, len))
if !status
error("could not msync")
end
end # @windows_only

function set_protected(A::Array)
Astart = pointer(A)
Aend = Astart + length(A)*sizeof(eltype(A))
push!(protected_buffers, Astart)
push!(protected_buffers, Aend)
ccall(:jl_update_protected_buffers, Void, (Ptr{Void}, Cint),
protected_buffers, length(protected_buffers)>>1)
end

function clear_protected(A::Array)
Astart = uint(pointer(A))
for i = 1:2:length(protected_buffers)
if protected_buffers[i] == Astart
deleteat!(protected_buffers, i:i+1)
ccall(:jl_update_protected_buffers, Void, (Ptr{Void}, Cint),
protected_buffers, length(protected_buffers)>>1)
return
end
end
warn("Array not found in protected list")
end
end # let protected_buffers

# Mmapped-bitarray constructor
function mmap_bitarray{N}(dims::NTuple{N,Integer}, s::IOStream, offset::FileOffset)
Expand Down
24 changes: 24 additions & 0 deletions src/init.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,24 @@ static int is_addr_on_stack(void *addr)
#endif
}

void **protected_buffers;
int n_protected_buffers = 0;

static int is_addr_protected(void *addr)
{
for(int i = 0; i < 2*n_protected_buffers; i += 2) {
if (addr >= protected_buffers[i] && addr < protected_buffers[i+1])
return 1;
}
return 0;
}

DLLEXPORT void jl_update_protected_buffers(void *buf, int n)
{
protected_buffers = (void**)buf;
n_protected_buffers = n;
}

#if defined(__linux__) || defined(__FreeBSD__)
extern int in_jl_;
void segv_handler(int sig, siginfo_t *info, void *context)
Expand All @@ -146,6 +164,12 @@ void segv_handler(int sig, siginfo_t *info, void *context)
sigprocmask(SIG_UNBLOCK, &sset, NULL);
jl_throw(jl_stackovf_exception);
}
else if (is_addr_protected(info->si_addr)) {
sigemptyset(&sset);
sigaddset(&sset, SIGSEGV);
sigprocmask(SIG_UNBLOCK, &sset, NULL);
jl_throw(jl_memory_exception);
}
else {
uv_tty_reset_mode();
sigfillset(&sset);
Expand Down

10 comments on commit a339592

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

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

This is really cool if you can make it work nicely!

@timholy
Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I think it already works nicely 😄.

I should point out that I went to more effort than is perhaps necessary in a perverse attempt to allow segfaults to happen in certain situations. For example, unsafe_store!(convert(Ptr{Int}, 0), 3, 1) still segfaults after this patch, because 0 is not going to be within a span of memory addresses corresponding to a mmapped file. Likewise, if you do this:

function f(A)
    @inbounds A[11] = 5
end
function g(A)
    @inbounds A[7] = 5
end

then when A is a read-only block of 10 elements, g will result in a MemoryError but f will segfault. This happens because A[11] is not within the "trapped" block of memory (because it is not within the bounds of the array), and using @inbounds circumvents the BoundsError that would normally result, so you end up getting a segfault. It would of course be a slightly shorter patch if we just threw a MemoryError for anything that would have resulted in a segfault---I had to go to extra effort to allow the segfault to happen---but for better or for worse I decided to make this minimalistic in terms of the way in which it changes julia's behavior. If you prefer the bigger change of trapping SIGSEGV under all circumstances, do let me know.

@Keno
Copy link
Member

@Keno Keno commented on a339592 May 19, 2014

Choose a reason for hiding this comment

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

I actually considered making all Segfaults throw back when I implemented proper stack overflow support for Mac, but it was considered to easy to catch, so we took it out again.

@Keno
Copy link
Member

@Keno Keno commented on a339592 May 19, 2014

Choose a reason for hiding this comment

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

Also, would it make more sense to just look for SEGV_ACCERR rather than dealing with protected regions?

@timholy
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 should have looked more closely at what goodies siginfo_t provides. I like it.

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

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

If segfaults where uncatchable errors (see #6845) and used a signal handling interface instead, then it would be much more reasonable for segfaults to produce errors rather than terminating the process.

@ivarne
Copy link
Member

@ivarne ivarne commented on a339592 May 19, 2014

Choose a reason for hiding this comment

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

I always find myself writing type assertions when catching exceptions.

try
    sqrt(-1)
catch a::DomainError
    # Tell the user to use complex numbers if he wants a complex answer.
end

If that syntax was allowed, we could have catch without assertion only catch subtypes of abstract Exception. InterruptException and SegmentationFault would not inherit from abstract Exception, and thus you would need to specify them explicitly if you wanted to catch them.

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

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

I should have realized that SIGINT and SIGSEGV are fundamentally different – SIGSEGV indicates that the code that's running made an error and cannot continue correctly. SIGINT has no such implication – it's a purely external signal.

@timholy
Copy link
Member Author

Choose a reason for hiding this comment

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

If the error is uncatchable, wouldn't that cover the fact that the code simply can't complete?

Of course, the more serious concern is that your generic SIGSEGV might indicate that julia is in a corrupted state, and under those circumstances you probably wouldn't want to try to continue. We seem to have at least some protections against this; for example, using this branch let's try to trash the method table for sum:

julia> p = convert(Ptr{Int}, sum.fptr)
Ptr{Int64} @0x00007fea14895ea0

julia> unsafe_store!(p, 0, 1)
ERROR: MemoryError()
 in unsafe_store! at pointer.jl:50

So we weren't able to trash that memory location, and since it didn't succeed we don't actually have to worry about julia being corrupt.

However, the compiler (among others) surely could corrupt a method; if that happened for a method needed by the REPL, trying to cope with that error without crashing julia sounds like a dicey proposition. I fear we could pretty quickly get into forced termination/ctrl-alt-delete territory. I'm a little worried that this less "picky" version of this patch (which does away with the explicit check that this is just issue #3434) might already move us in that direction.

@timholy
Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, since this is a branch comment thread and isn't linked with the original PR, I should have clarified what I meant by the "less picky" version of this patch.

Please sign in to comment.