Out-of-scope memory use and memory leak issue #172

gaoethan · 2017-11-01T10:32:44Z

temp going out of scope leaks the storage it points to
resource leak from local_namespace_

Signed-off-by: Ethan Gao ethan.gao@linux.intel.com

temp going out of scope leaks the storage it points to resource leak from local_namespace_ Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>

wjwwood · 2017-11-02T00:35:52Z

rcl/src/rcl/node.c

@@ -163,6 +165,7 @@ rcl_node_init(
  ret = rmw_validate_namespace(local_namespace_, &validation_result, NULL);
  if (ret != RMW_RET_OK) {
    RCL_SET_ERROR_MSG(rmw_get_error_string_safe(), *allocator);
+    allocator->deallocate((char *)local_namespace_, allocator->state);


This cannot be done if line 147 is executed (if local_namespace_ is a literal string). I think it needs to check should_free_local_namespace_.

Here and below.

exactly, thanks !

wjwwood

lgtm now, I'll run CI for you:

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>

wjwwood · 2017-11-06T21:33:16Z

Thanks @gaoethan!

dirk-thomas · 2017-11-06T22:02:51Z

I don't think the scope of the variable needs to be increased. temp is being used to avoid changing local_namespace_ in case of errors. As long as local_namespace_ is being freed when should_free_local_namespace_ is being set (which is part of this PR) this block should not leak memory anymore.

The scope of temp doesn't affect when it is being cleaned up (since it is a bare pointer) and the variable is not used anywhere below. Therefore I think the scope of temp should be reverted to be local as it was before (keeping the new deallocate calls of course).

wjwwood · 2017-11-06T22:36:35Z

You’re right, I’ll make a follow up pr ASAP.

follow up to #172

* out-of-scope memory use and memory leak issue

48334db

temp going out of scope leaks the storage it points to resource leak from local_namespace_ Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>

wjwwood requested changes Nov 2, 2017

View reviewed changes

wjwwood approved these changes Nov 2, 2017

View reviewed changes

exclude the memory free when the namespace given is null

8f70c14

Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>

dhood added the in review Waiting for review (Kanban column) label Nov 2, 2017

wjwwood merged commit f7d7df6 into ros2:master Nov 6, 2017

wjwwood removed the in review Waiting for review (Kanban column) label Nov 6, 2017

wjwwood added a commit that referenced this pull request Nov 7, 2017

re-reduce scope of temp variable

fa239c5

follow up to #172

wjwwood mentioned this pull request Nov 7, 2017

re-reduce scope of temp variable #175

Merged

wjwwood added a commit that referenced this pull request Nov 8, 2017

re-reduce scope of temp variable (#175)

ffcfc34

follow up to #172

gaoethan deleted the fix_node branch November 28, 2017 03:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Out-of-scope memory use and memory leak issue #172

Out-of-scope memory use and memory leak issue #172

gaoethan commented Nov 1, 2017

wjwwood Nov 2, 2017

gaoethan Nov 2, 2017

wjwwood left a comment

wjwwood commented Nov 6, 2017

dirk-thomas commented Nov 6, 2017 •

edited

Loading

wjwwood commented Nov 6, 2017

Out-of-scope memory use and memory leak issue #172

Out-of-scope memory use and memory leak issue #172

Conversation

gaoethan commented Nov 1, 2017

wjwwood Nov 2, 2017

Choose a reason for hiding this comment

gaoethan Nov 2, 2017

Choose a reason for hiding this comment

wjwwood left a comment

Choose a reason for hiding this comment

wjwwood commented Nov 6, 2017

dirk-thomas commented Nov 6, 2017 • edited Loading

wjwwood commented Nov 6, 2017

dirk-thomas commented Nov 6, 2017 •

edited

Loading