Fix potential use-after-free and double-free#4044
Fix potential use-after-free and double-free#4044dyupina wants to merge 1 commit intoClusterLabs:mainfrom
Conversation
|
Can one of the project admins check and authorise this run please: https://ci.kronosnet.org/job/pacemaker/job/pacemaker-pipeline/job/PR-4044/1/input |
lib/common/output.c
Outdated
|
|
||
| if ((*out)->init(*out) == false) { | ||
| pcmk__output_free(*out); | ||
| *out = NULL; |
There was a problem hiding this comment.
Thanks for the patch.
Instead, please use g_clear_pointer(out, pcmk__output_free) here. This is a relatively new thing we're starting to do, which is why you might not have run across it before. You'll also need to #include <glib.h> and update the copyright date to run through 2026. Thanks!
There was a problem hiding this comment.
Should I also replace the highlighted lines from the code below with g_clear_pointer(out, pcmk__output_free) ?
if (pcmk__str_eq(filename, "-", pcmk__str_null_matches)) {
(*out)->dest = stdout;
} else {
(*out)->dest = fopen(filename, "w");
if ((*out)->dest == NULL) {
pcmk__output_free(*out); // <<<<<
*out = NULL; // <<<<<
return errno;
}
}
There was a problem hiding this comment.
Yes, exactly. Those two lines go away and g_clear_pointer gets used instead.
In pcmk__output_new() -> pcmk__bare_output_new(), when the condition if ((*out)->init(*out) == false) is met, pcmk__output_free(*out) is called and the function returns with rc = ENOMEM. The *_init() functions return false when memory allocation via calloc() fails - an unlikely but possible scenario. Since rc != pcmk_rc_ok, execution jumps to the done label after pcmk__output_new() returns. The codebase contains numerous functions that follow the same cleanup pattern: they check if (out != NULL) at the done label before invoking out->finish() and pcmk__output_free(out). Because *out is freed but not set to NULL on initialization failure, this check evaluates to true. This leads to: - Use-after-free: dereferencing the dangling pointer when calling out->finish(...); - Double-free: attempting to free the same memory again via pcmk__output_free(out). To resolve this, use g_clear_pointer(out, pcmk__output_free) within pcmk__bare_output_new(). This ensures that if (out != NULL) check at the done label is false, thereby preventing both the use-after-free and the double-free. Also update the copyright date. Signed-off-by: Alexandra Diupina <[email protected]>
bf8dc11 to
82ea8b9
Compare
|
Can one of the project admins check and authorise this run please: https://ci.kronosnet.org/job/pacemaker/job/pacemaker-pipeline/job/PR-4044/2/input |
In pcmk__output_new() -> pcmk__bare_output_new(), when the condition if ((*out)->init(*out) == false) is met, pcmk__output_free(*out) is called and the function returns with rc = ENOMEM.
The *_init() functions return false when memory allocation via calloc() fails - an unlikely but possible scenario.
Since rc != pcmk_rc_ok, execution jumps to the done label after pcmk__output_new() returns.
The codebase contains numerous functions that follow the same cleanup pattern: they check if (out != NULL) at the done label before invoking out->finish() and pcmk__output_free(out). Because *out is freed but not set to NULL on initialization failure, this check evaluates to true.
This leads to:
To resolve this, set *out = NULL immediately after calling pcmk__output_free(*out) within pcmk__bare_output_new(). This ensures that if (out != NULL) check at the done label is false, thereby preventing both the use-after-free and the double-free.