Skip to content

BUG: F2Py Invalid read in string_from_pyobj #18431

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.

Sign up for GitHub

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

Jump to bottom
Closed
seberg opened this issue Feb 17, 2021 · 2 comments · Fixed by #18759 or #19251
Closed

BUG: F2Py Invalid read in string_from_pyobj #18431

seberg opened this issue Feb 17, 2021 · 2 comments · Fixed by #18759 or #19251
Assignees
@pearu @melissawm
Labels
00 - Bug component: numpy.f2py

Comments

@seberg
Copy link
Member

seberg commented Feb 17, 2021

In this chunk of code:

numpy/numpy/f2py/cfuncs.py

Lines 652 to 663 in cafda24

if (PyArray_Check(obj)) {
if ((arr = (PyArrayObject *)obj) == NULL)
goto capi_fail;
if (!ISCONTIGUOUS(arr)) {
PyErr_SetString(PyExc_ValueError,\"array object is non-contiguous.\");
goto capi_fail;
}
if (*len == -1)
*len = (PyArray_ITEMSIZE(arr))*PyArray_SIZE(arr);
STRINGMALLOC(*str,*len);
STRINGCOPYN(*str,PyArray_DATA(arr),*len+1);
return 1;

F2Py is always copying one byte too much. But it seems that it expects a byte to be copied even for 0 length strings, this is also what happens for normal strings (simple copy, null byte is never reached). My gut feeling would be to copy *len and then dump \0 in the last *len item of the output, but apparently that fails the tests (excerpt):

numpy/f2py/tests/test_return_character.py:145: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <numpy.f2py.tests.test_return_character.TestF90ReturnCharacter object at 0x3c056c0>, t = <fortran object>, tname = 's1'

    def check_function(self, t, tname):
        if tname in ['t0', 't1', 's0', 's1']:
            assert_(t(23) == b'2')
            r = t('ab')
            assert_(r == b'a', repr(r))
            r = t(array('ab'))
>           assert_(r == b'a', repr(r))
E           AssertionError: b''

r          = b''
self       = <numpy.f2py.tests.test_return_character.TestF90ReturnCharacter object at 0x3c056c0>
t          = <fortran object>
tname      = 's1'

numpy/f2py/tests/test_return_character.py:18: AssertionError
================================================================================================================== short test summary info ===================================================================================================================
FAILED numpy/f2py/tests/test_return_character.py::TestF90ReturnCharacter::test_all[t0] - AssertionError: b''
FAILED numpy/f2py/tests/test_return_character.py::TestF90ReturnCharacter::test_all[t1] - AssertionError: b''
FAILED numpy/f2py/tests/test_return_character.py::TestF90ReturnCharacter::test_all[s0] - AssertionError: b''
FAILED numpy/f2py/tests/test_return_character.py::TestF90ReturnCharacter::test_all[s1] - AssertionError: b''
4 failed, 4 passed in 1.

Honestly, I am suprised how few problems there, maybe some vanished recently, but it might be with this fixed (and one more small refcount issue), valgrind may stop complaining about f2py tests.
@seberg
Copy link
Member Author

seberg commented Feb 17, 2021

It is also a bit worrying that it uses int instead of ssize_t everywhere, that may be worst for arrays, but is also problematic for the other branches though.

@melissawm
Copy link
Member

I can take a look at this, thanks for the heads up, @seberg !

@melissawm melissawm self-assigned this Feb 17, 2021
@seberg seberg mentioned this issue Feb 17, 2021
Merged
@charris charris mentioned this issue Feb 21, 2021
Merged
@melissawm melissawm mentioned this issue Mar 19, 2021
Closed
This was referenced Apr 12, 2021
Merged
Open
@pearu pearu self-assigned this Apr 13, 2021
pearu added a commit to pearu/numpy that referenced this issue May 10, 2021
@pearu
BUG: revise string_from_pyobj/try_pyarr_from_string usages, fixes num…
8992459 
…py#18431.
pearu added a commit that referenced this issue May 28, 2021
@pearu
Merge pull request #18759 from pearu/gh-18431-string_from_pyobj
4c93c93 
BUG: revise string_from_pyobj/try_pyarr_from_string with respect to malloc and copy.
@pearu pearu mentioned this issue Jun 15, 2021
Merged
charris added a commit that referenced this issue Jun 17, 2021
@charris
Merge pull request #19251 from pearu/gh-18431-string_from_pyobj-2
53d74a1 
BUG: revise string_from_pyobj/try_pyarr_from_string with respect to malloc and copy (the second round)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@pearu pearu

@melissawm melissawm

Labels
00 - Bug component: numpy.f2py
Projects
None yet
Milestone
No milestone
3 participants
@seberg @pearu @melissawm