Skip to content
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.

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

possible unitialized values in PyTupleObject #146

Closed
mattip opened this issue Oct 8, 2020 · 9 comments
Closed

possible unitialized values in PyTupleObject #146

mattip opened this issue Oct 8, 2020 · 9 comments

Comments

@mattip
Copy link

mattip commented Oct 8, 2020

The code in sliding_window in itertoolz.pyx and here too can create a PyTupleObject with uninitialized items. I added some print(n) to the code, and the test_sliding_window code did not assign to self.prev[0]. If my guess is correct, this will cause PyPy to crash. All values must be assigned in creating a PyTupleObject. CPython will not check this, and will crash later if you ever access an uninitialized item, PyPy checks it and crashes earlier.

@hmaarrfk
Copy link

@mattip have you identified a fix for this, this is indeed crashing on pypy in conda-forge/cytoolz-feedstock#32

@mattip
Copy link
Author

mattip commented Jan 10, 2021

One solution would be to assign Py_None to self.prev[0] and current[0].

@hmaarrfk
Copy link

ill have to check this works later
#149

@hmaarrfk
Copy link

I'm sorry, i'm a real noob. But where do you import Py_None from?

@hmaarrfk
Copy link

Thanks. i managed to find 1 of the locations and that seemed to fix at least some tests. I still need to find the other.

@mattip
Copy link
Author

mattip commented Jan 13, 2021

I may have been mistaken in claiming there is another. Is there still an error when testing with PyPy?

@hmaarrfk
Copy link

Unfortunately so.
conda-forge/cytoolz-feedstock#34

At this point, it really is an intellectual exercise (like forgetting to initialize a variable), rather than hoping for the best user experience with pypy + toolz (as it is likely to let pypy do its magic rather than use cytoolz).

@mattip
Copy link
Author

mattip commented Jan 13, 2021

By doing

CFLAGS="-O0 -g3" python setup.py build_ext --inplace --with-cython
gdb --args python -mpytest cytoolz/tests/

I found two more:

diff --git a/cytoolz/itertoolz.pyx b/cytoolz/itertoolz.pyx
index e138f71..6c5bc88 100644
--- a/cytoolz/itertoolz.pyx
+++ b/cytoolz/itertoolz.pyx
@@ -960,12 +960,13 @@ cdef class sliding_window:
     [1.5, 2.5, 3.5]
     """
     def __cinit__(self, Py_ssize_t n, object seq):
         cdef Py_ssize_t i
         self.iterseq = iter(seq)
         self.prev = PyTuple_New(n)
+        PyTuple_SET_ITEM(self.prev, 0, None)
         for i, seq in enumerate(islice(self.iterseq, n-1), 1):
             Py_INCREF(seq)
             PyTuple_SET_ITEM(self.prev, i, seq)
         self.n = n
 
     def __iter__(self):
@@ -1051,12 +1052,14 @@ cdef class partition_all:
             i += 1
             if i == self.n:
                 return result
         # iterable exhausted before filling the tuple
         if i == 0:
             raise StopIteration
+        for j in range(i, self.n):
+            PyTuple_SET_ITEM(result, j, None)
         return PyTuple_GetSlice(result, 0, i)
 
 
 cpdef object count(object seq):
     """
     Count the number of items in seq

@eriknw
Copy link
Member

eriknw commented Jan 13, 2021

Fixed in #149 . I really do appreciate the care and effort y'all put into this. Is there anything else to be done? Would you like me to do a release?

@eriknw eriknw closed this as completed Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants