mmap_array segfaults

[quinnj]

While battle-testing #11280, I discovered the following for Windows:

s = open(file, "r") # open file as read-only with contents "Hello World\n"
m = Mmap.Array(s) # same as previous mmap_array()
m[5] = UInt8('x')
# segfault

On OSX, I get an OutOfMemoryError(), but I don't think that's very informative.

I'm not entirely sure what to do here. Restrict Mmap.Array to file's where we have read & write permissions? Is that overly restrictive? Put a warning in the docs to be careful with read-only files? Rethink the Mmap.Array interface entirely?

[ScottPJones]

Does this also happen with the current mmap code?
Please don't restrict it to files with read & write permission!

[quinnj]

Yes, this is current behavior on 0.3/0.4.

[quinnj]

Here's another fun Windows mmap_array segfault:

f = open("C:/Users/karbarcca/test.jl", "r")
r = mmap_array(Uint8, (10,), f, -1)
# segfault

By trying to do a negative offset. (Yay for more testing!)

[ScottPJones]

Ugh... Is m[5] = x done by setindex!(m,5,x) ? Could you simply have a readonly flag that gives an error for that?

[quinnj]

I think this is one of the design flaws of mmap_array and allowing the A = pointer_to_array(ptr, dims) where ptr is our pointer to the mmapped region. It's certainly easy to add a iswritable flag to my new Mmap.Stream type (which I'm finding is extremely similar to an IOBuffer), but for Arrays, I'm not sure what we can do other than yell at people in docs.

[ihnorton]

(related:
https://groups.google.com/d/msg/julia-users/_OFCVa3HAZQ/sF5VSeC6rIsJ)

On Tue, May 19, 2015 at 1:51 PM, Jacob Quinn notifications@github.com
wrote:

I think this is one of the design flaws of mmap_array and allowing the A
= pointer_to_array(ptr, dims) where ptr is our pointer to the mmapped
region. It's certainly easy to add a iswritable flag to my new Mmap.Stream
type (which I'm finding is extremely similar to an IOBuffer), but for
Arrays, I'm not sure what we can do other than yell at people in docs.

—
Reply to this email directly or view it on GitHub
#11351 (comment).

[simonster]

See also #3434 and #6703.

[timholy]

#6877, see in particular the last sentences of my introductory post.

[simonster]

Also, I suppose this is a case where #10503 made things less clear.

[quinnj]

So I know this is bleeding into #11280, but what do people think about getting rid of mmap_array entirely and just having Mmap.Stream? It would basically function as currently outlined in my PR, yet we could easily add an iswriteable field to the type and define getindex/setindex! so that it becomes a pseudo-array/Stream type (which is what it pretty much is).

[simonster]

My workflow often involves creating objects that have array-typed fields, saving them to disk, and then reading the objects back in using JLD's mmaparrays option, which uses mmap_array to read the underlying arrays. Because JLD constructs instances of the same types with mmapped arrays in place of heap-allocated arrays, they really do need to be Arrays. But I could always move the existing mmap_array code to JLD.

However, if we want a separate type, I'm not sure if it should be Mmap.Stream or something new. We don't have multiple inheritance, so we'd have to pick between Mmap.Stream <: IO and Mmap.Stream <: AbstractArray.

[quinnj]

Yeah, I was just looking through rewriting without mmap_array and realized the multiple inheritance problem as well. My thought would be to have Mmap.Stream <: IO with a convert(::Type{Array}, x::Stream) method that essentially performs the last line of mmap_array currently. Mainly because I think it's easier to subtype IO than AbstractArray.

[timholy]

From a certain perspective it's just a block of memory; it's a detail that it happens to be backed by a disk file. Seems much more array-like than IO-like to me.

[quinnj]

Yes, the more I think about it, the more it makes sense to think about it like an Array, but a special Array that has to deal with file permissions 😨 Alright, I think I'll try to make Mmap.Stream <: AbstractArray; who can point me to what it takes to be a 1st class AbstractArray around here?