Fix memory leak in print_string #27
Conversation
|
In our application we noticed a pretty severe memory leak. Each time we ran our yang validation code the memory usage would increase (26GB over 3 days). This fixes the issue however it seems to have failed some compilation step in the tests? |
|
@Ebulus7899 Wow. Good catch! It seems most usages of Would you mind fixing the other cases as well? I was thinking we could add a I'll update the CI to use the stable toolchain and get rid of those instabilities. |
|
No worries. I changed the code so the free is in the char_ptr_to_string function instead |
|
I think we need something like this: pub(crate) fn char_ptr_to_string(c_str: *const c_char, free: bool) -> String {
let string = unsafe { CStr::from_ptr(c_str).to_string_lossy().into_owned() };
if free {
unsafe { free(c_str as *mut std::ffi::c_void) };
}
string
}We do not always want to free the string passed as input, as it might not be heap-allocated in some cases (e.g. string returned by |
|
Just making sure. For the function char_ptr_to_opt_string. All current uses are not using the heap so should be set to false for now? |
src/data.rs
Outdated
| @@ -1259,7 +1259,7 @@ impl<'a> DataNodeRef<'a> { | |||
| panic!("Failed to generate path of the data node"); | |||
| } | |||
|
|
|||
| char_ptr_to_string(buf.as_ptr()) | |||
| char_ptr_to_string(buf.as_ptr(), true) | |||
There was a problem hiding this comment.
This should be false because we're passing a local buffer to lyd_path(). This is done for performance reasons.
src/schema.rs
Outdated
| @@ -576,7 +576,7 @@ impl<'a> SchemaNode<'a> { | |||
| panic!("Failed to generate path of the schema node"); | |||
| } | |||
|
|
|||
| char_ptr_to_string(buf.as_ptr()) | |||
| char_ptr_to_string(buf.as_ptr(), true) | |||
src/schema.rs
Outdated
| @@ -1247,7 +1247,7 @@ impl SchemaLeafType<'_> { | |||
| /// Returns the typedef name if it exists. | |||
| pub fn typedef_name(&self) -> Option<String> { | |||
| let typedef = unsafe { (*self.raw).name }; | |||
| char_ptr_to_opt_string(typedef) | |||
| char_ptr_to_opt_string(typedef, true) | |||
src/schema.rs
Outdated
| @@ -1292,7 +1292,7 @@ impl<'a> SchemaExtInstance<'a> { | |||
| /// Returns the optional extension's argument. | |||
| pub fn argument(&self) -> Option<String> { | |||
| let argument = unsafe { (*self.raw).argument }; | |||
| char_ptr_to_opt_string(argument) | |||
| char_ptr_to_opt_string(argument, true) | |||
src/error.rs
Outdated
| let apptag = unsafe { char_ptr_to_opt_string((*error).apptag) }; | ||
| let msg = unsafe { char_ptr_to_opt_string((*error).msg, true) }; | ||
| let path = unsafe { char_ptr_to_opt_string((*error).data_path, true) }; | ||
| let apptag = unsafe { char_ptr_to_opt_string((*error).apptag, true) }; |
There was a problem hiding this comment.
false for all three above, libyang will handle this internally
The cstr pointer is assigned memory inside the lyd_print_mem function however the print_string function returns without explicitly freeing this memory. Update the function to call char_ptr_to_string with a free boolean that when true will free the memory. Also updated the char_ptr_to_opt_string function to also take the free boolean as we do not want to free the memory for the lyd_value_get_canonical case
There was a problem hiding this comment.
@Ebulus7899 Thank you a lot! I'll publish a point release containing this fix now.
|
Thank you very much for your patience |
The cstr pointer is assigned memory inside the lyd_print_mem function however the print_string function returns without explicitly freeing this memory. Update the function to call char_ptr_to_string, free the cstr, and then pass result to Ok