get_segment_info leaks reference count for items

Bug #1604335 reported by Kota Tsuyuzaki
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Object Storage (swift)
Invalid
Undecided
Unassigned
PyECLib
Fix Released
Undecided
Unassigned

Bug Description

ECDriver.get_segment_info seems to leak reference counts for the return dict due to a lack of Py_DECREF for each PyObject. This seems to cause a memory leak for callers (in particular, swift can be affected because swift uses the call to calculate segment/fragment seize)

*Current* pyeclib does like here[1]:

  ret_dict = PyDict_New();
  if (NULL == ret_dict) {
    pyeclib_c_seterr(-ENOMEM, "pyeclib_c_get_segment_info ERROR: ");
  } else {
    PyDict_SetItem(ret_dict, PyString_FromString("segment_size\0"), PyInt_FromLong(segment_size));
    PyDict_SetItem(ret_dict, PyString_FromString("last_segment_size\0"), PyInt_FromLong(last_segment_size));
    PyDict_SetItem(ret_dict, PyString_FromString("fragment_size\0"), PyInt_FromLong(fragment_size));
    PyDict_SetItem(ret_dict, PyString_FromString("last_fragment_size\0"), PyInt_FromLong(last_fragment_size));
    PyDict_SetItem(ret_dict, PyString_FromString("num_segments\0"), PyInt_FromLong(num_segments));
  }

  return ret_dict;

However, the docs for Python C-Extension says, sort of Python Object conversion (e.g. PyInt_FromLong) returns *a new reference* (i.e. increment the reference)[2]. And PyDict_SetItem seems to never steal the ownership for the reference unless the PyObject values throw the reference. Hence the reference counts of those objects included in the get_segment_info return value are never decremented, therefore they can not be collected as garbage.

An example to set items to the dict is available here [3]:

PyObject *key, *value;
Py_ssize_t pos = 0;

while (PyDict_Next(self->dict, &pos, &key, &value)) {
    int i = PyInt_AS_LONG(value) + 1;
    PyObject *o = PyInt_FromLong(i);
    if (o == NULL)
        return -1;
    if (PyDict_SetItem(self->dict, key, o) < 0) {
        Py_DECREF(o);
        return -1;
    }
    Py_DECREF(o);
}

That needs Py_DECREF explicitly for each PyObject items of the dict.

With [4] code, I made sure the reference count increasing. (Note that it needs debug mode python built with --with-pydebug) And then, a patch I will attach the link for gerrit obviously prevent the ref count increasing.

In Swift perspective, PUT/GET objects against to EC policy containers are affected.
GET objects are always affected since EC supported. PUT objects are affected only if the requests has content-length and affected versions have been since https://github.com/openstack/swift/commit/b5a243e75a8033988063d2c1c90ac373bc0050d2

1: https://github.com/openstack/pyeclib/blob/master/src/c/pyeclib_c/pyeclib_c.c#L433-L446
2: https://docs.python.org/2.7/c-api/int.html#c.PyInt_FromString
3: https://docs.python.org/2.7/c-api/dict.html#c.PyDict_Next
4: https://gist.github.com/bloodeagle40234/f4c0cd267e085cc6224ffdc1b1822631

Revision history for this message
Kota Tsuyuzaki (tsuyuzaki-kota) wrote :
Revision history for this message
Kota Tsuyuzaki (tsuyuzaki-kota) wrote :

Add Swift as affected project because this can cause memory leak in running Swift (with EC policy)

Revision history for this message
Kevin Greenan (kmgreen2) wrote : Re: [Bug 1604335] Re: get_segment_info leaks reference count for items
Download full text (3.2 KiB)

Thanks, Kota! Let me know when you add push the fix and I can review it.

-kevin

On Wed, Jul 20, 2016 at 2:11 AM, Kota Tsuyuzaki <
<email address hidden>> wrote:

> Add Swift as affected project because this can cause memory leak in
> running Swift (with EC policy)
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1604335
>
> Title:
> get_segment_info leaks reference count for items
>
> Status in PyECLib:
> New
> Status in OpenStack Object Storage (swift):
> New
>
> Bug description:
> ECDriver.get_segment_info seems to leak reference counts for the
> return dict due to a lack of Py_DECREF for each PyObject. This seems
> to cause a memory leak for callers (in particular, swift can be
> affected because swift uses the call to calculate segment/fragment
> seize)
>
> *Current* pyeclib does like here[1]:
>
>
> ret_dict = PyDict_New();
> if (NULL == ret_dict) {
> pyeclib_c_seterr(-ENOMEM, "pyeclib_c_get_segment_info ERROR: ");
> } else {
> PyDict_SetItem(ret_dict, PyString_FromString("segment_size\0"),
> PyInt_FromLong(segment_size));
> PyDict_SetItem(ret_dict, PyString_FromString("last_segment_size\0"),
> PyInt_FromLong(last_segment_size));
> PyDict_SetItem(ret_dict, PyString_FromString("fragment_size\0"),
> PyInt_FromLong(fragment_size));
> PyDict_SetItem(ret_dict,
> PyString_FromString("last_fragment_size\0"),
> PyInt_FromLong(last_fragment_size));
> PyDict_SetItem(ret_dict, PyString_FromString("num_segments\0"),
> PyInt_FromLong(num_segments));
> }
>
> return ret_dict;
>
>
> However, the docs for Python C-Extension says, sort of Python Object
> conversion (e.g. PyInt_FromLong) returns *a new reference* (i.e. increment
> the reference)[2]. And PyDict_SetItem seems to never steal the ownership
> for the reference unless the PyObject values throw the reference. Hence the
> reference counts of those objects included in the get_segment_info return
> value are never decremented, therefore they can not be collected as garbage.
>
> An example to set items to the dict is available here [3]:
>
> PyObject *key, *value;
> Py_ssize_t pos = 0;
>
> while (PyDict_Next(self->dict, &pos, &key, &value)) {
> int i = PyInt_AS_LONG(value) + 1;
> PyObject *o = PyInt_FromLong(i);
> if (o == NULL)
> return -1;
> if (PyDict_SetItem(self->dict, key, o) < 0) {
> Py_DECREF(o);
> return -1;
> }
> Py_DECREF(o);
> }
>
>
> That needs Py_DECREF explicitly for each PyObject items of the dict.
>
>
> With [4] code, I made sure the reference count increasing. (Note that it
> needs debug mode python built with --with-pydebug) And then, a patch I will
> attach the link for gerrit obviously prevent the ref count increasing.
>
>
> 1:
> https://github.com/openstack/pyeclib/blob/master/src/c/pyeclib_c/pyeclib_c.c#L433-L446
> 2: https://docs.python.org/2.7/c-api/int.html#c.PyInt_FromString
> 3: https://docs.python.org/2.7/c-api/dict.html#c.PyDict_Next
> 4:
> https://gist.github.com/bloodeagle40234/f4c0cd267e085cc6224ffdc1b1822631
>
> To manage n...

Read more...

description: updated
Revision history for this message
Kota Tsuyuzaki (tsuyuzaki-kota) wrote :

@Kevin

The fix is already pushed in gerrit, here, https://review.openstack.org/#/c/344066/ :-)

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to swift (master)

Related fix proposed to branch: master
Review: https://review.openstack.org/345120

Revision history for this message
Kota Tsuyuzaki (tsuyuzaki-kota) wrote :

One more quick note, with https://gist.github.com/bloodeagle40234/f4c0cd267e085cc6224ffdc1b1822631 we can confirm my patch https://review.openstack.org/#/c/344066/ decrease "possibly lost" and "still reachable" counts on valgrind --leak-check=full before/after the patch.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to swift (master)

Reviewed: https://review.openstack.org/345120
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=2876f59d4cdbafc297d79f4e6295f05e3448ae47
Submitter: Jenkins
Branch: master

commit 2876f59d4cdbafc297d79f4e6295f05e3448ae47
Author: Kota Tsuyuzaki <email address hidden>
Date: Wed Jul 20 18:16:27 2016 -0700

    Cache fragment size for EC policy

    ECStoragePolicy.fragment_size is never changed on running Swift because
    it is from ec_segment_size and ec_type defined in swift.conf statically
    so let's cache the value after retrieving the value from the pyeclib driver.

    And more, pyeclib <= 1.2.1 (current newest) has a bug [1] to leak the reference
    count of the items in the returned dict (i.e. causes memory leak) so that
    this caching will be mitigation of the memory leak because this saves the call
    count fewer than current as possible.

    Note that the complete fix for the memory leak for pyeclib is proposed at
    https://review.openstack.org/#/c/344066/

    1: https://bugs.launchpad.net/pyeclib/+bug/1604335

    Related-Bug: #1604335
    Change-Id: I6bbaa4063dc462383c949764b6567b2bee233689

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to swift (feature/hummingbird)

Related fix proposed to branch: feature/hummingbird
Review: https://review.openstack.org/363111

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to swift (feature/hummingbird)
Download full text (84.1 KiB)

Reviewed: https://review.openstack.org/363111
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=1ab2a296f58ae76aeffef9f3f0fb90e15358be27
Submitter: Jenkins
Branch: feature/hummingbird

commit 3b5850836c59c46f2507a7f62aceccf4c37e5d41
Author: gecong1973 <email address hidden>
Date: Tue Aug 30 15:08:49 2016 +0800

    Remove white space between print and ()

    There is a white space between print and ()
    in /tempauth.py, This patch fix it

    Change-Id: Id3493bdef12223aa3a2bffc200db8710f5949101

commit f88e7fc0da2ed6a63e0ea3c3459d80772b3068cd
Author: zheng yin <email address hidden>
Date: Mon Aug 29 20:21:44 2016 +0800

    Clarify test case in common/ring/test_builder

    They use a bare assertRaises(ValueError, ring.RingBuilder, *,*,*), but
    it's not clear which one raises which ValueError(), so I extend them to
    validate the error strings as well.

    Change-Id: I63280a9fc47ff678fe143e635046a0b402fd4506

commit d68b1bd6ddf44c5088e9d02dcb2f1b802c71411b
Author: zhufl <email address hidden>
Date: Mon Aug 29 14:31:27 2016 +0800

    Remove unnecessary tearDown

    This is to remove unnecessary tearDown to keep code clean.

    Change-Id: Ie70e40d6b55f379b0cc9bc372a35705462cade8b

commit d2fc2614575b04fd9cab5ae589880b92eee9b186
Author: Matthew Oliver <email address hidden>
Date: Fri Aug 19 16:17:31 2016 +1000

    Authorise versioned write PUTs before copy

    Currently a versioned write PUT uses a pre-authed request to move
    it into the versioned container before checking whether the
    user is authorised. This can lead to some interesting behaviour
    whereby a user can select a versioned object path that it does not
    have access to, request a put on that versioned object, and this
    request will execute the copy part of the request before it fails
    due to lack of permissions.

    This patch changes the behaviour to be the same as versioned DELETE
    where the request is authorised before anything is moved.

    Change-Id: Ia8b92251718d10b1eb44a456f28d3d2569a30003
    Closes-Bug: #1562175

commit c1ef6539b6ba9d2e4354c9cd2eec8a0195cdb19f
Author: Clay Gerrard <email address hidden>
Date: Thu Aug 25 11:00:49 2016 -0700

    add test for expirer processes == process

    This is a follow up from a change that improved the error message.

    Related-Change: I3d12b79470d122b2114f9ee486b15d381f290f95

    Change-Id: I093801f3516a60b298c13e2aa026c11c68a63792

commit 01477c78c1163822de41484e914a0736e622085b
Author: zheng yin <email address hidden>
Date: Thu Aug 25 15:37:42 2016 +0800

    Fix ValueError information in obj/expirer

    I fix error information in raise ValueError(...)
    For example:
        if a>=b:
            # It should be under below and not 'a must be less than or equal to b'
            raise ValueError('a must be less than b')

    Change-Id: I3d12b79470d122b2114f9ee486b15d381f290f95

commit b81f53b964fdb8f3b50dd369ce2e194ee4dbb0b7
Author: zheng yin <email address hidden>
Date: Tue Aug 23 14:26:47 2016 +0800

    Improve readability in the obj server's unit tests

    This change improves the reada...

tags: added: in-feature-hummingbird
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to pyeclib (master)

Reviewed: https://review.openstack.org/344066
Committed: https://git.openstack.org/cgit/openstack/pyeclib/commit/?id=cbb3d9364ff9cf453b06866d24073330760f8918
Submitter: Jenkins
Branch: master

commit cbb3d9364ff9cf453b06866d24073330760f8918
Author: Kota Tsuyuzaki <email address hidden>
Date: Tue Jul 19 00:13:00 2016 -0700

    Ref count for dict item should be Py_DECREF

    PyDict_SetItems doesn't steal the reference count so that the reference
    count of each item in the dict should be decremented via Py_DECREF
    *before* returning the dict to Python VM. Otherwise, those items can be
    leaked which can never be collected as garbage.

    The evicence for memory leak is avaialble for using debug mode python VM like:
    https://gist.github.com/bloodeagle40234/f4c0cd267e085cc6224ffdc1b1822631

    Plus, this patch add one unit test to check the memory increasement for
    many call of get_segment_info.

    Closes-Bug: #1604335

    Change-Id: I6780efb9718017d296606f3c7e60684910584a1a

Changed in pyeclib:
status: New → Fix Released
Revision history for this message
Matthew Oliver (matt-0) wrote :

This seems to be pyeclib related, which has been fixed and not a swift problem. So will change the swift side to invalid.

Nice work kota!

Changed in swift:
status: New → Invalid
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.