Skip to content

Make lsan suppressions more specific and fix revealed leaks #1032

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Make lsan suppressions more specific and fix revealed leaks #1032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/build.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ jobs:
"-DCMAKE_INSTALL_PREFIX=${InstallPrefix}" \
"-DCMAKE_BUILD_TYPE=${BuildType}" \
"-GNinja" \
${ProtonCMakeExtraArgs}
${{env.ProtonCMakeExtraArgs}}

- name: qpid-proton cmake build/install
run: cmake --build "${ProtonBuildDir}" --config ${BuildType} -t install
Expand All @@ -137,7 +137,7 @@ jobs:
"-DCMAKE_BUILD_TYPE=${BuildType}" \
"-DPYTHON_TEST_COMMAND='-m;pytest;-vs;--junit-prefix=pytest.\${py_test_module};--junit-xml=junitxmls/\${py_test_module}.xml;--pyargs;\${py_test_module}'" \
"-GNinja" \
${DispatchCMakeExtraArgs}
${{env.DispatchCMakeExtraArgs}}

- name: qpid-dispatch cmake build/install
run: cmake --build "${DispatchBuildDir}" --config ${BuildType} -t install
Expand Down
11 changes: 9 additions & 2 deletions src/dispatch.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,7 @@ qd_error_t qd_dispatch_prepare(qd_dispatch_t *qd)
void qd_dispatch_set_agent(qd_dispatch_t *qd, void *agent) {
assert(agent);
assert(!qd->agent);
Py_IncRef(agent); // TODO: why not incref here? actually probably needed, I get a leak in test 9 without this, strange...
qd->agent = agent;
}

Expand Down Expand Up @@ -371,12 +372,18 @@ void qd_dispatch_free(qd_dispatch_t *qd)
qd_connection_manager_free(qd->connection_manager);
qd_policy_free(qd->policy);
Py_XDECREF((PyObject*) qd->agent);
// Py_XDECREF(qd_python_module()); // hack
PyGC_Collect(); // run destructors while we still have the router around
// int ret = PyRun_SimpleString("import objgraph; import gc; gc.collect(); from qpid_dispatch_internal.management import config");
// assert(ret == 0);

// qd_python_finalize();

qd_router_free(qd->router);
qd_container_free(qd->container);
qd_server_free(qd->server);
qd_log_finalize();
qd_alloc_finalize();
qd_python_finalize();
qd_alloc_finalize(); // python objects may use alloc pool objects during finalization? TODO: they do that now // actually, no, too late to do real work now
qd_dispatch_set_router_id(qd, NULL);
qd_dispatch_set_router_area(qd, NULL);
qd_iterator_finalize();
Expand Down
28 changes: 22 additions & 6 deletions src/python_embedded.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,7 +565,8 @@ static PyTypeObject LogAdapterType = {
.tp_dealloc = (destructor)LogAdapter_dealloc,
.tp_flags = Py_TPFLAGS_DEFAULT,
.tp_methods = LogAdapter_methods,
.tp_init = (initproc)LogAdapter_init
.tp_init = (initproc)LogAdapter_init,
.tp_new = PyType_GenericNew,
};


Expand Down Expand Up @@ -719,10 +720,24 @@ static int IoAdapter_init(IoAdapter *self, PyObject *args, PyObject *kwds)
return 0;
}

// visit all members which may conceivably participate in reference cycles
static int IoAdapter_traverse(IoAdapter* self, visitproc visit, void *arg)
{
Py_VISIT(self->handler);
return 0;
}

static int IoAdapter_clear(IoAdapter* self)
{
Py_CLEAR(self->handler);
return 0;
}

Comment on lines +723 to +735
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This deals with the following leak, which was previously suppressed, because it involves Python in the stacktrace

9: Direct leak of 56 byte(s) in 1 object(s) allocated from:
9:     #0 0x7f78a3606e8f in __interceptor_malloc (/nix/store/g40sl3zh3nv52vj0mrl4iki5iphh5ika-gcc-10.2.0-lib/lib/libasan.so.6+0xace8f)
9:     #1 0x7f78a2d64afb in qd_malloc ../include/qpid/dispatch/ctools.h:229
9:     #2 0x7f78a2d657da in qdr_core_subscribe ../src/router_core/route_tables.c:149
9:     #3 0x7f78a2c83072 in IoAdapter_init ../src/python_embedded.c:711
9:     #4 0x7f78a2353a6c in type_call (/nix/store/r85nxfnwiv45nbmf5yb60jj8ajim4m7w-python3-3.8.5/lib/libpython3.8.so.1.0+0x165a6c)

The problem is in

class Agent:
    ...
    def activate(self, address):
        ...
        self.io = IoAdapter(self.receive, address, 'L', '0', TREATMENT_ANYCAST_CLOSEST)

IoAdapter refers to Agent (through the bound method reference to self.receive) and Agent refers to IoAdapter (through property self.io). Since IoAdapter is implemented in C and does not support Python's GC, there is no way to break the cycle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1052

static void IoAdapter_dealloc(IoAdapter* self)
{
qdr_core_unsubscribe(self->sub);
Py_DECREF(self->handler);
PyObject_GC_UnTrack(self);
IoAdapter_clear(self);
Py_TYPE(self)->tp_free((PyObject*)self);
}

Expand Down Expand Up @@ -802,10 +817,13 @@ static PyTypeObject IoAdapterType = {
.tp_name = DISPATCH_MODULE ".IoAdapter",
.tp_doc = "Dispatch IO Adapter",
.tp_basicsize = sizeof(IoAdapter),
.tp_traverse = (traverseproc)IoAdapter_traverse,
.tp_clear = (inquiry)IoAdapter_clear,
.tp_dealloc = (destructor)IoAdapter_dealloc,
.tp_flags = Py_TPFLAGS_DEFAULT,
.tp_flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC,
.tp_methods = IoAdapter_methods,
.tp_init = (initproc)IoAdapter_init,
.tp_new = PyType_GenericNew,
};


Expand All @@ -821,8 +839,6 @@ static void qd_register_constant(PyObject *module, const char *name, uint32_t va

static void qd_python_setup(void)
{
LogAdapterType.tp_new = PyType_GenericNew;
IoAdapterType.tp_new = PyType_GenericNew;
if ((PyType_Ready(&LogAdapterType) < 0) || (PyType_Ready(&IoAdapterType) < 0)) {
qd_error_py();
qd_log(log_source, QD_LOG_CRITICAL, "Unable to initialize Adapters");
Expand Down Expand Up @@ -866,7 +882,7 @@ static void qd_python_setup(void)
qd_register_constant(m, "TREATMENT_ANYCAST_CLOSEST", QD_TREATMENT_ANYCAST_CLOSEST);
qd_register_constant(m, "TREATMENT_ANYCAST_BALANCED", QD_TREATMENT_ANYCAST_BALANCED);

Py_INCREF(m);
// Py_INCREF(m); // PyImport_ImportModule does the increment already
dispatch_module = m;
}

Expand Down
40 changes: 40 additions & 0 deletions src/router_core/router_core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,9 @@ void qdr_core_free(qdr_core_t *core)
}
}

// I have to first run qdrc_endpoint_do_cleanup_CT src/router_core/core_link_endpoint.c:241
// and only then qcm_edge_router_final_CT src/router_core/modules/edge_router/module.c:59

qdr_link_route_t *link_route = 0;
while ( (link_route = DEQ_HEAD(core->link_routes))) {
DEQ_REMOVE_HEAD(core->link_routes);
Expand Down Expand Up @@ -323,17 +326,54 @@ void qdr_core_free(qdr_core_t *core)
}
assert(DEQ_SIZE(core->streaming_connections) == 0);

// this now adds unsubscribe actions, so have to do before the discard next
// plus I need to run it before core mutexes are freed

qdr_modules_finalize(core);

// discard any left over actions

qdr_action_list_t action_list;
DEQ_MOVE(core->action_list, action_list);
DEQ_APPEND(action_list, core->action_list_background);
qdr_action_t *action = DEQ_HEAD(action_list);
while (action) {
DEQ_REMOVE_HEAD(action_list);
action->action_handler(core, action, true);
free_qdr_action_t(action);
action = DEQ_HEAD(action_list);
}

// Drain the general work lists
qdr_general_handler(core);

// at this point all the conn identifiers have been freed
qd_hash_free(core->conn_id_hash);

qdr_agent_free(core->mgmt_agent);

//
// Free the core resources
//
if (core->routers_by_mask_bit) free(core->routers_by_mask_bit);
if (core->control_links_by_mask_bit) free(core->control_links_by_mask_bit);
if (core->data_links_by_mask_bit) free(core->data_links_by_mask_bit);
if (core->neighbor_free_mask) qd_bitmask_free(core->neighbor_free_mask);
if (core->rnode_conns_by_mask_bit) free(core->rnode_conns_by_mask_bit);

sys_thread_free(core->thread);
sys_cond_free(core->action_cond);
sys_mutex_free(core->action_lock);
sys_mutex_free(core->work_lock);
sys_mutex_free(core->id_lock);
qd_timer_free(core->work_timer);

for (int i = 0; i <= QD_TREATMENT_LINK_BALANCED; ++i) {
if (core->forwarders[i]) {
free(core->forwarders[i]);
}
}

free(core);
}

Expand Down
2 changes: 1 addition & 1 deletion src/router_node.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2169,12 +2169,12 @@ void qd_router_free(qd_router_t *router)

qd_container_set_default_node_type(router->qd, 0, 0, QD_DIST_BOTH);

qd_router_python_free(router);
qdr_core_free(router->router_core);
qd_tracemask_free(router->tracemask);
qd_timer_free(router->timer);
sys_mutex_free(router->lock);
qd_router_configure_free(router);
qd_router_python_free(router);

free(router);
qd_router_id_finalize();
Expand Down
20 changes: 13 additions & 7 deletions src/router_pynode.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,11 +308,12 @@ static PyMethodDef RouterAdapter_methods[] = {

static PyTypeObject RouterAdapterType = {
PyVarObject_HEAD_INIT(NULL, 0)
.tp_name = "dispatch.RouterAdapter", /* tp_name*/
.tp_basicsize = sizeof(RouterAdapter), /* tp_basicsize*/
.tp_flags = Py_TPFLAGS_DEFAULT, /* tp_flags*/
.tp_doc = "Dispatch Router Adapter", /* tp_doc */
.tp_methods = RouterAdapter_methods, /* tp_methods */
.tp_name = "dispatch.RouterAdapter",
.tp_basicsize = sizeof(RouterAdapter),
.tp_flags = Py_TPFLAGS_DEFAULT,
.tp_doc = "Dispatch Router Adapter",
.tp_methods = RouterAdapter_methods,
.tp_new = PyType_GenericNew,
Copy link
Contributor Author

@jiridanek jiridanek Feb 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for some reason, the code is always setting tp_new separately afterwards; I think there is not real advantage to that (?) and this way is cleaner

};


Expand Down Expand Up @@ -393,7 +394,6 @@ qd_error_t qd_router_python_setup(qd_router_t *router)
return QD_ERROR_NONE;

PyObject *pDispatchModule = qd_python_module();
RouterAdapterType.tp_new = PyType_GenericNew;
PyType_Ready(&RouterAdapterType);
QD_ERROR_PY_RET();

Expand Down Expand Up @@ -443,6 +443,7 @@ qd_error_t qd_router_python_setup(qd_router_t *router)
// Instantiate the router
//
pyRouter = PyObject_CallObject(pClass, pArgs);
Py_DECREF(pClass);
Py_DECREF(pArgs);
Py_DECREF(adapterType);
QD_ERROR_PY_RET();
Expand All @@ -455,7 +456,12 @@ qd_error_t qd_router_python_setup(qd_router_t *router)
}

void qd_router_python_free(qd_router_t *router) {
// empty
Py_XDECREF(pyRouter);
Py_XDECREF(pyTick);
Py_XDECREF(pySetMobileSeq);
Py_XDECREF(pySetMyMobileSeq);
Py_XDECREF(pyLinkLost);
// qd_python_finalize();
}


Expand Down
3 changes: 0 additions & 3 deletions tests/lsan.supp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@
# found by AddressSanitizer (ASAN)
#

# to be triaged; pretty much all tests
leak:^IoAdapter_init$

# to be triaged; pretty much all tests
leak:^load_server_config$

Expand Down