From 563ba9ea6e60774086555998b957edf923e24b46 Mon Sep 17 00:00:00 2001 From: Michal Schmidt Date: Mon, 17 Oct 2011 11:12:12 +0200 Subject: [PATCH] manager: fix a crash in isolating HASHMAP_FOREACH is safe against the removal of the current entry, but not against the removal of other entries. job_finish_and_invalidate() can recursively remove other entries. It triggered an assertion failure: Assertion 'j->installed' failed at src/manager.c:1218, function transaction_apply(). Aborting. Fix the crash by iterating from the beginning when there is a possibility that the iterator could be invalid. It is O(n^2) in the worst case, but that's better than a crash. https://bugzilla.redhat.com/show_bug.cgi?id=717325 --- src/job.c | 19 ++++++++++++++----- src/manager.c | 7 ++++++- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/src/job.c b/src/job.c index 5c0913b..20971da 100644 --- a/src/job.c +++ b/src/job.c @@ -527,6 +527,7 @@ int job_finish_and_invalidate(Job *j, JobResult result) { Unit *other; JobType t; Iterator i; + bool recursed = false; assert(j); assert(j->installed); @@ -573,23 +574,29 @@ int job_finish_and_invalidate(Job *j, JobResult result) { if (other->meta.job && (other->meta.job->type == JOB_START || other->meta.job->type == JOB_VERIFY_ACTIVE || - other->meta.job->type == JOB_RELOAD_OR_START)) + other->meta.job->type == JOB_RELOAD_OR_START)) { job_finish_and_invalidate(other->meta.job, JOB_DEPENDENCY); + recursed = true; + } SET_FOREACH(other, u->meta.dependencies[UNIT_BOUND_BY], i) if (other->meta.job && (other->meta.job->type == JOB_START || other->meta.job->type == JOB_VERIFY_ACTIVE || - other->meta.job->type == JOB_RELOAD_OR_START)) + other->meta.job->type == JOB_RELOAD_OR_START)) { job_finish_and_invalidate(other->meta.job, JOB_DEPENDENCY); + recursed = true; + } SET_FOREACH(other, u->meta.dependencies[UNIT_REQUIRED_BY_OVERRIDABLE], i) if (other->meta.job && !other->meta.job->override && (other->meta.job->type == JOB_START || other->meta.job->type == JOB_VERIFY_ACTIVE || - other->meta.job->type == JOB_RELOAD_OR_START)) + other->meta.job->type == JOB_RELOAD_OR_START)) { job_finish_and_invalidate(other->meta.job, JOB_DEPENDENCY); + recursed = true; + } } else if (t == JOB_STOP) { @@ -597,8 +604,10 @@ int job_finish_and_invalidate(Job *j, JobResult result) { if (other->meta.job && (other->meta.job->type == JOB_START || other->meta.job->type == JOB_VERIFY_ACTIVE || - other->meta.job->type == JOB_RELOAD_OR_START)) + other->meta.job->type == JOB_RELOAD_OR_START)) { job_finish_and_invalidate(other->meta.job, JOB_DEPENDENCY); + recursed = true; + } } } @@ -626,7 +635,7 @@ finish: manager_check_finished(u->meta.manager); - return 0; + return recursed; } int job_start_timer(Job *j) { diff --git a/src/manager.c b/src/manager.c index e626347..6d20258 100644 --- a/src/manager.c +++ b/src/manager.c @@ -1214,13 +1214,18 @@ static int transaction_apply(Manager *m, JobMode mode) { /* When isolating first kill all installed jobs which * aren't part of the new transaction */ + rescan: HASHMAP_FOREACH(j, m->jobs, i) { assert(j->installed); if (hashmap_get(m->transaction_jobs, j->unit)) continue; - job_finish_and_invalidate(j, JOB_CANCELED); + /* 'j' itself is safe to remove, but if other jobs + are invalidated recursively, our iterator may become + invalid and we need to start over. */ + if (job_finish_and_invalidate(j, JOB_CANCELED) > 0) + goto rescan; } } -- 1.7.3.4