Fix one of the TODO items when creating a new thread: release the copied
cpu and free the task state.

Signed-off-by: Warner Losh <imp@bsdimp.com>
---
Free the new task state and drop references to copied cpu structure when
pthread_create failes.
---
 linux-user/syscall.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index d3d9fffb54..7b2e32bcf5 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7005,7 +7005,6 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
         cpu->random_seed = qemu_guest_random_seed_thread_part1();
 
         ret = pthread_create(&info.thread, &attr, clone_func, &info);
-        /* TODO: Free new CPU state if thread creation failed.  */
 
         sigprocmask(SIG_SETMASK, &info.sigmask, NULL);
         pthread_attr_destroy(&attr);
@@ -7014,7 +7013,10 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
             pthread_cond_wait(&info.cond, &info.mutex);
             ret = info.tid;
         } else {
-            ret = -1;
+            ret = -host_to_target_errno(ret);
+            object_unparent(OBJECT(new_cpu));
+            object_unref(OBJECT(new_cpu));
+            g_free(ts);
         }
         pthread_mutex_unlock(&info.mutex);
         pthread_cond_destroy(&info.cond);

---
base-commit: ac0cc20ad2fe0b8df2e5d9458e90a095ac711ab1
change-id: 20260507-linux-user-bug-6a5e4524d2db

Best regards,
-- 
Warner Losh <imp@bsdimp.com>

On Thu, 7 May 2026 at 16:20, Warner Losh <imp@bsdimp.com> wrote:
>
> Fix one of the TODO items when creating a new thread: release the copied
> cpu and free the task state.
>
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> ---
> Free the new task state and drop references to copied cpu structure when
> pthread_create failes.
> ---
>  linux-user/syscall.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index d3d9fffb54..7b2e32bcf5 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7005,7 +7005,6 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
>          cpu->random_seed = qemu_guest_random_seed_thread_part1();
>
>          ret = pthread_create(&info.thread, &attr, clone_func, &info);
> -        /* TODO: Free new CPU state if thread creation failed.  */
>
>          sigprocmask(SIG_SETMASK, &info.sigmask, NULL);
>          pthread_attr_destroy(&attr);
> @@ -7014,7 +7013,10 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
>              pthread_cond_wait(&info.cond, &info.mutex);
>              ret = info.tid;
>          } else {
> -            ret = -1;
> +            ret = -host_to_target_errno(ret);

This is an unrelated bugfix to the one in the commit message.

I found do_fork()'s error handling confusing (the comment at
the top doesn't exactly clarify either), but I think it is:
 * might return a negative target errno value
 * might return -1 with "errno" set to a host errno
 * might succeed and return 0
 * might succeed and return some other not-minus-1 value

The callers all do
   return get_errno(do_fork(....));
which handles these cases.

It might be less confusing overall if we simplified this to:
 * returns negative target errno value on failure
 * otherwise success, returning value the guest expects
and dropped the get_errno() at the callsites. The only
cases that don't already follow one of those two options are
the "pthread_create() failed" one (which is buggy currently
since pthread_create() doesn't set errno) and the "fork()
failed" one (which relies on the "return -1 and caller fishes
error out of errno" handling).

> +            object_unparent(OBJECT(new_cpu));
> +            object_unref(OBJECT(new_cpu));
> +            g_free(ts);

For TARGET_AARCH64, the gcs_new_stack() call we did before
attempting the pthread_create() maps memory into the target
address space, which needs to be undone. Compare the bit in
TARGET_NR_exit handling which does:

#ifdef TARGET_AARCH64
            if (ts->gcs_base) {
                target_munmap(ts->gcs_base, ts->gcs_size);
            }
#endif

We could maybe share code between the NR_exit "free stuff
for this TaskState" and this error-exit path, though it's
a bit awkward so I'm 50:50 on that.

>          }
>          pthread_mutex_unlock(&info.mutex);
>          pthread_cond_destroy(&info.cond);

thanks
-- PMM

On Thu, May 7, 2026 at 9:48 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Thu, 7 May 2026 at 16:20, Warner Losh <imp@bsdimp.com> wrote:
> >
> > Fix one of the TODO items when creating a new thread: release the copied
> > cpu and free the task state.
> >
> > Signed-off-by: Warner Losh <imp@bsdimp.com>
> > ---
> > Free the new task state and drop references to copied cpu structure when
> > pthread_create failes.
> > ---
> >  linux-user/syscall.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index d3d9fffb54..7b2e32bcf5 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -7005,7 +7005,6 @@ static int do_fork(CPUArchState *env, unsigned int
> flags, abi_ulong newsp,
> >          cpu->random_seed = qemu_guest_random_seed_thread_part1();
> >
> >          ret = pthread_create(&info.thread, &attr, clone_func, &info);
> > -        /* TODO: Free new CPU state if thread creation failed.  */
> >
> >          sigprocmask(SIG_SETMASK, &info.sigmask, NULL);
> >          pthread_attr_destroy(&attr);
> > @@ -7014,7 +7013,10 @@ static int do_fork(CPUArchState *env, unsigned
> int flags, abi_ulong newsp,
> >              pthread_cond_wait(&info.cond, &info.mutex);
> >              ret = info.tid;
> >          } else {
> > -            ret = -1;
> > +            ret = -host_to_target_errno(ret);
>
> This is an unrelated bugfix to the one in the commit message.
>
> I found do_fork()'s error handling confusing (the comment at
> the top doesn't exactly clarify either), but I think it is:
>  * might return a negative target errno value
>  * might return -1 with "errno" set to a host errno
>  * might succeed and return 0
>  * might succeed and return some other not-minus-1 value
>

Yea, I was confused by that and opted to 'return negative target errno'
notion here, but adding 'errno = ret;' just before the ret = -1 in the
existing
code would keep get_errno() happy... But in reality do the same thing
I submitted...

I could also just leave the bug alone as part of my submission.


> The callers all do
>    return get_errno(do_fork(....));
> which handles these cases.
>
> It might be less confusing overall if we simplified this to:
>  * returns negative target errno value on failure
>  * otherwise success, returning value the guest expects
> and dropped the get_errno() at the callsites. The only
> cases that don't already follow one of those two options are
> the "pthread_create() failed" one (which is buggy currently
> since pthread_create() doesn't set errno) and the "fork()
> failed" one (which relies on the "return -1 and caller fishes
> error out of errno" handling).
>

Yea, unwinding that looks beyond what I have time for here.


> > +            object_unparent(OBJECT(new_cpu));
> > +            object_unref(OBJECT(new_cpu));
> > +            g_free(ts);
>
> For TARGET_AARCH64, the gcs_new_stack() call we did before
> attempting the pthread_create() maps memory into the target
> address space, which needs to be undone. Compare the bit in
> TARGET_NR_exit handling which does:
>
> #ifdef TARGET_AARCH64
>             if (ts->gcs_base) {
>                 target_munmap(ts->gcs_base, ts->gcs_size);
>             }
> #endif
>
> We could maybe share code between the NR_exit "free stuff
> for this TaskState" and this error-exit path, though it's
> a bit awkward so I'm 50:50 on that.
>

Yea, it is short enough I think inlining isn't terrible, but if more of
these
cases grow, then refactoring to a routine would make sense. At the moment,
I lean more towards inlining...

Warner


> >          }
> >          pthread_mutex_unlock(&info.mutex);
> >          pthread_cond_destroy(&info.cond);
>
> thanks
> -- PMM
>