version 3.0, any call to eval() leaks one reference:

>>> eval('1')
1
[42093 refs]
>>> eval('1')
1
[42094 refs]
>>> eval('1')
1
[42095 refs]
>>> eval('1')
1
[42096 refs]

The error is inside compile, not eval:

>>> compile("1", "test", "eval")
<code object <module> at 0x7ffe1ce2ed50, file "test", line 1>
[43379 refs]
>>> compile("1", "test", "eval")
<code object <module> at 0x7ffe1ce2e3b0, file "test", line 1>
[43380 refs]
>>> compile("1", "test", "eval")
<code object <module> at 0x7ffe1ce2ed50, file "test", line 1>
[43381 refs]
>>> compile("1", "test", "eval")
<code object <module> at 0x7ffe1ce2e3b0, file "test", line 1>
[43382 refs]

The leaked reference refers to the bytes object which encodes the code
being compiled:
>>> for x in range(1000): compile("1", "test", "eval")
>>> sys.getrefcount(b'1')
1004

Could it be related to the fact that test_bytes is leaking?

test_bytes leaked [129, 129, 129, 129] references, sum=516

Good point! It's leaking even more on my 64bit machine:

test_bytes leaked [195, 195, 195, 195] references, sum=780

Benjamin: yes, test_bytes calls eval() 128 times.
Christian: please "svn up", I just corrected another leak in bytesobject.c

I was already up to date. r65985 leaks 195 references in each run on my box.

The attached patch corrects the eval() and compile() reference leak.
The problem is in PyObject_AsReadBuffer.

Needs review: view->obj seems to own the reference, but object.h say the
opposite.

PyObject_AsCharBuffer is affected as well.
This fixes for me the last refleak in test_bytes.

Of course, I forgot PyObject_AsWriteBuffer in my patch.

I wonder if turning view.obj into an owned reference was a good idea.
There are more calls to bf_getbuffer (in getarg.c), they leak too: 

test_struct leaked [5, 5, 5, 5] references, sum=20

because of a PyArg_ParseTuple with s# and a bytes object.

Ignoring the question of whether owning the reference is the right thing
or not, the patch looks fine, although I don't see a reason for the
decrements to not be Py_DECREF since the function calls just won't even
happen if the object that the buffer is being created for is NULL.

As for changing whether Py_buffer holds a reference, that's tricky.
Removing the increment from PyBuffer_FillInfo() segfaults the
interpreter, so some code already assumes it does increment. There is
also the question of what the common use-case is; are buffers more like
items from a container, or their own object? The former says to
increment, the latter says to decrement. And since Py_buffer objects are
not true Python objects you can't just Py_DECREF() them.

My gut says to let buffers own a reference and to fix the "s#" leak.

Hi,

Making Py_buffer INCREF the original object is IMO the right thing to
do, because in most cases letting the original object disappear means
the memory region will become invalid as well. If you don't want the
INCREF, you can pass NULL as the obj parameter to PyBuffer_FillInfo() -
a piece of code in unicodeobject.c does just that.

However, since this decision was made recently (at the same time the s*
format codes were introduced), it is not reflected in the buffer API
documentation.

This is a partial (or complete) duplicate of 3656.

Even with the patch, there are still tons of leaks.  I don't know if
it's due to the same problem or not.  So far there is:

test_unittest leaked [124, 124] references, sum=248
test_array leaked [110, 110] references, sum=220
test_audioop leaked [75, 75] references, sum=150
test_binascii leaked [4, 4] references, sum=8

There are probably more.

The main cause of these leaks is each time PyArg_ParseTuple("s#") is
passed a bytes object.
If FillInfo() increfs the given object, every object should have a
bf_releasebuffer that decrefs it.

Le dimanche 24 août 2008 à 09:18 +0000, Amaury Forgeot d'Arc a écrit :
> If FillInfo() increfs the given object, every object should have a
> bf_releasebuffer that decrefs it.

There's no need for that, PyBuffer_Release() does the decref.
But PyBuffer_Release() must be used instead of calling bf_releasebuffer
directly.

This new version of the patch modifies getargs.c and use
PyObject_GetBuffer and PyBuffer_Release in place of the raw bf_getbuffer
and bf_releasebuffer.
Also make sure that each GetBuffer is matched with a ReleaseBuffer
(except for s* and w*, where the calling code is responsible for this)

This seems to correct most of the refleaks mentioned above, except:

- test_unittest leaks 120 refs because the 'audioop' module is
re-imported many times. See issue3667, or the test case should be dropped.

- test_binascii leaks one time because of issue3668.

Another PyBuffer_Release(&pin); looks necessary at @@ -805,6 +807,7 @@.

With the latest patch (and the one addition PyBuffer_Release() I noted
above), valgrind is happy wrt memory leaks.  The only problem valgrind
found is a read of uninitialized memory.  See
http://bugs.python.org/issue3657 .

I don't know the buffer code to know if this is the correct fix. 
However, it fixes the problems and that's probably good enough for now.
 If you can get someone familiar with the buffer code to review, that
would be great.  If not, this patch should be applied.

Also there should be a Misc/NEWS entry added. Also check the doc to see
it needs updating wrt ownership.

> I don't know the buffer code to know if this is the correct fix. 
> However, it fixes the problems and that's probably good enough for now.
>  If you can get someone familiar with the buffer code to review, that
> would be great.

I don't know if I pass as "someone familiar with the buffer code", but
Amaury's patch is ok to me.

Updated patch with the additional PyBuffer_Release mentioned by Neal,
plus a proposition of entry for the NEWS file.

There is no doc about the new interface, so no update is needed :-( 
See issue2619.

Amaury, assuming you have tested it :-), the patch is ok to me. You can
commit.

Fixed in r66047.