From c49dfd8911f49bf383ad7ac1d18be8a4ab39ab09 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 26 Apr 2023 13:59:34 +0900 Subject: [PATCH 1/5] core/transaction: use hashmap_remove_value() to make not remove job with same ID When we fail to deserialize job ID, or the current_job_id is overflowed, we may have jobs with the same ID. This is paranoia, and just for safety. Note, we already use hashmap_remove_value() in job_uninstall(). --- src/core/transaction.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/transaction.c b/src/core/transaction.c index 059558ebc0..cd0c28123f 100644 --- a/src/core/transaction.c +++ b/src/core/transaction.c @@ -662,7 +662,7 @@ static int transaction_apply( /* j has been merged into a previously installed job */ if (tr->anchor_job == j) tr->anchor_job = installed_job; - hashmap_remove(m->jobs, UINT32_TO_PTR(j->id)); + hashmap_remove_value(m->jobs, UINT32_TO_PTR(j->id), j); job_free(j); j = installed_job; } @@ -683,7 +683,7 @@ static int transaction_apply( rollback: HASHMAP_FOREACH(j, tr->jobs) - hashmap_remove(m->jobs, UINT32_TO_PTR(j->id)); + hashmap_remove_value(m->jobs, UINT32_TO_PTR(j->id), j); return r; } From 6ff134004d46079f1b515bdb23be256cfe829541 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 26 Apr 2023 14:23:12 +0900 Subject: [PATCH 2/5] core/job: add one more assertion In this stage, the unit should not have installed job of the type. --- src/core/job.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/core/job.c b/src/core/job.c index 826ef6809c..af47987938 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -237,6 +237,7 @@ Job* job_install(Job *j) { } /* Install the job */ + assert(!*pj); *pj = j; j->installed = true; From ace15701c1d596913045df8306ead63adef71be9 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 26 Apr 2023 14:24:12 +0900 Subject: [PATCH 3/5] core/job: fix indentation --- src/core/job.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/job.c b/src/core/job.c index af47987938..1010e6f869 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -262,8 +262,8 @@ int job_install_deserialized(Job *j) { if (j->type < 0 || j->type >= _JOB_TYPE_MAX_IN_TRANSACTION) return log_unit_debug_errno(j->unit, SYNTHETIC_ERRNO(EINVAL), - "Invalid job type %s in deserialization.", - strna(job_type_to_string(j->type))); + "Invalid job type %s in deserialization.", + strna(job_type_to_string(j->type))); pj = j->type == JOB_NOP ? &j->unit->nop_job : &j->unit->job; if (*pj) From 3e1aec303646c37e7a7c74b45ef8cc90e3bf3531 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 26 Apr 2023 14:37:26 +0900 Subject: [PATCH 4/5] core/job: handle job ID overflow or conflict more sanely This is paranoia, and just for safety. Should not change any behavior. --- src/core/job.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/src/core/job.c b/src/core/job.c index 1010e6f869..43a06a365e 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -48,6 +48,28 @@ Job* job_new_raw(Unit *unit) { return j; } +static uint32_t manager_get_new_job_id(Manager *m) { + bool overflow = false; + + assert(m); + + for (;;) { + uint32_t id = m->current_job_id; + + if (_unlikely_(id == UINT32_MAX)) { + assert_se(!overflow); + m->current_job_id = 1; + overflow = true; + } else + m->current_job_id++; + + if (hashmap_contains(m->jobs, UINT32_TO_PTR(id))) + continue; + + return id; + } +} + Job* job_new(Unit *unit, JobType type) { Job *j; @@ -57,7 +79,7 @@ Job* job_new(Unit *unit, JobType type) { if (!j) return NULL; - j->id = j->manager->current_job_id++; + j->id = manager_get_new_job_id(j->manager); j->type = type; /* We don't link it here, that's what job_dependency() is for */ From aafb96d5146c70ac30f2b5c677cd22fd43abd62c Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 26 Apr 2023 14:39:56 +0900 Subject: [PATCH 5/5] core/job: use new job ID when we failed to deserialize job ID This is for the case when we fail to deserialize job ID. In job_install_deserialized(), we also check the job type, and that is for the case when we failed to deserialize the job. Let's gracefully handle the failure in deserializing the job ID. This is paranoia, and just for safety. Should not change any behavior. --- src/core/job.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/core/job.c b/src/core/job.c index 43a06a365e..24f407ad83 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -292,6 +292,10 @@ int job_install_deserialized(Job *j) { return log_unit_debug_errno(j->unit, SYNTHETIC_ERRNO(EEXIST), "Unit already has a job installed. Not installing deserialized job."); + /* When the job does not have ID, or we failed to deserialize the job ID, then use a new ID. */ + if (j->id <= 0) + j->id = manager_get_new_job_id(j->manager); + r = hashmap_ensure_put(&j->manager->jobs, NULL, UINT32_TO_PTR(j->id), j); if (r == -EEXIST) return log_unit_debug_errno(j->unit, r, "Job ID %" PRIu32 " already used, cannot deserialize job.", j->id);