BUG: General fixes to f2py reference counts (dereferencing) #12633

seberg · 2018-12-31T00:39:46Z

Continuation of gh-12624, I am not sure how much I will keep working on this. I think the changes should be mostly/all fine. But the error handlig is not improved, only the reference counting dirtily. So sometimes I am not sure if this might actually make things worse and XDECREF should be used at least.

A lot of this could probably simply use a bit of an audit and love...

There is a single (probably) leak left in test_compile_functions.py after this, if I see it right.

mattip · 2018-12-31T07:55:23Z

Any idea why the python 3.5 tests are failing on travis?

seberg · 2018-12-31T10:11:41Z

Hmmm, I guess there may be one DECREF too many somewhere? But funny it only shows on 3.5.

seberg · 2018-12-31T14:02:02Z

numpy/f2py/src/fortranobject.c

            }
-    Py_XDECREF(v);
+    }


I guess this was correct before, I am just not used to not having curly brackets here.

seberg · 2018-12-31T14:05:10Z

numpy/f2py/rules.py

          #varname#_xa_capi = (PyTupleObject *)PySequence_Tuple(capi_tmp);
-        else
+          Py_DECREF(capi_tmp);


Hmm, this is the only thing that I am not quite sure of on first sight. I guess have to run it through a debugger/valgrind with python 3.5, but do not have a 3.5 on my arch laptop available right now.

Does not really smell wrong though, so really need to run through a debugger on py3.5...

mattip · 2018-12-31T14:56:34Z

Can reproduce the segfault in free after downloading the python3.5 used in the CI testing. It seems to be local to cleanup after running python3.5 runtests.py -t numpy/f2py/tests

seberg · 2018-12-31T15:07:30Z

@mattip can you run it in valgrind with --track-origins? I think that might tell us which python object got decref'd twice.

mattip · 2018-12-31T15:24:43Z

@seberg https://gist.github.com/mattip/8a98ca2e98258ea3d9e62173e0a4b776

mattip · 2018-12-31T15:52:47Z

found thise PyDict issue while nosing around, I don't know if it is relevant though it describes various use-after-free issues with dict that were fixed https://bugs.python.org/issue27945

mattip · 2018-12-31T16:16:42Z

numpy/f2py/rules.py

@@ -215,6 +215,7 @@
 \td = PyModule_GetDict(m);
 \ts = PyString_FromString(\"$R""" + """evision: $\");
 \tPyDict_SetItemString(d, \"__version__\", s);
+\tPy_DECREF(s);


This is the source of the double free on Python3.5. When I remove it I still get a segfault, but not a double free and abort

Seems strange that this should be so bad, it seems obviously right, no? Don't really have time to look into it though.

I have observed the double free with Python 3.5 as well, and the fix (counter-intuitively) is to use PyObject_Del instead of PyMem_Free in fortran_dealloc (see gh-14217).

Hmmm, PyObject_Free is the correct thing in any case I think. Although, I am a bit puzzle why it ends up crashing things so hard, but then I do not know how the python memory management works internally. I think I will rebase your PyObject_Free fix as a first commit into my PR?

mattip · 2018-12-31T16:39:22Z

I am not sure this is worth the effort. There is a problem with module teardown on python3.5. Using PYTHONVERBOSE=3 I get the following at teardown when running python3 runtests.py -t numpy/f2py/tests/test_callback.py

# cleanup[3] wiping _test_ext_module_5403
#   clear[2] func0
double free or corruption (out)

It seems the func0 value in the module dict has already been freed?

seberg · 2018-12-31T17:10:27Z

I think the leaks here are probably pretty harmless, since they are on import. The main reason would be to make debugging other stuff easier in the long run probably.

charris · 2019-01-19T22:44:19Z

Segfaulting on 3.5, not sure why.

charris · 2019-01-19T22:44:56Z

@seberg Do you feel that it would be safe to backport this?

seberg · 2019-01-19T23:11:17Z

@charris, likely safe. But at least almost all are just import time leaks that nobody ever noticed much. So I don't think it is important probably. Those 3.5 segfaults are worrying. Matti tracked something down that may be related, never looked at it closely, but it makes me think it might be best to just delay this for now in any case.

charris · 2019-01-19T23:21:38Z

OK, I kicked it off to 1.16.2.

Note that the extension module dict seems to be never dereferenced (there is an additional reference to it kept around somewhere). This reference seems to part of the C python module loading (possibly intentionally), and I could not find how to remove it or where it originates from.

mattip · 2019-09-22T05:51:46Z

numpy/f2py/tests/test_compile_function.py



 def test_f2py_init_compile_failure():
    # verify an appropriate integer status value returned by
    # f2py.compile() when invalid Fortran is provided
-    ret_val = numpy.f2py.compile(b"invalid")
+    ret_val = numpy.f2py.compile(b"invalid", verbose=False)


curious why you changed this?

Oh, those are those spammy comilation prints you say if you run the tests with -- -s... and they seem pretty unimportant to pirnt out by default.

This assumes anyone looking for what went wrong in the tests will know to add verbose=True in whatever test interests them

maybe add a comment somewhere?

Maybe, it would probably be nicestest to store it in a variable. In a sense that is what pytest does. I think part of it was that pytest leaks had issues with it (which are fixed). So I just remove it for now, can be added easily at any time.

mattip · 2019-09-22T05:53:47Z

LGTM. Anyone else want to take a look?

seberg mentioned this pull request Dec 31, 2018

BUG: Fix incorrect/missing reference cleanups found using valgrind #12624

Merged

9 tasks

seberg commented Dec 31, 2018

View reviewed changes

mattip reviewed Dec 31, 2018

View reviewed changes

charris changed the title ~~BUG: Gneral fixes to f2py reference counts (dereferencing)~~ BUG: General fixes to f2py reference counts (dereferencing) Jan 2, 2019

charris added this to the 1.16.1 release milestone Jan 2, 2019

charris added 00 - Bug component: numpy.f2py 09 - Backport-Candidate PRs tagged should be backported labels Jan 2, 2019

seberg mentioned this pull request Jan 8, 2019

BUG: Fix leak of void scalar buffer info #12696

Merged

charris mentioned this pull request Jan 17, 2019

BUG: Fix leak of void scalar buffer info #12784

Merged

charris modified the milestones: 1.16.1 release, 1.16.2 release Jan 19, 2019

charris removed this from the 1.16.2 release milestone Jan 22, 2019

charris removed the 09 - Backport-Candidate PRs tagged should be backported label Jun 28, 2019

seberg mentioned this pull request Aug 7, 2019

BUG: Fix leak in the f2py-generated module init and PyMem_Del usage #14217

Merged

seberg force-pushed the f2py-refcnt branch from 1009986 to 812dda2 Compare August 7, 2019 19:43

BUG: General fixes to f2py reference counts (dereferencing)

684bee2

seberg force-pushed the f2py-refcnt branch from 736e497 to 59d4100 Compare August 19, 2019 18:13

seberg added the 58 - Ready for review label Aug 19, 2019

seberg requested a review from mattip September 22, 2019 04:13

mattip reviewed Sep 22, 2019

View reviewed changes

mattip approved these changes Sep 22, 2019

View reviewed changes

seberg force-pushed the f2py-refcnt branch from 59d4100 to c091779 Compare September 22, 2019 16:07

mattip merged commit 908b601 into numpy:master Oct 15, 2019

seberg deleted the f2py-refcnt branch October 15, 2019 17:35

Uh oh!

BUG: General fixes to f2py reference counts (dereferencing) #12633

BUG: General fixes to f2py reference counts (dereferencing) #12633

Uh oh!

Conversation

seberg commented Dec 31, 2018

Uh oh!

mattip commented Dec 31, 2018

Uh oh!

seberg commented Dec 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattip commented Dec 31, 2018

Uh oh!

seberg commented Dec 31, 2018

Uh oh!

mattip commented Dec 31, 2018

Uh oh!

mattip commented Dec 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

superbobry Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattip commented Dec 31, 2018

Uh oh!

seberg commented Dec 31, 2018

Uh oh!

charris commented Jan 19, 2019

Uh oh!

charris commented Jan 19, 2019

Uh oh!

seberg commented Jan 19, 2019

Uh oh!

charris commented Jan 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattip commented Sep 22, 2019

Uh oh!

Uh oh!

superbobry Aug 7, 2019 •

edited

Loading