Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory issues with pcmk__schedule_actions() #3810

Merged
merged 19 commits into from
Feb 7, 2025
Merged
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
a90267c
Refactor: various: Use uint32_t for group of pcmk_sim_flags internally
nrwahl2 Jan 24, 2025
ff6e7a9
Refactor: fencer: Set scheduler input, flags, and status explicitly
nrwahl2 Jan 24, 2025
37f7210
Refactor: scheduler: Set scheduler input, flags, and status explicitly
nrwahl2 Jan 24, 2025
625672d
Fix: tools: Avoid crash in crm_simulate --profile
nrwahl2 Jan 24, 2025
8b9caad
Low: libpacemaker: Fix mem leak in pcmk__profile_dir()
nrwahl2 Jan 24, 2025
a1d8655
Refactor: libpacemaker: Create scheduler object in pcmk__profile_dir()
nrwahl2 Jan 24, 2025
481b0dd
Low: libpacemaker: Handle scandir() error in pcmk__profile_dir()
nrwahl2 Jan 24, 2025
64281ae
Refactor: libpacemaker: Best practices for pcmk__profile_dir()
nrwahl2 Jan 24, 2025
9f1edd8
Refactor: libpacemaker: Use scandir() filter in pcmk__profile_dir()
nrwahl2 Jan 24, 2025
3fca1ba
Low: tools: Fix overflow in crm_simulate --repeat
nrwahl2 Jan 25, 2025
767c55f
Refactor: libpacemaker: Pass NULL input in pcmk__simulate()
nrwahl2 Jan 25, 2025
27d519f
Refactor: libpacemaker: Set input and flags explicitly in pcmk__verify()
nrwahl2 Jan 25, 2025
eae8f5b
Refactor: libpacemaker: Don't make unnecessary copy for scheduler input
nrwahl2 Jan 26, 2025
00b82bd
Refactor: tools: update_dataset() passes nulls to pcmk__schedule_actions
nrwahl2 Jan 25, 2025
ae63ed5
Refactor: tools: Set scheduler flags explicitly in wait_till_stable()
nrwahl2 Jan 25, 2025
4a3f90b
Refactor: libpacemaker: Drop unused args from pcmk__schedule_actions()
nrwahl2 Jan 25, 2025
5dbc819
Fix: libpe_status: Make cluster_status() idempotent
nrwahl2 Jan 25, 2025
f280506
Refactor: libpacemaker: Unpack status in pcmk__schedule_actions()
nrwahl2 Jan 25, 2025
b1ac2a5
Feature: libpacemaker: Reset scheduler object in pcmk_simulate()
nrwahl2 Jan 25, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Low: libpacemaker: Fix mem leak in pcmk__profile_dir()
Previously, we leaked cib_object in profile_file() if repeat > 1.

Currently the only caller is crm_simulate, and we know the scheduler
object has no relevant state at call time. Thus we can reset the
scheduler before doing the simulation.

We can avoid unnecessary copies and special case handling by setting
scheduler->input to cib_object and then setting it back to NULL before
resetting the scheduler.

Then we can make sure cib_object is freed unconditionally at the end.

Signed-off-by: Reid Wahl <nrwahl@protonmail.com>
nrwahl2 committed Jan 31, 2025
commit 8b9caadbdfe7063ef3d15e54a5d8eca7dcdf8137
20 changes: 10 additions & 10 deletions lib/pacemaker/pcmk_simulate.c
Original file line number Diff line number Diff line change
@@ -345,13 +345,11 @@ profile_file(const char *xml_file, long long repeat,
}

if (pcmk__update_configured_schema(&cib_object, false) != pcmk_rc_ok) {
pcmk__xml_free(cib_object);
return;
goto done;
}

if (!pcmk__validate_xml(cib_object, NULL, NULL, NULL)) {
pcmk__xml_free(cib_object);
return;
goto done;
}

if (pcmk_is_set(scheduler->flags, pcmk__sched_output_scores)) {
@@ -362,22 +360,24 @@ profile_file(const char *xml_file, long long repeat,
}

for (int i = 0; i < repeat; ++i) {
xmlNode *input = cib_object;

if (repeat > 1) {
input = pcmk__xml_copy(NULL, cib_object);
}
pcmk_reset_scheduler(scheduler);
scheduler->input = input;

scheduler->input = cib_object;
pcmk__set_scheduler_flags(scheduler, scheduler_flags);
set_effective_date(scheduler, false, use_date);
cluster_status(scheduler);
pcmk__schedule_actions(NULL, pcmk__sched_none, scheduler);

// Avoid freeing cib_object in pcmk_reset_scheduler()
scheduler->input = NULL;
}

pcmk_reset_scheduler(scheduler);
end = clock();
out->message(out, "profile", xml_file, start, end);

done:
pcmk__xml_free(cib_object);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could split this out into its own PR as well to make potential backporting easier, but it's only ever going to affect crm_simulate, and I don't think that's really worth the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can check if it's trivial or not. Currently I've got it ordered after the the crash fix. I did that because right now on main, the mem leak is not reproducible (and thus the fix is not testable) until the crash is fixed. Meanwhile, the crash isn't present on 3.0.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But yeah to your point I don't see how it could affect anything besides crm_simulate so why bother. It's not currently exposed via any public API, and if it ever is, then it'll be on a Pacemaker version that has this fix.

}

void