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

(#1729228) fix garbage collection of units with cyclic references #30

Merged
merged 7 commits into from Jul 12, 2019

Conversation

@dtardon
Copy link
Contributor

dtardon commented Jul 12, 2019

No description provided.

poettering and others added 7 commits Sep 29, 2015
…are located in

We place the processes we fork off in the cgroup anyway, and we probably
shouldn't be able to get that far if we couldn't set up the slice due to
resource problems or unmet conditions. Hence upgrade the dependency
between units and the slices they are located in from Wants= to
Requires=.

(cherry picked from commit 8c8da0e)
(cherry picked from commit 0600681)

Related: #1729228
Let's make sure when we drop a reference to a unit, that we run the GC queue on
it again.

This (together with the previous commit) should deal with the GC issues pointed
out in:

systemd/systemd#2993 (comment)

(cherry picked from commit b75102e)
(cherry picked from commit 0ace071)

Related: #1729228
"check" is unclear: what is true, what is false? Let's rename to "can_gc" and
revert the return value ("positive" values are easier to grok).

v2:
- rename from unit_can_gc to unit_may_gc

(cherry picked from commit f2f725e)
(cherry picked from commit 1716821)

Related: #1729228
No functional change.

The source unit manages the reference. It allocates the UnitRef structure and
registers it in the target unit, and then the reference must be destroyed
before the source unit is destroyed. Thus, is should be OK to include the
pointer to the source unit, it should be live as long as the reference exists.

v2:
- rename refs to refs_by_target

(cherry picked from commit 7f7d01e)
(cherry picked from commit ff80bfd)

Related: #1729228
A .socket will reference a .service unit, by registering a UnitRef with the
.service unit. If this .service unit has the .socket unit listed in Wants or
Sockets or such, a cycle will be created. We would not free this cycle
properly, because we treated any unit with non-empty refs as uncollectable. To
solve this issue, treats refs with UnitRef in u->refs_by_target similarly to
the refs in u->dependencies, and check if the "other" unit is known to be
needed. If it is not needed, do not treat the reference from it as preventing
the unit we are looking at from being freed.

(cherry picked from commit 2641f02)
(cherry picked from commit 703cc49)

Resolves: #1729228
… unit

We would free stuff like the names of the unit first, and then recurse
into other structures to remove the unit from there. Technically this
was OK, since the code did not access the name, but this makes debugging
harder. And if any log messages are added in any of those functions, they
are likely to access u->id and such other basic information about the unit.
So let's move the removal of this "basic" information towards the end
of unit_free().

(cherry picked from commit a946fa9)
(cherry picked from commit 42a93a8)

Related: #1729228
…al cleanup

When various references to the unit were dropped during cleanup in unit_free(),
add_to_gc_queue() could be called on this unit. If the unit was previously in
the gc queue (at the time when unit_free() was called on it), this wouldn't
matter, because it'd have in_gc_queue still set even though it was already
removed from the queue. But if it wasn't set, then the unit could be added to
the queue. Then after unit_free() would deallocate the unit, we would be left
with a dangling pointer in gc_queue.

A unit could be added to the gc queue in two places called from unit_free():
in the job_install calls, and in unit_ref_unset(). The first was OK, because
it was above the LIST_REMOVE(gc_queue,...) call, but the second was not, because
it was after that. Move the all LIST_REMOVE() calls down.

(cherry picked from commit 1bdf279)
(cherry picked from commit 8f1df94)

Related: #1729228
@dtardon dtardon force-pushed the dtardon:bz1729228-nested-slices branch from 19fb184 to 4fcb799 Jul 12, 2019
@systemd-rhel-bot systemd-rhel-bot added needs-ci and removed needs-ci labels Jul 12, 2019
@lnykryn lnykryn merged commit 019b191 into systemd-rhel:rhel-7.7 Jul 12, 2019
5 checks passed
5 checks passed
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
CentOS CI Build finished.
Details
LGTM analysis: C/C++ No new or fixed alerts
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dtardon dtardon deleted the dtardon:bz1729228-nested-slices branch Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.