[PATCH] __libc_exit_fini forgets to do pthread_mutex_unlock

[PATCH] __libc_exit_fini forgets to do pthread_mutex_unlock

[rebecca.zhang.cn]

From: Rebecca Zhang <rebecca.zhang.cn@windriver.com>

This commit fixes the issue that __libc_exit_fini only do
pthread_mutex_lock, but forget to do pthread_mutex_unlock.
---
 ldso/dynlink.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ldso/dynlink.c b/ldso/dynlink.c
index ceca3c9..7885675 100644
--- a/ldso/dynlink.c
+++ b/ldso/dynlink.c
@@ -1492,6 +1492,7 @@ void __libc_exit_fini()
 			fpaddr(p, dyn[DT_FINI])();
 #endif
 	}
+	pthread_mutex_unlock(&init_fini_lock);
 }
 
 void __ldso_atfork(int who)
-- 
2.34.1

Re: [PATCH] __libc_exit_fini forgets to do pthread_mutex_unlock

[Markus Wichmann]

Am Wed, Jul 02, 2025 at 10:28:54AM +0800 schrieb rebecca.zhang.cn@windriver.com:
> From: Rebecca Zhang <rebecca.zhang.cn@windriver.com>
> 
> This commit fixes the issue that __libc_exit_fini only do
> pthread_mutex_lock, but forget to do pthread_mutex_unlock.
> ---
>  ldso/dynlink.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/ldso/dynlink.c b/ldso/dynlink.c
> index ceca3c9..7885675 100644
> --- a/ldso/dynlink.c
> +++ b/ldso/dynlink.c
> @@ -1492,6 +1492,7 @@ void __libc_exit_fini()
>  			fpaddr(p, dyn[DT_FINI])();
>  #endif
>  	}
> +	pthread_mutex_unlock(&init_fini_lock);
>  }
>  
>  void __ldso_atfork(int who)
> -- 
> 2.34.1
> 
I think that is a deliberate omision. __libc_exit_fini() is called on
process exit. After it runs, it must not run again, and no new
initializer must run at all. The process will exit very soon anyway. The
only way to deadlock here is if a destructor calls exit(), which they
aren't allowed to do.

Ciao,
Markus

RE: [musl] [PATCH] __libc_exit_fini forgets to do pthread_mutex_unlock

[Zhang, Huilin (Rebecca) (CN)]

[-- Attachment #1: Type: text/plain, Size: 1976 bytes --]

Hello, Markus,

Please see attached test code main.c. Assume we compile it with MUSL and generate the executable program named myApp.
This myApp needs an input parameter which is an another executable program. If this parameter pointed to a nonexistent
program, this myApp will get stuck.

For example: (aaa is a nonexistent program)
./myApp aaa


The attached .png file is the snapshot that I ran myApp on ubutntu 22.04.3.

Thanks,
Rebecca


-----Original Message-----
From: Markus Wichmann <nullplan@gmx.net> 
Sent: Wednesday, July 2, 2025 12:31 PM
To: musl@lists.openwall.com
Cc: Zhang, Huilin (Rebecca) (CN) <Rebecca.Zhang.CN@windriver.com>; Deng, Wenbin (CN) <Wenbin.Deng.CN@windriver.com>
Subject: Re: [musl] [PATCH] __libc_exit_fini forgets to do pthread_mutex_unlock

CAUTION: This email comes from a non Wind River email account!
Do not click links or open attachments unless you recognize the sender and know the content is safe.

Am Wed, Jul 02, 2025 at 10:28:54AM +0800 schrieb rebecca.zhang.cn@windriver.com:
> From: Rebecca Zhang <rebecca.zhang.cn@windriver.com>
>
> This commit fixes the issue that __libc_exit_fini only do 
> pthread_mutex_lock, but forget to do pthread_mutex_unlock.
> ---
>  ldso/dynlink.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/ldso/dynlink.c b/ldso/dynlink.c index ceca3c9..7885675 
> 100644
> --- a/ldso/dynlink.c
> +++ b/ldso/dynlink.c
> @@ -1492,6 +1492,7 @@ void __libc_exit_fini()
>                       fpaddr(p, dyn[DT_FINI])();  #endif
>       }
> +     pthread_mutex_unlock(&init_fini_lock);
>  }
>
>  void __ldso_atfork(int who)
> --
> 2.34.1
>
I think that is a deliberate omision. __libc_exit_fini() is called on process exit. After it runs, it must not run again, and no new initializer must run at all. The process will exit very soon anyway. The only way to deadlock here is if a destructor calls exit(), which they aren't allowed to do.

Ciao,
Markus

[-- Attachment #2: main.c --]
[-- Type: text/plain, Size: 1305 bytes --]

#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <unistd.h>

void main(int argc, char *argv[]) {
  if (argc < 2) {
    fprintf(stderr, "usage: %s <program> [args...]\n", argv[0]);
    exit(EXIT_FAILURE);
  }

  printf("parent process PID=%d is starting\n", getpid());

  pid_t pid = vfork(); // use vfork to create child process

  if (pid < 0) {
    // vfork fail
    perror("vfork fail");
    exit(EXIT_FAILURE);
  } else if (pid == 0) {
    // child process
    printf("child process PID=%d will start: %s\n", getpid(), argv[1]);

    // execute program which is pass in via argv
    execvp(argv[1], &argv[1]);

    perror("execvp fail");
    exit(EXIT_FAILURE);
  } else {
    // parent process
    printf("parent process PID=%d wait for child process PID=%d\n", getpid(), pid);

    int status;
    pid_t wait_pid = waitpid(pid, &status, 0); // wait for child process terminating

    if (wait_pid < 0) {
      perror("waitpid fail");
    } else if (WIFEXITED(status)) {
      printf("child process exit，exit code: %d\n", WEXITSTATUS(status));
    } else if (WIFSIGNALED(status)) {
      printf("child process is terminated by signal，signal is: %d\n", WTERMSIG(status));
    }

    printf("parent process exit\n");
  }

  exit(0);
}

[-- Attachment #3: myApp hang.png --]
[-- Type: image/png, Size: 28487 bytes --]

RE: [musl] [PATCH] __libc_exit_fini forgets to do pthread_mutex_unlock

[Deng, Wenbin (CN)]

Hell Markus, 

I think the key issue in the "vfork+execv" case is that :  

With vfork, the parent and the child process share the same virtual memory space. Once after child execv failed and exit, the child process locks init_fini_lock but not unlock it. This causes the parent process pended on the same init_fini_lock forever and can't exit normally. My understanding is the child process's execv failed should not blocks parent process exit.

Thanks,
Wenbin

-----Original Message-----
From: Zhang, Huilin (Rebecca) (CN) <Rebecca.Zhang.CN@windriver.com> 
Sent: Wednesday, July 2, 2025 2:06 PM
To: Markus Wichmann <nullplan@gmx.net>; musl@lists.openwall.com
Cc: Deng, Wenbin (CN) <Wenbin.Deng.CN@windriver.com>
Subject: RE: [musl] [PATCH] __libc_exit_fini forgets to do pthread_mutex_unlock

Hello, Markus,

Please see attached test code main.c. Assume we compile it with MUSL and generate the executable program named myApp.
This myApp needs an input parameter which is an another executable program. If this parameter pointed to a nonexistent program, this myApp will get stuck.

For example: (aaa is a nonexistent program) ./myApp aaa


The attached .png file is the snapshot that I ran myApp on ubutntu 22.04.3.

Thanks,
Rebecca


-----Original Message-----
From: Markus Wichmann <nullplan@gmx.net>
Sent: Wednesday, July 2, 2025 12:31 PM
To: musl@lists.openwall.com
Cc: Zhang, Huilin (Rebecca) (CN) <Rebecca.Zhang.CN@windriver.com>; Deng, Wenbin (CN) <Wenbin.Deng.CN@windriver.com>
Subject: Re: [musl] [PATCH] __libc_exit_fini forgets to do pthread_mutex_unlock

CAUTION: This email comes from a non Wind River email account!
Do not click links or open attachments unless you recognize the sender and know the content is safe.

Am Wed, Jul 02, 2025 at 10:28:54AM +0800 schrieb rebecca.zhang.cn@windriver.com:
> From: Rebecca Zhang <rebecca.zhang.cn@windriver.com>
>
> This commit fixes the issue that __libc_exit_fini only do 
> pthread_mutex_lock, but forget to do pthread_mutex_unlock.
> ---
>  ldso/dynlink.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/ldso/dynlink.c b/ldso/dynlink.c index ceca3c9..7885675
> 100644
> --- a/ldso/dynlink.c
> +++ b/ldso/dynlink.c
> @@ -1492,6 +1492,7 @@ void __libc_exit_fini()
>                       fpaddr(p, dyn[DT_FINI])();  #endif
>       }
> +     pthread_mutex_unlock(&init_fini_lock);
>  }
>
>  void __ldso_atfork(int who)
> --
> 2.34.1
>
I think that is a deliberate omision. __libc_exit_fini() is called on process exit. After it runs, it must not run again, and no new initializer must run at all. The process will exit very soon anyway. The only way to deadlock here is if a destructor calls exit(), which they aren't allowed to do.

Ciao,
Markus

Re: [PATCH] __libc_exit_fini forgets to do pthread_mutex_unlock

[Rich Felker]

On Wed, Jul 02, 2025 at 06:06:11AM +0000, Zhang, Huilin (Rebecca) (CN) wrote:
> Hello, Markus,
> 
> Please see attached test code main.c. Assume we compile it with MUSL and generate the executable program named myApp.
> This myApp needs an input parameter which is an another executable program. If this parameter pointed to a nonexistent
> program, this myApp will get stuck.
> 
> For example: (aaa is a nonexistent program)
> ./myApp aaa
> 
> 
> The attached .png file is the snapshot that I ran myApp on ubutntu 22.04.3.
> 
> Thanks,
> Rebecca
> 
> 
> -----Original Message-----
> From: Markus Wichmann <nullplan@gmx.net> 
> Sent: Wednesday, July 2, 2025 12:31 PM
> To: musl@lists.openwall.com
> Cc: Zhang, Huilin (Rebecca) (CN) <Rebecca.Zhang.CN@windriver.com>; Deng, Wenbin (CN) <Wenbin.Deng.CN@windriver.com>
> Subject: Re: [musl] [PATCH] __libc_exit_fini forgets to do pthread_mutex_unlock
> 
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> Am Wed, Jul 02, 2025 at 10:28:54AM +0800 schrieb rebecca.zhang.cn@windriver.com:
> > From: Rebecca Zhang <rebecca.zhang.cn@windriver.com>
> >
> > This commit fixes the issue that __libc_exit_fini only do 
> > pthread_mutex_lock, but forget to do pthread_mutex_unlock.
> > ---
> >  ldso/dynlink.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/ldso/dynlink.c b/ldso/dynlink.c index ceca3c9..7885675 
> > 100644
> > --- a/ldso/dynlink.c
> > +++ b/ldso/dynlink.c
> > @@ -1492,6 +1492,7 @@ void __libc_exit_fini()
> >                       fpaddr(p, dyn[DT_FINI])();  #endif
> >       }
> > +     pthread_mutex_unlock(&init_fini_lock);
> >  }
> >
> >  void __ldso_atfork(int who)
> > --
> > 2.34.1
> >
> I think that is a deliberate omision. __libc_exit_fini() is called
> on process exit. After it runs, it must not run again, and no new
> initializer must run at all. The process will exit very soon anyway.
> The only way to deadlock here is if a destructor calls exit(), which
> they aren't allowed to do.
> 
> Ciao,
> Markus

> #include <errno.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <sys/types.h>
> #include <sys/wait.h>
> #include <unistd.h>
> 
> void main(int argc, char *argv[]) {
>   if (argc < 2) {
>     fprintf(stderr, "usage: %s <program> [args...]\n", argv[0]);
>     exit(EXIT_FAILURE);
>   }
> 
>   printf("parent process PID=%d is starting\n", getpid());
> 
>   pid_t pid = vfork(); // use vfork to create child process
> 
>   if (pid < 0) {
>     // vfork fail
>     perror("vfork fail");
>     exit(EXIT_FAILURE);
>   } else if (pid == 0) {
>     // child process
>     printf("child process PID=%d will start: %s\n", getpid(), argv[1]);
> 
>     // execute program which is pass in via argv
>     execvp(argv[1], &argv[1]);
> 
>     perror("execvp fail");
>     exit(EXIT_FAILURE);
>   } else {
>     // parent process
>     printf("parent process PID=%d wait for child process PID=%d\n", getpid(), pid);
> 
>     int status;
>     pid_t wait_pid = waitpid(pid, &status, 0); // wait for child process terminating
> 
>     if (wait_pid < 0) {
>       perror("waitpid fail");
>     } else if (WIFEXITED(status)) {
>       printf("child process exit???exit code: %d\n", WEXITSTATUS(status));
>     } else if (WIFSIGNALED(status)) {
>       printf("child process is terminated by signal???signal is: %d\n", WTERMSIG(status));
>     }
> 
>     printf("parent process exit\n");
>   }
> 
>   exit(0);
> }

This is completely expected. You cannot call exit in the child after
vfork. The only permissible actions are execve (and variants) and
_exit.

Rich

Re: [PATCH] __libc_exit_fini forgets to do pthread_mutex_unlock

[Rich Felker]

On Wed, Jul 02, 2025 at 06:30:33AM +0200, Markus Wichmann wrote:
> Am Wed, Jul 02, 2025 at 10:28:54AM +0800 schrieb rebecca.zhang.cn@windriver.com:
> > From: Rebecca Zhang <rebecca.zhang.cn@windriver.com>
> > 
> > This commit fixes the issue that __libc_exit_fini only do
> > pthread_mutex_lock, but forget to do pthread_mutex_unlock.
> > ---
> >  ldso/dynlink.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/ldso/dynlink.c b/ldso/dynlink.c
> > index ceca3c9..7885675 100644
> > --- a/ldso/dynlink.c
> > +++ b/ldso/dynlink.c
> > @@ -1492,6 +1492,7 @@ void __libc_exit_fini()
> >  			fpaddr(p, dyn[DT_FINI])();
> >  #endif
> >  	}
> > +	pthread_mutex_unlock(&init_fini_lock);
> >  }
> >  
> >  void __ldso_atfork(int who)
> > -- 
> > 2.34.1
> > 
> I think that is a deliberate omision. __libc_exit_fini() is called on
> process exit. After it runs, it must not run again, and no new
> initializer must run at all. The process will exit very soon anyway. The
> only way to deadlock here is if a destructor calls exit(), which they
> aren't allowed to do.

It is very much deliberate that this lock is never released. Similarly
with a number of other locks they did not seem to notice that would
cause the same behavior even if this one was changed. The general
pattern is "a contractual requirement of exit is that all things of
category X have finished before the process terminates", and this
necessitates ensuring that no new "things of category X" can come into
existence once you're past the step where they're processed.

Other examples include flushing stdio (must not allow any new data to
become buffered after flush is complete) and processing atexit
handlers (must not allow a new handler to be registered after the loop
that runs them).

Rich

RE: [PATCH] __libc_exit_fini forgets to do pthread_mutex_unlock

[Zhang, Huilin (Rebecca) (CN)]

Hello, Rich, Markus,

Thanks for your explanation. 

Yes, if we change "exit" to "_exit", the test case could exit normally.

OK, we accept it is really a deliberate omission.

Thanks,
Rebecca


-----Original Message-----
From: Rich Felker <dalias@libc.org> 
Sent: Wednesday, July 2, 2025 10:33 PM
To: Markus Wichmann <nullplan@gmx.net>
Cc: musl@lists.openwall.com; Zhang, Huilin (Rebecca) (CN) <Rebecca.Zhang.CN@windriver.com>; Deng, Wenbin (CN) <Wenbin.Deng.CN@windriver.com>
Subject: Re: [musl] [PATCH] __libc_exit_fini forgets to do pthread_mutex_unlock

CAUTION: This email comes from a non Wind River email account!
Do not click links or open attachments unless you recognize the sender and know the content is safe.

On Wed, Jul 02, 2025 at 06:30:33AM +0200, Markus Wichmann wrote:
> Am Wed, Jul 02, 2025 at 10:28:54AM +0800 schrieb rebecca.zhang.cn@windriver.com:
> > From: Rebecca Zhang <rebecca.zhang.cn@windriver.com>
> >
> > This commit fixes the issue that __libc_exit_fini only do 
> > pthread_mutex_lock, but forget to do pthread_mutex_unlock.
> > ---
> >  ldso/dynlink.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/ldso/dynlink.c b/ldso/dynlink.c index ceca3c9..7885675 
> > 100644
> > --- a/ldso/dynlink.c
> > +++ b/ldso/dynlink.c
> > @@ -1492,6 +1492,7 @@ void __libc_exit_fini()
> >                     fpaddr(p, dyn[DT_FINI])();  #endif
> >     }
> > +   pthread_mutex_unlock(&init_fini_lock);
> >  }
> >
> >  void __ldso_atfork(int who)
> > --
> > 2.34.1
> >
> I think that is a deliberate omision. __libc_exit_fini() is called on 
> process exit. After it runs, it must not run again, and no new 
> initializer must run at all. The process will exit very soon anyway. 
> The only way to deadlock here is if a destructor calls exit(), which 
> they aren't allowed to do.

It is very much deliberate that this lock is never released. Similarly with a number of other locks they did not seem to notice that would cause the same behavior even if this one was changed. The general pattern is "a contractual requirement of exit is that all things of category X have finished before the process terminates", and this necessitates ensuring that no new "things of category X" can come into existence once you're past the step where they're processed.

Other examples include flushing stdio (must not allow any new data to become buffered after flush is complete) and processing atexit handlers (must not allow a new handler to be registered after the loop that runs them).

Rich