memory leak issues #63

gaoethan · 2017-10-31T11:33:17Z

memory allocated from rcutils_format_string() is relatively easy
easy to forget to free and this leaks system resource. A wrapper
API added to make it shared and centralized, then fix those mem
leak issues using that API.

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

wjwwood

Thanks for finding and opening the pr to fix the memory leak here 👍!

I'm not sure this new function is necessary (maybe I'm wrong and you can explain why you think we need it), but if we keep it, then it needs to be a macro and not a function to preserve the file and line number capture the macro does at the original place the error state is set.

wjwwood · 2017-11-01T00:48:55Z

include/rcutils/error_handling.h

@@ -103,6 +103,22 @@ rcutils_set_error_state(
 #define RCUTILS_SET_ERROR_MSG(msg, allocator) \
  rcutils_set_error_state(msg, __FILE__, __LINE__, allocator);

+/// Set the formatted error msg and free its memory allocated


nitpick: no punctuation

wjwwood · 2017-11-01T00:50:36Z

include/rcutils/error_handling.h

@@ -103,6 +103,22 @@ rcutils_set_error_state(
 #define RCUTILS_SET_ERROR_MSG(msg, allocator) \
  rcutils_set_error_state(msg, __FILE__, __LINE__, allocator);

+/// Set the formatted error msg and free its memory allocated
+/**
+ * This function sets the error msg with the format specified and free the 


This implies, at least to me, that a format string is given and extra arguments are expected to generate a string, a la printf, but that's no the case here. I think you mean something like "allocated string" or "non-literal" string or "string that should be deallocated after use".

wjwwood · 2017-11-01T00:51:02Z

include/rcutils/error_handling.h

+/**
+ * This function sets the error msg with the format specified and free the 
+ * corresponding memory allocated from function rcutils_format_string_limit() 
+ * or macro rcutils_format_string drived from it to avoid memory leak.


I think that should be the responsibility of the person who calls rcutils_format_string() or rcutils_format_string_limit().

Why do we need a function here to do that. What if someone uses snprintf or just did malloc and memcpy to create a message string? Should those get functions too? (I would say no)

wjwwood · 2017-11-01T00:53:08Z

src/error_handling.c

+    return false;
+  }
+
+  RCUTILS_SET_ERROR_MSG(formatted_msg, allocator);


Because you implemented this as a function rather than a maco, (and then call the macro here) the error messages that use this will always get a file and line number here, rather than where it was called from. This isn't correct imo, so this would need to be implemented as a macro instead.

wjwwood · 2017-11-01T00:53:34Z

src/error_handling.c

+  RCUTILS_SET_ERROR_MSG(formatted_msg, allocator);
+
+  allocator.deallocate(formatted_msg, allocator.state);
+  formatted_msg = nullptr;


Cannot use nullptr in C.

exactly, it's not in C++ and I'll revise it, thanks.

wjwwood · 2017-11-01T00:54:30Z

src/split.c

@@ -115,7 +115,7 @@ rcutils_split(
  if (rcutils_string_array_fini(string_array) != RCUTILS_RET_OK) {
    error_msg = rcutils_format_string(allocator, "FATAL: %s. Leaking memory", error_msg);
  }
-  RCUTILS_SET_ERROR_MSG(error_msg, allocator);
+  rcutils_set_formatted_error(error_msg, allocator);


This is a memory leak and should be fixed, but again, I don't think a special function is required here. Also this change will change the file and line number associated with this error state.

wjwwood · 2017-11-01T00:54:45Z

src/string_map.c

@@ -354,8 +354,7 @@ rcutils_string_map_unset(rcutils_string_map_t * string_map, const char * key)
  size_t key_index;
  if (!__get_index_of_key_if_exists(string_map->impl, key, strlen(key), &key_index)) {
    char * msg = rcutils_format_string(allocator, "key '%s' not found", key);
-    RCUTILS_SET_ERROR_MSG(msg, allocator)
-    allocator.deallocate(msg, allocator.state);
+    rcutils_set_formatted_error(msg, allocator);


This was ok before right?

it doesn't check whether the memory allocated to msg is successful or not, just image that msg is NULL or a certain invalid memory address, and then to operate at that address.

Makes sense.

gaoethan · 2017-11-01T04:10:45Z

@wjwwood agree with you to implement it as a macro. and let me explain why I add that new function: actually I want to directly modify those issues, but I found that these issues are not unique and similar; moreover, it's really prone to forget to free the memory from rcutils_format_string() and I hope a helper API may benefit this.

I've ever considered to add one API to address this in format_string.h to implement current API content:
check whether the memory allocated from rcutils_format_string() is available or not
do the original action RCUTILS_SET_ERROR_MSG
free the memory allocated after using
in this way it will involve cross reference btw format_string.h and error_handling.h, I think it doesn't make sense for format_string.h, but adding it in error_handling.h, it will add a helper API for the case which needs to set error requiring string in a specific format with rcutils_format_string and doesn't impact the format_string.h yet.

anyway, I think this helper API (now should be helper macro) is not mandatory, please weight it and give your comments. thanks.

wjwwood · 2017-11-01T23:43:59Z

I would be fine with fixing the memory leaks (or potential null access) without a new function.

Or I would be fine with a new macro to help with this more common case, if it:

is a macro and therefore preserves the file and line number of the calling location
it does the formatting as well as error setting

What I mean by the second point is something like this:

RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING(
  allocator,
  "key '%s' not found", key);
return SOME_ERROR_CODE;

So that it creates and uses the format string together. I would prefer that to having the user create a string, give it to the macro and let it deallocate it. I would prefer to have the same actor allocate and deallocate if possible, so either the user does both or the function does both, but not a mixture.

Instead of code that used to do something like this:

char * msg = rcutils_format_string(allocator, "key '%s' not found", key);
if (!msg) {
  SAFE_WRITE_STDERR("could not allocate memory for error message\n");
} else {
  RCUTILS_SET_ERROR_MSG(msg, allocator);
}
allocator.deallocate(msg, allocator.state);
return SOME_ERROR_CODE;

gaoethan · 2017-11-02T11:15:37Z

@wjwwood I propose not to add return SOME_ERROR_CODE to the macro definition, because it changes the flow of the code using it in a sense, moreover, the return code is also different in some components. no return will make it more flexible and common to use. thanks.

wjwwood · 2017-11-06T21:36:56Z

@wjwwood I propose not to add return SOME_ERROR_CODE to the macro definition, because it changes the flow of the code using it in a sense, moreover, the return code is also different in some components.

Yeah, that's what I proposed too:

#63 (comment)

What I mean by the second point is something like this:

RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING(
  allocator,
  "key '%s' not found", key);
return SOME_ERROR_CODE;

The return statement is outside of the macro.

wjwwood · 2017-11-06T21:37:35Z

include/rcutils/error_handling.h

@@ -23,6 +23,8 @@ extern "C"
 #endif

 #include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
 #include <stddef.h>


nitpick: please alphabetically order the includes

wjwwood · 2017-11-06T21:48:22Z

src/split.c

  }
-  RCUTILS_SET_ERROR_MSG(error_msg, allocator);


This doesn't look right, when will line 114's message be printed? (the case where finalizing the string array succeeds)

I actually keep the original without further checking :), yes, it's not reasonable and gets it tweaked, thanks.

wjwwood · 2017-11-06T21:49:40Z

include/rcutils/error_handling.h

@@ -39,6 +41,8 @@ typedef struct rcutils_error_state_t
  rcutils_allocator_t allocator;
 } rcutils_error_state_t;

+#define SAFE_FWRITE_TO_STDERR(msg) fwrite(msg, sizeof(char), sizeof(msg), stderr)


Since this is a public header, this needs to be namespaced, e.g. RCUTILS_SAFE_FWRITE_TO_STDERR or something like that.

wjwwood · 2017-11-07T16:09:18Z

include/rcutils/error_handling.h

+    allocator.deallocate(output_msg, allocator.state); \
+  } else { \
+    RCUTILS_SAFE_FWRITE_TO_STDERR("Failed to allocate memory for error message"); \
+  }


This should probably be put into a scope, i.e. within something like {} or do {} while (0), so that you can use this macro more than once in a function without colliding symbol definitions, e.g. char * output_msg.

Sorry I didn't see this before.

@wjwwood exactly, good catch 👍

wjwwood · 2017-11-07T16:17:43Z

src/split.c

-  }
-  RCUTILS_SET_ERROR_MSG(error_msg, allocator);
+  RCUTILS_SET_ERROR_MSG("unable to allocate memory for string array data", allocator);
+  rcutils_string_array_fini(string_array);


I don't think this is a good idea to not check the return value. If it is not RCUTILS_RET_OK then it will have set the error message internally, overwriting the error message set one line above.

I'd do something like this:

if (rcutils_string_array_fini(string_array) != RCUTILS_RET_OK) { RCUTILS_SAFE_FWRITE_TO_STDERR("failed to finalize string array during error handling: "); RCUTILS_SAFE_FWRITE_TO_STDERR(rcutils_get_error_string_safe()); RCUTILS_SAFE_FWRITE_TO_STDERR("\n"); rcutils_reset_error(); } RCUTILS_SET_ERROR_MSG("unable to allocate memory for string array data", allocator);

wjwwood · 2017-11-07T16:18:02Z

include/rcutils/error_handling.h

+    RCUTILS_SET_ERROR_MSG(output_msg, allocator); \
+    allocator.deallocate(output_msg, allocator.state); \
+  } else { \
+    RCUTILS_SAFE_FWRITE_TO_STDERR("Failed to allocate memory for error message"); \


I think this string needs a terminating \n.

wjwwood

changes lgtm, but I think the style needs to be fixed up, I'll do that for you, but for future reference I'd recommend using ament_uncrustify path/to/repo --reformat or at least running the test suite for the package before pushing for review.

wjwwood · 2017-11-10T19:25:08Z

I started some CI for this pr:

Linux
Linux-aarch64
macOS
Windows

dhood · 2017-11-10T20:00:16Z

I added 536ae0e since I've seen ros/ros_comm#1179

memory allocated from rcutils_format_string() is relatively easy easy to forget to free and this leaks system resource. A wrapper API added to make it shared and centralized Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>

define the macro RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING to help format error string and fix those memory leak issues with this marco Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>

If the `goto fail;` at a certain allocation failure in the middle, just to free those memory before it in `rcutils_string_array_fini()`, those after that failure is invalid

wjwwood · 2017-11-10T20:25:11Z

Thanks @dhood, needs to rebased anyways:

Linux
Linux-aarch64
macOS
Windows

mikaelarguedas · 2017-11-10T23:15:34Z

I reapplied 536ae0e that disappeared during rebase

gaoethan · 2017-11-13T02:12:09Z

@wjwwood @dhood @mikaelarguedas thanks for your contribution.

BTW, @wjwwood I'll run ament_uncrustify to format code, but just wanna double confirm with you for

at least running the test suite for the package before pushing for review.

Do you mean "ament build --build-tests" or something else ?

wjwwood · 2017-11-13T19:17:00Z

at least running the test suite for the package before pushing for review.

Do you mean "ament build --build-tests" or something else ?

I mean with ament test (ament build --build-tests only builds the tests). I usually do this:

ament test --skip-build --skip-install --only <package I want to test>

This prevents ament test from trying to rebuild or reinstall any of the packages being tested and therefore avoids any issues with mismatched options.

This command basically just runs CTest, so you could do that too in the build folder of the package you're working on (assuming it is a cmake package).

wjwwood · 2017-11-13T19:44:41Z

I should have just linked to our docs: https://github.com/ros2/ros2/wiki/Ament-Tutorial#run-the-tests

wjwwood · 2017-11-13T21:25:36Z

Ok, finally got a Windows job to finish 😄.

gaoethan mentioned this pull request Oct 31, 2017

memory leak issues ros2/rcl#171

Closed

wjwwood requested changes Nov 1, 2017

View reviewed changes

wjwwood added the in review Waiting for review (Kanban column) label Nov 1, 2017

wjwwood requested changes Nov 6, 2017

View reviewed changes

wjwwood reviewed Nov 6, 2017

View reviewed changes

wjwwood reviewed Nov 7, 2017

View reviewed changes

wjwwood approved these changes Nov 10, 2017

View reviewed changes

dhood mentioned this pull request Nov 10, 2017

Use RCUTILS_SET_ERROR_MSG in logging config #65

Merged

gaoethan and others added 12 commits November 10, 2017 12:24

* memory leak issues

387db0d

memory allocated from rcutils_format_string() is relatively easy easy to forget to free and this leaks system resource. A wrapper API added to make it shared and centralized Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>

* Define a helper MACRO and fix memory leak with it

2e12cd7

define the macro RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING to help format error string and fix those memory leak issues with this marco Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>

fixup documentation and variable names

0a034e5

adjust header file and add marco namespace

328d3eb

avoid unknown free to invalid address

4e0d7ad

If the `goto fail;` at a certain allocation failure in the middle, just to free those memory before it in `rcutils_string_array_fini()`, those after that failure is invalid

tweak marco definition to be safer while using

ea35b67

minor grammar fix

0428dc0

write out error in rcutils_string_array_fini

ad422b0

typo

1ec7d92

missing continuation in macro

caf12a0

uncrustify

351b248

remove unused variable

3647147

wjwwood force-pushed the formatstr_memleak branch from 536ae0e to 3647147 Compare November 10, 2017 21:16

while (0) -> while (false) reapplied from 536ae0e

6275f39

wjwwood merged commit 954ae9b into ros2:master Nov 13, 2017

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

gaoethan deleted the formatstr_memleak branch November 16, 2017 06:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

memory leak issues #63

memory leak issues #63

gaoethan commented Oct 31, 2017

wjwwood left a comment

wjwwood Nov 1, 2017

wjwwood Nov 1, 2017

wjwwood Nov 1, 2017

wjwwood Nov 1, 2017

wjwwood Nov 1, 2017

gaoethan Nov 1, 2017

wjwwood Nov 1, 2017

wjwwood Nov 1, 2017

gaoethan Nov 1, 2017

wjwwood Nov 1, 2017

gaoethan commented Nov 1, 2017

wjwwood commented Nov 1, 2017

gaoethan commented Nov 2, 2017

wjwwood commented Nov 6, 2017

wjwwood Nov 6, 2017

wjwwood Nov 6, 2017

gaoethan Nov 7, 2017

wjwwood Nov 6, 2017

wjwwood Nov 7, 2017

wjwwood Nov 7, 2017

gaoethan Nov 8, 2017

wjwwood Nov 7, 2017

wjwwood Nov 7, 2017

wjwwood left a comment

wjwwood commented Nov 10, 2017

dhood commented Nov 10, 2017

wjwwood commented Nov 10, 2017 •

edited

Loading

mikaelarguedas commented Nov 10, 2017

gaoethan commented Nov 13, 2017

wjwwood commented Nov 13, 2017

wjwwood commented Nov 13, 2017

wjwwood commented Nov 13, 2017

memory leak issues #63

memory leak issues #63

Conversation

gaoethan commented Oct 31, 2017

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaoethan commented Nov 1, 2017

wjwwood commented Nov 1, 2017

gaoethan commented Nov 2, 2017

wjwwood commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwwood left a comment

Choose a reason for hiding this comment

wjwwood commented Nov 10, 2017

dhood commented Nov 10, 2017

wjwwood commented Nov 10, 2017 • edited Loading

mikaelarguedas commented Nov 10, 2017

gaoethan commented Nov 13, 2017

wjwwood commented Nov 13, 2017

wjwwood commented Nov 13, 2017

wjwwood commented Nov 13, 2017

wjwwood commented Nov 10, 2017 •

edited

Loading