cfg_dupopt_array fixes #131
Conversation
This prevents double-free when cleaning up. Signed-off-by: Peter Rosin <peda@axentia.se>
|
I guess we should combine this PR with the proposed fix to a memory leak mentioned in #129. Would you mind having a look at that too? Other than that I have no objections, clean fixes, separated in logical commits 👍 |
|
They could be combined, but that's seems like a separate issue. Related perhaps, but separate. Maybe you're asking me to extend this pull request? However, I'm not sure what email address "doublex" wants to use as git author, and doing so would invalidate my branch name when that comment leak is in the "wrong" function. I'll add a comment to #129 with my take on that issue though... |
Signed-off-by: Peter Rosin <peda@axentia.se>
CFG_PTR_CB makes use of the def.parsed field, but that field is only duplicated for lists and functions. Since cfg_free_opt_array() frees all fields unconditionally, this patch matches that and takes the safe route by duplicating all fields, regardless of option flags and option type. The fields that are duplicated unconditionally should be empty if they are not used, so there should be no memory thrown away by duplicating stuff that is not needed. Signed-off-by: Peter Rosin <peda@axentia.se>
Signed-off-by: Peter Rosin <peda@axentia.se>
Without copies of ptrs to the old dynamically allocated memory from the new opt array, it is now possible to avoid ternary operators and double tests, and thus get more idiomatic code that is still reasonably compact. Signed-off-by: Peter Rosin <peda@axentia.se>
4d045b2 to
9666f8d
Compare
|
Ok, I realized that my patch "In cfg_dupopt_array, avoid trashing the original on failure" actually interfered with CFG_PTR_CB handling, but when I dug into that I found serious issues with CFG_PTR_CB. So, I inserted a test and a fix for that and force-pushed after an additional patch simplifying cfg_dupopt_array() a bit. Reordering things like that made the unexpected interference between my original patch and CFG_PTR_CB vanish. |
|
Looks good, merging! |
I noticed that comments where not strdup-ed, which would lead to use -after-free and eventually a double-free, if/when both the copy and the original try to free the same comment. I think this is perhaps not normally a problem because comments are not used heavily?
The second patch similarly prevents use-after-free and double-free if the dupopt operation fails in the middle. Failure should be rare though.