segfault when "viewing" closed mmap object

This crashes in python2 and python3 with a segfault:

import mmap

# create test file
a = arange(10., dtype='f4')
a.tofile('test')

# opens it as a memmap
with open('test', 'r+b') as f:
      mm = mmap.mmap(f.fileno(),0)

#view the mmap, then close it
x = np.frombuffer(mm, dtype='f4')
mm.close()
print(x)  # segfault

In frombuffer mumpy uses the PEP3118 interface to the mmap to get a pointer to the data, so it seems we are using the interface incorrectly somewhere.

It seems to me that the "correct" behavior may be for it to be impossible to close the memmap while pointers to it exist; this is the behavior for memoryviews of mmaps. That is, the line mm.close() shoud raise an error BufferError: cannot close exported pointers, as in currently happens in python3:

>>> import mmap
>>> with open('test', 'r+b') as f:
>>>     mm = mmap.mmap(f.fileno(),0)
>>> mv = memoryview(mm)
>>> mm.close()
BufferError: cannot close exported pointers exist

however note also that in python2 we cannot create the memoryview (mv) because of an error TypeError: cannot make memory view because object does not have the buffer interface.

Note how mmap_buffer_getbuf (link) in the mmap module takes care to keep track of any buffer access to the mmap (the exports counter). Evidently we are somehow bypassing that in numpy.

This was reported on the mailing list in this message

The bug was not found by me, but by Maurizio Monge maurizio.monge at im.ufrj.br; I just used it to check the behaviour of numpy in a specific case.

Maybe it ends up using the legacy buffer API, which iirc has no such safety guarantees?

There were some fixes for buffers recently in PR ( #10740 ) included in NumPy 1.15.0. Does this issue persist with those changes?

No errors on python 3 for me in 1.17.x

Does segfault for me, maybe windows is different? I somewhat thought I saw a warning/error here on closing the memmap even though it still has references.

My mistake, failed to copy the print line when I tested this. Fails on windows for me.

I've run into similar issues when trying to use np.frombuffer against our own objects that implement the buffer protocol. I think the issue is that numpy mis-uses the buffer api - it gets and immediately releases the buffer instead of releasing the buffer during the the returned arrays destructor.

https://github.com/numpy/numpy/blob/v1.18.3/numpy/core/src/multiarray/ctors.c#L3817-L3823

To be clear, this comes from a time when the code would have been perfectly correct, because view->obj was always defined as the exporting object, and PyBuffer_Release was not able to do anything more than decreasing the refcount of obj.

NumPy itself doesn't even use the "free" function right now, because it leads to annoying side effects with the PyArg_Parse* functions.

So, as long as the data pointed to by your object remains available for the lifetime of that object, you are actually fine. Otherwise, it seems we would have to do the internal reference counting dance that memoryview has to do. I do not know how tricky that is, but I am not sure I am willing to do that. So the only thing I can think of is: Wrap all "buffers" directly in memoryviews. I wonder if that would be better to begin with... if holding on to the buffer is important for the memory to remain valid, why not promote it to a python object, they already do memory management?

But of course, in NumPy we thus use sys.refcount in one or two places, to warn users that they are doing invalid things. With mmap+memoryview, you actually get a real warning also on PyPy, since it does its own reference counting...

Long story short, I did not realize that memoryviews had this ingrained so much, and mmap actually uses it semi-prominently when I opened https://bugs.python.org/issue39471 to at least clarify things. I am not sure we should even consider doing that internal refcounting dance... But luckily, we now have only python 3, so we could just wrap everything in a memoryview and use that. Of course after that we may have to brace for impact of users expecting arr.base not to be a memoryview suddenly?

Thanks for chiming in - I certainly don't know the intricate implementation details of numpy, but I think the assumption that "the object remains available for the lifetime of that object" might be a bad assumption. At the very least, it needs to be warned and documented.

For example, this seems like it should be valid:

def to_numpy(x: SomeCustomInt32BufferProtocol):
  return np.frombuffer(x, dtype=np.int32)

y = to_numpy(SomeCustomInt32BufferProtocol(...))

Due to the current implementation of np.frombuffer though, there is no way for the custom impl to know that anything is still referencing its underlying buffer.

I think due to the inherent nature of the buffer protocol, libraries/consumers that call PyObject_GetBuffer are expected to manage their calls back into PyBuffer_Release with care.

@seberg

@seberg, I'm afraid I never quite followed the conclusion of your one-sided discussion on that issue. Was it ever clear what the intended semantics were?

I might be mis-interpreting your issue https://bugs.python.org/issue39471, but it seems like the testbuffer impl doesn't allocate during the getbufferproc stage:

https://github.com/python/cpython/blob/v3.8.2/Modules/_testbuffer.c#L1527-L1529

That said, I think it's completely legitimate for getbufferproc to allocate instead of returning "views" - it should be an implementation detail that shouldn't matter to a generic consumer (the generic consumer in this case being np.frombuffer). Of course, as a specific consumer, using a specific buffer protocol library, I should be aware about the expectations of the implementation. IE, mmap and np.array implement the buffer protocol with efficient "view" semantics.

As a hypothetical - I could implement a getbufferproc that returns new random bytes for each call.

The only assumption that it has, is that buffer->obj is going to be the exporting object and not some new object created during the export. I think that is fine, if you want to be pedantic, I guess we could check that.
Of course the only reason to do that is to safe a few cpu cycles creating a memorymap and retain the arr.base attribute. So it may not be worth the trouble.
CPython currently uses (has to use) that assumption also during argument parsing to retain backward compatibility (the reason why NumPy does not use releasebuffer, which is seriously annoying).

CPython currently uses (has to use) that assumption also during argument parsing to retain backward compatibility

In what way is that a compatibility choice? Presumably the issue is that ParseTuple... and friends do not have a code that means "get me a pointer and and object to manage its lifetime with".

@skrah

Either way - it's probably good to get some input from a cpython buffer protocol maintainer.

@skrah and @abalkin might be your best bet there, at least based on the reviewers assigned to python/cpython#5576 and python/cpython#5561

Because PyArg_ParseTuple("%z") (or whatever), can keep working with NumPy arrays because NumPy arrays do not implement releasebuffer and thus it can assume that the pointer is valid for the whole lifetime of the exporting object. So I think it is "compatibility" with the old buffer-protocol where the lifetime assumption was correct. And the reason why NumPy is stuck with it currently.

The compatibility choice is ours though right, not cpython's? Cpython's behavior is required to be correct.

@eric-wieser

@eric-wieser I guess I have a different angle. Implicitly CPython choose to say that the old-buffer protocol definition (lifetime is bound to exporting object) is valid. Otherwise they would have to deprecate and remove that special case path, leaving NumPy no choice (or no gain in sticking with the old implementation).
Of course, in all likelyhood nobody ever seriously tried to think about deprecating it in CPython, and simply put in the backward compatibility skim to close an issue or two.

So maybe its different. The choice is ours, but CPython gives us less choice to move to the new buffer-protocol definition.
And maybe the solution is actually to just screw it, and release NumPy with a flag NPY_USE_RELEASE_BUFFER=0. Keep at at 0 for a while in release versions, put it to 1 in pre-releases and wait out until the shouting dies down...

. Implicitly CPython choose to say that the old-buffer protocol definition (lifetime is bound to exporting object) is valid

My reading of this is different. CPython has concluded that if your buffer has no release process, then one of the following is true:

The buffer doesn't need releasing, as its lifetime is bound to the object
The buffer doesn't need releasing, as its lifetime is global

In essence, the reason goes "I cannot release this buffer, therefore no one can, and I can guarantee it will stick around as long as the pointer I return".

Yes, that is correct of course. But it doesn't help with the fact that it puts NumPy in a position where transitioning to the new meaning is tricky. Even though it would help us a lot (if we assume realloc's do not happen at least – if we do not assume that, it would still help us a lot, but we would have to do more shenanigans, much less than now though).

It seems also a bit annoying that the ownership tracking has to go through memoryview objects in this way. I admit, its probably just that something about that setup rubs me the wrong way.

But yes, you are right. There is nothing wrong as such, its just all not ideal and potentially Python could help us, since the argument-parsing is the main/only reason I have ever seen for NumPy to remain stuck with the old protocol/interpretation.

EDIT: But yes, from a Python perspective/protocol perspective, it is fully correct. Although it feels a bit weird if we may have good reason to check for bf_releasebuffer.

FWIW, Numpy is already using memoryviews for the life cycle management in other places:

numpy/numpy/core/src/multiarray/ctors.c

Lines 1532 to 1537 in 2889545

    
           PyObject *memoryview = PyMemoryView_FromObject(op); 
        
           if (memoryview == NULL) { 
        
               PyErr_Clear(); 
        
           } 
        
           else { 
        
               tmp = _array_from_buffer_3118(memoryview);

This was done, as the Py_buffer has to be stored somewhere (and the struct cannot be copied) and it was simplest to let a separate base object manage it. Frombuffer was not touched at the time, as it was using the old unsafe PyObject_AsReadBuffer/AsWriteBuffer API that still worked. But maybe it should have been changed back then.

BUG: Make mmap handling safer in frombuffer

Navigation Menu

Repository navigation

segfault when "viewing" closed mmap object #9537

6 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	PyObject *memoryview = PyMemoryView_FromObject(op);
	if (memoryview == NULL) {
	PyErr_Clear();
	}
	else {
	tmp = _array_from_buffer_3118(memoryview);

segfault when "viewing" closed mmap object #9537

Description

Activity

ahaldane commented on Aug 10, 2017

orkolorko commented on Aug 10, 2017

pv commented on Aug 10, 2017

jakirkham commented on Aug 3, 2018

eric-wieser commented on Apr 20, 2019

seberg commented on Apr 20, 2019

eric-wieser commented on Apr 20, 2019

devinrsmith commented on Apr 21, 2020

seberg commented on Apr 21, 2020

devinrsmith commented on Apr 21, 2020

eric-wieser commented on Apr 21, 2020

devinrsmith commented on Apr 21, 2020

6 remaining items

seberg commented on Apr 22, 2020

eric-wieser commented on Apr 22, 2020

eric-wieser commented on Apr 22, 2020

seberg commented on Apr 22, 2020

eric-wieser commented on Apr 22, 2020

seberg commented on Apr 22, 2020

seberg commented on Apr 22, 2020

eric-wieser commented on Apr 22, 2020

seberg commented on Apr 22, 2020

pv commented on Jun 22, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions