Repository: cython/cython
Pull request: Fix segfault on zero-length slice assignment on memoryview
As outlined in #6227 — we have experienced an issue in production where attempting to write a folly:IOBuf with zero-length backing buffers is triggering a SIGSEGV.

The specific issue actual has to do with how `memoryview` attempts to use scalar assignment instead of slice assignment in the case where the source buffer (in this case, an empty slice) has `len == 0`. By fixing the fork to properly use slice assignment, we avoid the SIGSEGV when we string format the view format (which is NULL).

This PR also adds two additional tests for writing either a single byte or an empty slice to a `memoryview`.

Tested via: `python runtests.py memslice -vv` (which includes the two new tests)

fixes #6227 

 

Problem statement:
[BUG] Segfault on zero-length slice assignment on memoryview
### Describe the bug

We've experienced an issue in production where attempting to write a `folly:IOBuf` with zero-length backing buffers is triggering a SIGSEGV. 

We're traced this issue to the way Cython is executing zero-length slices on `cmemoryview`, e.g. `view[0:0]`. While this problem was trivially addressed by no-oping zero-length writes to our stream, this behavior represents a divergence from the standard Python behavior where zero-length slice access works as intended.

### Code to reproduce the behaviour:

main.pyx: Create a struct that wraps a Python `memoryview` and exposes a function to write nothing to an empty slice
```cython
from cython.view cimport memoryview as cmemoryview
from cpython.buffer cimport PyBUF_C_CONTIGUOUS

cdef class MemoryViewStream:
  cdef cmemoryview view

  def __cinit__(self, object view):
    self.view = cmemoryview(view, PyBUF_C_CONTIGUOUS)

  def write_nothing(self):
    self.view[0:0] = []
```

test.py
```python
from main import MemoryViewStream

# empty byte array of len 10
dest_byte_arr = bytearray(10)
mem_view = memoryview(dest_byte_arr)

# create C struct to represent this stream
stream = MemoryViewStream(mem_view)

# write zero length bytearray to the stream
# Segfaults here
stream.write_nothing()
print("Completed successfully")
```

setup.py
```python
from setuptools import setup
from Cython.Build import cythonize

setup(
    ext_modules = cythonize("main.pyx")
)
```


### Expected behaviour

We expect `python test.py` to execute successfully and print "Completed successfully" — however, instead we end up with a SIGSEGV due to an invalid memory access in the C stdlib.

### OS

Repro'd on both Mac and CentOS

### Python version

3.9.6

### Cython version

3.0.10

### Additional context

Stack trace on SIGSEGV indicates that C `strlen` is being invoked on a null pointer, and most likely a fix is required in `__pyx_memoryview_assign_item_from_object`

Thread 0 Crashed::  Dispatch queue: com.apple.main-thread
0   libsystem_platform.dylib      	       0x184d18904 _platform_strlen + 4
1   Python3                       	       0x104c75e2c PyBytes_FromString + 24
2   main.cpython-39-darwin.so     	       0x10445a95c __pyx_memoryview_assign_item_from_object + 132 (main.c:8673)
3   main.cpython-39-darwin.so     	       0x104459e90 __pyx_memoryview_setitem_slice_assign_scalar + 172 (main.c:8149)
4   main.cpython-39-darwin.so     	       0x104464c50 __pyx_memoryview___pyx_pf_15View_dot_MemoryView_10memoryview_6__setitem__ + 1044 (main.c:7623) [inlined]
5   main.cpython-39-darwin.so     	       0x104464c50 __pyx_memoryview___setitem__ + 1044 (main.c:7469) [inlined]
6   main.cpython-39-darwin.so     	       0x104464c50 __pyx_mp_ass_subscript_memoryview + 1304 (main.c:18513)
7   main.cpython-39-darwin.so     	       0x10445b5c4 __Pyx_PyObject_SetSlice + 36 (main.c:22955) [inlined]
8   main.cpython-39-darwin.so     	       0x10445b5c4 __pyx_pf_4main_16MemoryViewStream_2write_nothing + 56 (main.c:17573) [inlined]
9   main.cpython-39-darwin.so     	       0x10445b5c4 __pyx_pw_4main_16MemoryViewStream_3write_nothing + 88 (main.c:17550)
10  Python3                       	       0x104d52a38 0x104c40000 + 1124920
11  Python3                       	       0x104d4fb54 _PyEval_EvalFrameDefault + 22664
12  Python3                       	       0x104d537ec 0x104c40000 + 1128428
13  Python3                       	       0x104d4a204 PyEval_EvalCode + 80
14  Python3                       	       0x104d8e2f0 0x104c40000 + 1368816
15  Python3                       	       0x104d8e4c4 0x104c40000 + 1369284
16  Python3                       	       0x104d8c5ec PyRun_SimpleFileExFlags + 820
17  Python3                       	       0x104da97cc Py_RunMain + 1604
18  Python3                       	       0x104da9c3c 0x104c40000 + 1481788
19  Python3                       	       0x104da9cdc Py_BytesMain + 40
20  dyld                          	       0x1849620e0 start + 2360

Hints from issue discussion:
I think the problem is that the buffer format is `NULL`.

If that's something that Cython controls (e.g. if skips filling in the buffer format for empty slices) then it might be better to change that instead. Otherwise we need a NULL check as you suggest.
I have a repro via unit tests here: https://github.com/michael99man/cython/tree/memoryview_zero_length_slice_fix

Am currently working on a fix

Commit message: Fix zero length memoryview assignment segfault by correctly using slice assignment

Patch to review:
diff --git a/Cython/Utility/MemoryView.pyx b/Cython/Utility/MemoryView.pyx
index c3525912638..66c035e4957 100644
--- a/Cython/Utility/MemoryView.pyx
+++ b/Cython/Utility/MemoryView.pyx
@@ -417,7 +417,7 @@ cdef class memoryview:
 
         if have_slices:
             obj = self.is_slice(value)
-            if obj:
+            if obj != None:
                 self.setitem_slice_assignment(self[index], obj)
             else:
                 self.setitem_slice_assign_scalar(self[index], value)
diff --git a/tests/memoryview/memslice.pyx b/tests/memoryview/memslice.pyx
index 85cbfe3ec4e..f8364fd5679 100644
--- a/tests/memoryview/memslice.pyx
+++ b/tests/memoryview/memslice.pyx
@@ -12,14 +12,17 @@ from cpython.ref cimport Py_INCREF, Py_DECREF, Py_CLEAR, Py_REFCNT
 cimport cython
 from cython cimport view
 from cython.view cimport array
+from cython.view cimport memoryview
 from cython.parallel cimport prange, parallel
-
 from functools import wraps
 import gc
 import sys
 
 import builtins
 
+cdef extern from "Python.h":
+    cdef int PyBUF_C_CONTIGUOUS
+
 try:
     from Cython.Tests.this_module_does_not_exist import *
 except ImportError:
@@ -2177,6 +2180,40 @@ cdef _test_slice_assignment_broadcast_strides(slice_1d src, slice_2d dst, slice_
         for j in range(1, 3):
             assert dst[i, j] == dst_f[i, j] == j - 1, (dst[i, j], dst_f[i, j], j - 1)
 
+def test_slice_assignment_single_length_slice():
+    """
+    >>> test_slice_assignment_single_length_slice()
+    1
+    """
+    cdef char[1] src_buf
+    src_buf[0] = 0x01
+
+    dest_buf: bytearray = bytearray(1)
+
+    # construct a memory view of the destination buffer
+    view = memoryview(dest_buf, PyBUF_C_CONTIGUOUS)
+
+    # assign a slice of len = 1
+    view[0:1] = src_buf[0:1]
+    print(dest_buf[0])
+
+def test_slice_assignment_zero_length_slice():
+    """
+    >>> test_slice_assignment_zero_length_slice()
+    0
+    """
+    cdef char[1] src_buf
+    src_buf[0] = 0x01
+
+    dest_buf: bytearray = bytearray(1)
+
+    # construct a memory view of the destination buffer
+    view = memoryview(dest_buf, PyBUF_C_CONTIGUOUS)
+
+    # assign a empty slice — this should noop
+    view[0:0] = src_buf[0:0]
+    print(dest_buf[0])
+
 @testcase
 def test_borrowed_slice():
     """


Code review comments:
diff_hunk: @@ -12,14 +12,17 @@ from cpython.ref cimport Py_INCREF, Py_DECREF, Py_CLEAR, Py_REFCNT
 cimport cython
 from cython cimport view
 from cython.view cimport array
+from cython.view cimport memoryview
 from cython.parallel cimport prange, parallel
-
path: tests/memoryview/memslice.pyx
text: @user1:
Unnecessary change.

diff_hunk: @@ -2177,6 +2180,40 @@ cdef _test_slice_assignment_broadcast_strides(slice_1d src, slice_2d dst, slice_
         for j in range(1, 3):
             assert dst[i, j] == dst_f[i, j] == j - 1, (dst[i, j], dst_f[i, j], j - 1)
 
+def test_slice_assignment_single_length_slice():
+    """
+    >>> test_slice_assignment_single_length_slice()
+    1
+    """
+    cdef char[1] src_buf
+    src_buf[0] = 0x01
+
+    dest_buf: bytearray = bytearray(1)
+
+    # construct a memory view of the destination buffer
+    view = memoryview(dest_buf, PyBUF_C_CONTIGUOUS)
+
+    # assign a slice of len = 1
+    view[0:1] = src_buf[0:1]
+    print(dest_buf[0])
+
+def test_slice_assignment_zero_length_slice():
+    """
+    >>> test_slice_assignment_zero_length_slice()
+    0
+    """
+    cdef char[1] src_buf
+    src_buf[0] = 0x01
+
+    dest_buf: bytearray = bytearray(1)
+
+    # construct a memory view of the destination buffer
+    view = memoryview(dest_buf, PyBUF_C_CONTIGUOUS)
path: tests/memoryview/memslice.pyx
text: @user1:
```suggestion
    cdef char[::1] view = dest_buf
```

@author:
Under the hood, does this do the same memory mapping between the Python bytearray and the memoryview object?

diff_hunk: @@ -2177,6 +2180,40 @@ cdef _test_slice_assignment_broadcast_strides(slice_1d src, slice_2d dst, slice_
         for j in range(1, 3):
             assert dst[i, j] == dst_f[i, j] == j - 1, (dst[i, j], dst_f[i, j], j - 1)
 
+def test_slice_assignment_single_length_slice():
path: tests/memoryview/memslice.pyx
text: @user2:
Probably no longer needed overall, but we should be consistent across the test module.
```suggestion
@user1
def test_slice_assignment_single_length_slice():
```

diff_hunk: @@ -2177,6 +2180,40 @@ cdef _test_slice_assignment_broadcast_strides(slice_1d src, slice_2d dst, slice_
         for j in range(1, 3):
             assert dst[i, j] == dst_f[i, j] == j - 1, (dst[i, j], dst_f[i, j], j - 1)
 
+def test_slice_assignment_single_length_slice():
+    """
+    >>> test_slice_assignment_single_length_slice()
+    1
+    """
+    cdef char[1] src_buf
+    src_buf[0] = 0x01
+
+    dest_buf: bytearray = bytearray(1)
+
+    # construct a memory view of the destination buffer
+    view = memoryview(dest_buf, PyBUF_C_CONTIGUOUS)
+
+    # assign a slice of len = 1
+    view[0:1] = src_buf[0:1]
+    print(dest_buf[0])
+
+def test_slice_assignment_zero_length_slice():
path: tests/memoryview/memslice.pyx
text: @user2:
```suggestion
@user1
def test_slice_assignment_zero_length_slice():
```

diff_hunk: @@ -2177,6 +2180,40 @@ cdef _test_slice_assignment_broadcast_strides(slice_1d src, slice_2d dst, slice_
         for j in range(1, 3):
             assert dst[i, j] == dst_f[i, j] == j - 1, (dst[i, j], dst_f[i, j], j - 1)
 
+def test_slice_assignment_single_length_slice():
+    """
+    >>> test_slice_assignment_single_length_slice()
+    1
+    """
+    cdef char[1] src_buf
+    src_buf[0] = 0x01
+
+    dest_buf: bytearray = bytearray(1)
+
+    # construct a memory view of the destination buffer
+    view = memoryview(dest_buf, PyBUF_C_CONTIGUOUS)
+
+    # assign a slice of len = 1
+    view[0:1] = src_buf[0:1]
+    print(dest_buf[0])
path: tests/memoryview/memslice.pyx
text: @user1:
I think `return` is simpler than `print()` here.
```suggestion
    return dest_buf[0]
```

diff_hunk: @@ -2177,6 +2180,40 @@ cdef _test_slice_assignment_broadcast_strides(slice_1d src, slice_2d dst, slice_
         for j in range(1, 3):
             assert dst[i, j] == dst_f[i, j] == j - 1, (dst[i, j], dst_f[i, j], j - 1)
 
+def test_slice_assignment_single_length_slice():
+    """
+    >>> test_slice_assignment_single_length_slice()
+    1
+    """
+    cdef char[1] src_buf
+    src_buf[0] = 0x01
+
+    dest_buf: bytearray = bytearray(1)
+
+    # construct a memory view of the destination buffer
+    view = memoryview(dest_buf, PyBUF_C_CONTIGUOUS)
+
+    # assign a slice of len = 1
+    view[0:1] = src_buf[0:1]
+    print(dest_buf[0])
+
+def test_slice_assignment_zero_length_slice():
+    """
+    >>> test_slice_assignment_zero_length_slice()
+    0
+    """
+    cdef char[1] src_buf
+    src_buf[0] = 0x01
+
+    dest_buf: bytearray = bytearray(1)
+
+    # construct a memory view of the destination buffer
+    view = memoryview(dest_buf, PyBUF_C_CONTIGUOUS)
+
+    # assign a empty slice — this should noop
+    view[0:0] = src_buf[0:0]
+    print(dest_buf[0])
path: tests/memoryview/memslice.pyx
text: @user1:
```suggestion
    return dest_buf[0]
```

diff_hunk: @@ -2177,6 +2180,40 @@ cdef _test_slice_assignment_broadcast_strides(slice_1d src, slice_2d dst, slice_
         for j in range(1, 3):
             assert dst[i, j] == dst_f[i, j] == j - 1, (dst[i, j], dst_f[i, j], j - 1)
 
+def test_slice_assignment_single_length_slice():
+    """
+    >>> test_slice_assignment_single_length_slice()
+    1
+    """
+    cdef char[1] src_buf
+    src_buf[0] = 0x01
+
+    dest_buf: bytearray = bytearray(1)
+
+    # construct a memory view of the destination buffer
+    view = memoryview(dest_buf, PyBUF_C_CONTIGUOUS)
path: tests/memoryview/memslice.pyx
text: @user1:
```suggestion
    cdef char[::1] view = dest_buf
```

diff_hunk: @@ -417,7 +417,7 @@ cdef class memoryview:
 
         if have_slices:
             obj = self.is_slice(value)
-            if obj:
+            if obj != None:
path: Cython/Utility/MemoryView.pyx
text: @user1:
The usual way to spell this is with an identity test, not equality.
```suggestion
            if obj is not None:
```

@author:
Good catch — got tripped up between C/Python :P

diff_hunk: @@ -12,14 +12,17 @@ from cpython.ref cimport Py_INCREF, Py_DECREF, Py_CLEAR, Py_REFCNT
 cimport cython
 from cython cimport view
 from cython.view cimport array
+from cython.view cimport memoryview
 from cython.parallel cimport prange, parallel
-
 from functools import wraps
 import gc
 import sys
 
 import builtins
 
+cdef extern from "Python.h":
+    cdef int PyBUF_C_CONTIGUOUS
+
path: tests/memoryview/memslice.pyx
text: @user1:
We shouldn't be needing this either.
```suggestion
```

diff_hunk: @@ -12,14 +12,17 @@ from cpython.ref cimport Py_INCREF, Py_DECREF, Py_CLEAR, Py_REFCNT
 cimport cython
 from cython cimport view
 from cython.view cimport array
+from cython.view cimport memoryview
path: tests/memoryview/memslice.pyx
text: @user1:
We shouldn't be needing this. There's syntax for it.
```suggestion
```