You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by dskarbek <gi...@git.apache.org> on 2016/10/31 17:10:53 UTC
[GitHub] qpid-dispatch pull request #108: Fix memory leaks
GitHub user dskarbek opened a pull request:
https://github.com/apache/qpid-dispatch/pull/108
Fix memory leaks
Just getting my feet wet really. Fix a few little things.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dskarbek/qpid-dispatch master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/qpid-dispatch/pull/108.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #108
----
commit 405bd63ab7015ad3b794fe67d596c7e4cd7a7876
Author: Daniel Skarbek <ds...@stanfordalumni.org>
Date: 2016-10-27T23:12:52Z
Fix memory leak in dispatch
router_id is being reassigned without freeing the prior string. Made a setter method that ensures
that the prior pointer is freed. Did the same for router_area as well to keep the design parallel.
commit aeb8f41872b9749c5d20d576b030b0af012c18a8
Author: Daniel Skarbek <ds...@stanfordalumni.org>
Date: 2016-10-28T00:05:28Z
Fix memory leaks
Add more clean-up of router core object. Several members were simply being leaked.
commit faeca6faca91df634ef4425caa7103e99e52b910
Author: Daniel Skarbek <ds...@stanfordalumni.org>
Date: 2016-10-28T18:43:25Z
Cleanup in hash.c
Code was not using DEQ_NEXT() to access the next pointer
qd_hash_insert_const() was passing "error" as the "exists" indicator. This is semantically incorrect and has
the effect of returning QD_ERROR_NOT_FOUND when the entry already exists after actually over-writting with the
new item.
qd_hash_retrieve_prefix_const() was missing the qd_iterator_hash_segments() call.
factor out qd_hash_internal_remove_item() to consolidate the remove logic. Also, reuse the retrieve logic
inside the remove method
commit 46960384b4af10b4640025d37bbbaa3d0d5c4187
Author: Daniel Skarbek <ds...@stanfordalumni.org>
Date: 2016-10-28T20:42:23Z
Fix memory leak of core addrs
Decompose addr removal and free logic and use when freeing the core
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #108: Fix memory leaks
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/qpid-dispatch/pull/108
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch issue #108: Fix memory leaks
Posted by ganeshmurthy <gi...@git.apache.org>.
Github user ganeshmurthy commented on the issue:
https://github.com/apache/qpid-dispatch/pull/108
I was able to successfully merge your code to my local master but ran into a compile issue.
In file included from /home/gmurthy/opensource/dispatch/include/qpid/dispatch/ctools.h:27:0,
from /home/gmurthy/opensource/dispatch/include/qpid/dispatch/container.h:35,
from /home/gmurthy/opensource/dispatch/src/dispatch_private.h:40,
from /home/gmurthy/opensource/dispatch/src/router_core/router_core_private.h:22,
from /home/gmurthy/opensource/dispatch/src/router_core/route_tables.c:20:
/home/gmurthy/opensource/dispatch/src/router_core/route_tables.c: In function \u2018qdr_del_router_CT\u2019:
/home/gmurthy/opensource/dispatch/src/router_core/route_tables.c:353:17: error: \u2018qdr_node_t {aka struct qdr_node_t}\u2019 has no member named \u2018maskbit\u2019; did you mean \u2018mask_bit\u2019?
assert(rnode->maskbit == router_maskbit);
Fix is easy enough (missing an underscore in maskbit)
assert(rnode->mask_bit == router_maskbit);
After the fix, the code compiled and I was able to run all unit tests successfully. I did not see any segfaults.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #108: Fix memory leaks
Posted by dskarbek <gi...@git.apache.org>.
Github user dskarbek commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/108#discussion_r87630081
--- Diff: src/router_config.c ---
@@ -26,6 +26,27 @@
#include "entity_cache.h"
#include "schema_enum.h"
+static void qdi_router_configure_body(qdr_core_t *core,
+ qd_composed_field_t *body,
+ qd_router_entity_type_t type,
+ char *name)
+{
+ qd_buffer_list_t buffers;
+ qd_compose_take_buffers(body, &buffers);
+
+ qd_field_iterator_t *iter = qd_field_iterator_buffer(DEQ_HEAD(buffers), 0, qd_buffer_list_length(&buffers));
+ qd_parsed_field_t *in_body = qd_parse(iter);
+ qd_field_iterator_free(iter);
+
+ qd_field_iterator_t *name_iter = 0;
+ if (name)
+ name_iter = qd_field_iterator_string(name);
+
+ qdr_manage_create(core, 0, type, name_iter, in_body, 0, buffers);
--- End diff --
Yeah, I agree this is not the most pretty solution. But, wasn't sure how else to handle it. Happy to hear about a nicer approach.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch issue #108: Fix memory leaks
Posted by dskarbek <gi...@git.apache.org>.
Github user dskarbek commented on the issue:
https://github.com/apache/qpid-dispatch/pull/108
Hmm\u2026 I wonder how it compiled on my machine?
-Dan Skarbek
From: Ganesh Murthy <no...@github.com>
Reply-To: apache/qpid-dispatch <re...@reply.github.com>
Date: Friday, November 11, 2016 at 11:08 AM
To: apache/qpid-dispatch <qp...@noreply.github.com>
Cc: "Skarbek, Daniel" <ds...@paypal.com>, Mention <me...@noreply.github.com>
Subject: Re: [apache/qpid-dispatch] Fix memory leaks (#108)
I was able to successfully merge your code to my local master but ran into a compile issue.
In file included from /home/gmurthy/opensource/dispatch/include/qpid/dispatch/ctools.h:27:0,
from /home/gmurthy/opensource/dispatch/include/qpid/dispatch/container.h:35,
from /home/gmurthy/opensource/dispatch/src/dispatch_private.h:40,
from /home/gmurthy/opensource/dispatch/src/router_core/router_core_private.h:22,
from /home/gmurthy/opensource/dispatch/src/router_core/route_tables.c:20:
/home/gmurthy/opensource/dispatch/src/router_core/route_tables.c: In function \u2018qdr_del_router_CT\u2019:
/home/gmurthy/opensource/dispatch/src/router_core/route_tables.c:353:17: error: \u2018qdr_node_t {aka struct qdr_node_t}\u2019 has no member named \u2018maskbit\u2019; did you mean \u2018mask_bit\u2019?
assert(rnode->maskbit == router_maskbit);
Fix is easy enough (missing an underscore in maskbit)
assert(rnode->mask_bit == router_maskbit);
After the fix, the code compiled and I was able to run all unit tests successfully. I did not see any segfaults.
\u2014
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://github.com/apache/qpid-dispatch/pull/108#issuecomment-260031465>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ADfgn1NcK3ynL9pbKhI5AXnxGWahHo8Pks5q9L0egaJpZM4KlQvY>.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch issue #108: Fix memory leaks
Posted by dskarbek <gi...@git.apache.org>.
Github user dskarbek commented on the issue:
https://github.com/apache/qpid-dispatch/pull/108
@ganeshmurthy Ted had mentioned that he pulled my commits and was getting a seg-fault on exit. I haven't been able to reproduce that, and can't see any change that would be causing that just by code inspection. Can you confirm this issue and let me know how to reproduce?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #108: Fix memory leaks
Posted by ganeshmurthy <gi...@git.apache.org>.
Github user ganeshmurthy commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/108#discussion_r87624058
--- Diff: src/connection_manager.c ---
@@ -240,12 +247,15 @@ static qd_error_t load_server_config(qd_dispatch_t *qd, qd_server_config_t *conf
config->allowInsecureAuthentication = true;
config->verify_host_name = verifyHostName;
+ char *stripAnnotations = qd_entity_opt_string(entity, "stripAnnotations", 0);
--- End diff --
The existing code already seems to free stripAnnotations
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #108: Fix memory leaks
Posted by dskarbek <gi...@git.apache.org>.
Github user dskarbek commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/108#discussion_r86455403
--- Diff: src/router_config.c ---
@@ -26,6 +26,27 @@
#include "entity_cache.h"
#include "schema_enum.h"
+static void qdi_router_configure_body(qdr_core_t *core,
--- End diff --
wasn't sure what the best function name would be here. Let me know if you want a different name.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #108: Fix memory leaks
Posted by dskarbek <gi...@git.apache.org>.
Github user dskarbek commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/108#discussion_r87629907
--- Diff: src/dispatch_private.h ---
@@ -141,4 +141,16 @@ void qd_dispatch_unregister_entity(qd_dispatch_t *qd, void *impl);
/** Set the agent */
void qd_dispatch_set_agent(qd_dispatch_t *qd, void *agent);
+/**
--- End diff --
Yeah, I tried it that way also. The advantage of this is that I can pass in NULL to cause the memory to be freed. But, the copy in may be the better way. Only issue, is if you look at the two places this is being use, one is passing strdup, so no problem there, the strdup could just happen inside the method. The other one is passing the result of qd_entity_opt_string(). That also returns newly allocated memory, right? So, I'd have to catch that return separately, pass it in and then free it. And, we'd basically be doing a wasted extra alloc/free. That didn't seem so nice. So, that's why I tended toward having this internal method just manage the setting of the pointer and ensuring that prior pointee's are freed. And, of course, that's why I commented the method as taking ownership of the memory. Let me know if you prefer the other approach.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #108: Fix memory leaks
Posted by ganeshmurthy <gi...@git.apache.org>.
Github user ganeshmurthy commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/108#discussion_r87621418
--- Diff: src/router_config.c ---
@@ -26,6 +26,27 @@
#include "entity_cache.h"
#include "schema_enum.h"
+static void qdi_router_configure_body(qdr_core_t *core,
+ qd_composed_field_t *body,
+ qd_router_entity_type_t type,
+ char *name)
+{
+ qd_buffer_list_t buffers;
+ qd_compose_take_buffers(body, &buffers);
+
+ qd_field_iterator_t *iter = qd_field_iterator_buffer(DEQ_HEAD(buffers), 0, qd_buffer_list_length(&buffers));
+ qd_parsed_field_t *in_body = qd_parse(iter);
+ qd_field_iterator_free(iter);
+
+ qd_field_iterator_t *name_iter = 0;
+ if (name)
+ name_iter = qd_field_iterator_string(name);
+
+ qdr_manage_create(core, 0, type, name_iter, in_body, 0, buffers);
--- End diff --
You are correct that these buffers are not freed. We are thinking of other ways to free the buffers without having to pass it thru to the qdr_manage_create() method because it might not be relevant to the method itself other than the fact that the method is freeing it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #108: Fix memory leaks
Posted by ganeshmurthy <gi...@git.apache.org>.
Github user ganeshmurthy commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/108#discussion_r87624676
--- Diff: src/dispatch_private.h ---
@@ -141,4 +141,16 @@ void qd_dispatch_unregister_entity(qd_dispatch_t *qd, void *impl);
/** Set the agent */
void qd_dispatch_set_agent(qd_dispatch_t *qd, void *agent);
+/**
--- End diff --
I am wondering if the function should take ownership of the passed in pointer. I feel more comfortable if the function makes its own copy. Just my 2 cents.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #108: Fix memory leaks
Posted by ganeshmurthy <gi...@git.apache.org>.
Github user ganeshmurthy commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/108#discussion_r87621680
--- Diff: src/connection_manager.c ---
@@ -98,9 +98,13 @@ static void qd_server_config_free(qd_server_config_t *cf)
if (!cf) return;
free(cf->host);
free(cf->port);
- free(cf->name);
free(cf->role);
- free(cf->sasl_mechanisms);
+ if (cf->name) free(cf->name);
--- End diff --
This is a good change. It makes sure that it frees all the components of qd_server_config_t
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #108: Fix memory leaks
Posted by dskarbek <gi...@git.apache.org>.
Github user dskarbek commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/108#discussion_r86036444
--- Diff: src/router_core/router_core.c ---
@@ -106,29 +106,31 @@ void qdr_core_free(qdr_core_t *core)
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]);
}
}
- if (core->addr_hash) {
- qd_hash_free(core->addr_hash);
- }
- if (core->conn_id_hash) {
- qd_hash_free(core->conn_id_hash);
- }
- if (core->query_lock) {
- sys_mutex_free(core->query_lock);
- }
- 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);
+
+ qdr_address_t *addr = 0;
+ while ( (addr = DEQ_HEAD(core->addrs)) ) {
+ qdr_core_remove_address(core, addr);
}
- if (core->data_links_by_mask_bit) {
- free(core->data_links_by_mask_bit);
+ qdr_address_config_t *addr_config = 0;
+ while ( (addr_config = DEQ_HEAD(core->addr_config))) {
+ free_qdr_address_config_t(addr_config);
--- End diff --
this is a bug. I've got an update to the commit that fixes this which I haven't pushed yet.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #108: Fix memory leaks
Posted by dskarbek <gi...@git.apache.org>.
Github user dskarbek commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/108#discussion_r86644159
--- Diff: src/router_config.c ---
@@ -26,6 +26,27 @@
#include "entity_cache.h"
#include "schema_enum.h"
+static void qdi_router_configure_body(qdr_core_t *core,
+ qd_composed_field_t *body,
+ qd_router_entity_type_t type,
+ char *name)
+{
+ qd_buffer_list_t buffers;
+ qd_compose_take_buffers(body, &buffers);
+
+ qd_field_iterator_t *iter = qd_field_iterator_buffer(DEQ_HEAD(buffers), 0, qd_buffer_list_length(&buffers));
+ qd_parsed_field_t *in_body = qd_parse(iter);
+ qd_field_iterator_free(iter);
+
+ qd_field_iterator_t *name_iter = 0;
+ if (name)
+ name_iter = qd_field_iterator_string(name);
+
+ qdr_manage_create(core, 0, type, name_iter, in_body, 0, buffers);
--- End diff --
What do you think about passing the buffers through the action so that the action method can free them when it is done?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org