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.

Sign up for GitHub

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 parent refcount (huge memory leak) on non-last thread exit #51

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix parent refcount (huge memory leak) on non-last thread exit #51

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion linux-user/syscall.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8309,7 +8309,8 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
TaskState *ts = cpu->opaque;

object_property_set_bool(OBJECT(cpu), "realized", false, NULL);
object_unref(OBJECT(cpu));
object_unparent(OBJECT(cpu));

/*
* At this point the CPU should be unrealized and removed
* from cpu lists. We can clean-up the rest of the thread
Expand All @@ -8323,6 +8324,8 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
do_sys_futex(g2h(cpu, ts->child_tidptr),
FUTEX_WAKE, INT_MAX, NULL, NULL, 0);
}

object_unref(OBJECT(cpu));
Copy link
Member

Choose a reason for hiding this comment

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

Looking at current QEMU, this should be done before unlocking clone_lock

Copy link
Author

Choose a reason for hiding this comment

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

the thing is that:

from comments in object_initialize_child_with_propsv():

  • We want @obj's reference to be 1 on success, 0 on failure.
    * On success, it's 2: one taken by object_initialize(), and one
    * by object_property_add_child().
    * On failure in object_initialize() or earlier, it's 1.
    * On failure afterwards, it's also 1: object_unparent() releases
    * the reference taken by object_property_add_child().

next, when thread done, obj->ref == 2;
We doing just single obj_unref(), which leads to:
if (qatomic_fetch_dec(&obj->ref) == 1) {, which executed as if (2 == 1),
and decreasing obj->ref by one, after fetch...
So, the cpu object, which is analogue to task_struct kernel object, still allocated and forgotten.

Moreover, even if we pass this branch, then in
static void object_finalize(void *data) we have g_assert(obj->parent == NULL); which expect no-linkage to parent.

Finally, we remove linkage to parent while holding a lock, and freeing (unknown to parent now) object without.

thread_cpu = NULL;
g_free(ts);
rcu_unregister_thread();
Expand Down
4 changes: 2 additions & 2 deletions qom/object.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1179,10 +1179,10 @@ void object_unref(void *objptr)
if (!obj) {
return;
}
g_assert(obj->ref > 0);
g_assert(qatomic_mb_read(&obj->ref) > 0);
Copy link
Member

Choose a reason for hiding this comment

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

object_unref in current QEMU is still the same of this one

void object_unref(void *objptr)
{
    Object *obj = OBJECT(objptr);
    if (!obj) {
        return;
    }
    g_assert(obj->ref > 0);

    /* parent always holds a reference to its children */
    if (qatomic_fetch_dec(&obj->ref) == 1) {
        object_finalize(obj);
    }
}

If you spotted a bug in QEMU you should report to them

Copy link
Author

Choose a reason for hiding this comment

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

if (qatomic_fetch_dec(&obj->ref) == 1) { is equal to if (qatomic_dec_fetch(&obj->ref) == 0) {, we can left it as is and the patch above would still working.
Anyway, we are fuzzing and don't care about hw emulation, so we can just change the things.
Same method, as with rcu_disable_atfork(), right?

/* parent always holds a reference to its children */
so the child left allocated forever.


/* parent always holds a reference to its children */
if (qatomic_fetch_dec(&obj->ref) == 1) {
if (qatomic_dec_fetch(&obj->ref) == 0) {
object_finalize(obj);
}
}
Expand Down