| Message ID | 20260227100943.284203-1-florian.schmaus@codasip.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix dirname() handling in run_ptests() | expand |
Hi Florian, Thanks for the patch, comments below, Can you refer me to the musl dirname(3) function that uses static allocation?. I reviewed [1] and found that modifying the same string anyway doing the assignment is needed. [1] https://git.musl-libc.org/cgit/musl/tree/src/misc/dirname.c On Fri, Feb 27, 2026 at 4:09 AM Florian Schmaus <florian.schmaus@codasip.com> wrote: > It is required to use the return value of dirname() instead of > assuming that dirname() modifies the input string. While certain > implementations of dirname(), for example glibc, *may* modify the > input string, this is not guaranteed behavior. Musl, on the other > hand, does not modify the input string but instead returns a pointer > to a statically allocated string. > > Previously, ptest-runner would fail on musl systems: > > $ ptest-runner busybox > START: ptest-runner > 2026-02-27T09:21 > BEGIN: /usr/lib/busybox/ptest/run-ptest > ERROR: Unable to chdir(/usr/lib/busybox/ptest/run-ptest), Not a directory > > ERROR: Exited from signal Killed (9) > DURATION: 0 > END: /usr/lib/busybox/ptest/run-ptest > 2026-02-27T09:21 > STOP: ptest-runner > TOTAL: 1 FAIL: 1 > > Signed-off-by: Florian Schmaus <florian.schmaus@codasip.com> > --- > utils.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/utils.c b/utils.c > index 6cf7705586bc..7614dce99bd8 100644 > --- a/utils.c > +++ b/utils.c > @@ -361,13 +361,19 @@ run_ptests(struct ptest_list *head, const struct > ptest_options opts, > > fprintf(fp, "START: %s\n", progname); > PTEST_LIST_ITERATE_START(head, p) > - char ptest_dir[PATH_MAX] = {'\0'}; > + char *ptest_dir; > + char run_ptest[PATH_MAX] = {'\0'}; > int pipefd_stdout[2] = {-1, -1}; > int pipefd_stderr[2] = {-1, -1}; > int pty[2] = {-1, -1}; > > - strcpy(ptest_dir, p->run_ptest); > - dirname(ptest_dir); > strcpy(ptest_dir, p->run_ptest); // This assumes ptest_dir and run_ptest are both PATH_MAX; perhaps improve with a secure version of strcpy? ptest_dir = dirname(ptest_dir); > + /* Copy, since dirname() may modify its input > buffer */ > + if (strlcpy(run_ptest, p->run_ptest, > sizeof(run_ptest)) >= sizeof(run_ptest)) { > + fprintf(fp, "ERROR: %s exceeds > PATH_MAX\n", p->run_ptest); > + rc = -1; > + goto ptest_list_fail1; > + } > + ptest_dir = dirname(run_ptest); > > if (pipe2(pipefd_stdout, 0) == -1) { > fprintf(fp, "ERROR: pipe2() failed with: > %s.\n", strerror(errno)); > -- > 2.52.0 > >
On 27/02/2026 16.58, Anibal Limon wrote: > Hi Florian, Hi Anibal, > Thanks for the patch, comments below, Thanks for looking at my patch. > Can you refer me to the musl dirname(3) function that uses static > allocation?. > I reviewed [1] and found that modifying the same string anyway doing the > assignment is needed. Oh, right. It's the CHERI-enable musl libc that is using a static allocation [1]. I wasn't aware that this is a deviation from upstream. But it doesn't really matter, the current usage of dirname() is fragile and should be fixed. > [1] https://git.musl-libc.org/cgit/musl/tree/src/misc/dirname.c > <https://git.musl-libc.org/cgit/musl/tree/src/misc/dirname.c> > > On Fri, Feb 27, 2026 at 4:09 AM Florian Schmaus > <florian.schmaus@codasip.com <mailto:florian.schmaus@codasip.com>> wrote: > > It is required to use the return value of dirname() instead of > assuming that dirname() modifies the input string. While certain > implementations of dirname(), for example glibc, *may* modify the > input string, this is not guaranteed behavior. Musl, on the other > hand, does not modify the input string but instead returns a pointer > to a statically allocated string. > > Previously, ptest-runner would fail on musl systems: > > $ ptest-runner busybox > START: ptest-runner > 2026-02-27T09:21 > BEGIN: /usr/lib/busybox/ptest/run-ptest > ERROR: Unable to chdir(/usr/lib/busybox/ptest/run-ptest), Not a > directory > > ERROR: Exited from signal Killed (9) > DURATION: 0 > END: /usr/lib/busybox/ptest/run-ptest > 2026-02-27T09:21 > STOP: ptest-runner > TOTAL: 1 FAIL: 1 > > Signed-off-by: Florian Schmaus <florian.schmaus@codasip.com > <mailto:florian.schmaus@codasip.com>> > --- > utils.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/utils.c b/utils.c > index 6cf7705586bc..7614dce99bd8 100644 > --- a/utils.c > +++ b/utils.c > @@ -361,13 +361,19 @@ run_ptests(struct ptest_list *head, const > struct ptest_options opts, > > fprintf(fp, "START: %s\n", progname); > PTEST_LIST_ITERATE_START(head, p) > - char ptest_dir[PATH_MAX] = {'\0'}; > + char *ptest_dir; > + char run_ptest[PATH_MAX] = {'\0'}; > int pipefd_stdout[2] = {-1, -1}; > int pipefd_stderr[2] = {-1, -1}; > int pty[2] = {-1, -1}; > > - strcpy(ptest_dir, p->run_ptest); > - dirname(ptest_dir); > > > strcpy(ptest_dir, p->run_ptest); // This assumes ptest_dir and run_ptest > are both PATH_MAX; perhaps improve with a secure version of strcpy? Not sure if I understand the comment. My patch is using strlcpy() and should be safe. > ptest_dir = dirname(ptest_dir); > > + /* Copy, since dirname() may modify its > input buffer */ > + if (strlcpy(run_ptest, p->run_ptest, > sizeof(run_ptest)) >= sizeof(run_ptest)) { > + fprintf(fp, "ERROR: %s exceeds > PATH_MAX\n", p->run_ptest); > + rc = -1; > + goto ptest_list_fail1; > + } > + ptest_dir = dirname(run_ptest); > > if (pipe2(pipefd_stdout, 0) == -1) { > fprintf(fp, "ERROR: pipe2() failed > with: %s.\n", strerror(errno)); > -- > 2.52.0 > - Florian 1: https://git.morello-project.org/morello/musl-libc/-/blob/morello/master/src/misc/dirname.c
On Fri, Feb 27, 2026 at 1:52 PM Florian Schmaus <florian.schmaus@codasip.com> wrote: > On 27/02/2026 16.58, Anibal Limon wrote: > > Hi Florian, > > Hi Anibal, > > > > Thanks for the patch, comments below, > > Thanks for looking at my patch. > > > > Can you refer me to the musl dirname(3) function that uses static > > allocation?. > > I reviewed [1] and found that modifying the same string anyway doing the > > assignment is needed. > > Oh, right. It's the CHERI-enable musl libc that is using a static > allocation [1]. I wasn't aware that this is a deviation from upstream. > > But it doesn't really matter, the current usage of dirname() is fragile > and should be fixed. > > > > [1] https://git.musl-libc.org/cgit/musl/tree/src/misc/dirname.c > > <https://git.musl-libc.org/cgit/musl/tree/src/misc/dirname.c> > > > > On Fri, Feb 27, 2026 at 4:09 AM Florian Schmaus > > <florian.schmaus@codasip.com <mailto:florian.schmaus@codasip.com>> > wrote: > > > > It is required to use the return value of dirname() instead of > > assuming that dirname() modifies the input string. While certain > > implementations of dirname(), for example glibc, *may* modify the > > input string, this is not guaranteed behavior. Musl, on the other > > hand, does not modify the input string but instead returns a pointer > > to a statically allocated string. > > > > Previously, ptest-runner would fail on musl systems: > > > > $ ptest-runner busybox > > START: ptest-runner > > 2026-02-27T09:21 > > BEGIN: /usr/lib/busybox/ptest/run-ptest > > ERROR: Unable to chdir(/usr/lib/busybox/ptest/run-ptest), Not a > > directory > > > > ERROR: Exited from signal Killed (9) > > DURATION: 0 > > END: /usr/lib/busybox/ptest/run-ptest > > 2026-02-27T09:21 > > STOP: ptest-runner > > TOTAL: 1 FAIL: 1 > > > > Signed-off-by: Florian Schmaus <florian.schmaus@codasip.com > > <mailto:florian.schmaus@codasip.com>> > > --- > > utils.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/utils.c b/utils.c > > index 6cf7705586bc..7614dce99bd8 100644 > > --- a/utils.c > > +++ b/utils.c > > @@ -361,13 +361,19 @@ run_ptests(struct ptest_list *head, const > > struct ptest_options opts, > > > > fprintf(fp, "START: %s\n", progname); > > PTEST_LIST_ITERATE_START(head, p) > > - char ptest_dir[PATH_MAX] = {'\0'}; > > + char *ptest_dir; > > + char run_ptest[PATH_MAX] = {'\0'}; > > int pipefd_stdout[2] = {-1, -1}; > > int pipefd_stderr[2] = {-1, -1}; > > int pty[2] = {-1, -1}; > > > > - strcpy(ptest_dir, p->run_ptest); > > - dirname(ptest_dir); > > > > > > strcpy(ptest_dir, p->run_ptest); // This assumes ptest_dir and run_ptest > > are both PATH_MAX; perhaps improve with a secure version of strcpy? > > Not sure if I understand the comment. My patch is using strlcpy() and > should be safe. > > > ptest_dir = dirname(ptest_dir); > I mean making this change should be enough, plus the `strcpy` change to `strlcpy` removes the assumption about using PATH_MAX. Cheers!. Anibal > > > > + /* Copy, since dirname() may modify its > > input buffer */ > > + if (strlcpy(run_ptest, p->run_ptest, > > sizeof(run_ptest)) >= sizeof(run_ptest)) { > > + fprintf(fp, "ERROR: %s exceeds > > PATH_MAX\n", p->run_ptest); > > + rc = -1; > > + goto ptest_list_fail1; > > + } > > + ptest_dir = dirname(run_ptest); > > > > if (pipe2(pipefd_stdout, 0) == -1) { > > fprintf(fp, "ERROR: pipe2() failed > > with: %s.\n", strerror(errno)); > > -- > > 2.52.0 > > > > - Florian > > 1: > > https://git.morello-project.org/morello/musl-libc/-/blob/morello/master/src/misc/dirname.c > >
On 27/02/2026 21.20, Anibal Limon wrote: > > > On Fri, Feb 27, 2026 at 1:52 PM Florian Schmaus > <florian.schmaus@codasip.com <mailto:florian.schmaus@codasip.com>> wrote: > > On 27/02/2026 16.58, Anibal Limon wrote: > > Hi Florian, > > Hi Anibal, > > > > Thanks for the patch, comments below, > > Thanks for looking at my patch. > > > > Can you refer me to the musl dirname(3) function that uses static > > allocation?. > > I reviewed [1] and found that modifying the same string anyway > doing the > > assignment is needed. > > Oh, right. It's the CHERI-enable musl libc that is using a static > allocation [1]. I wasn't aware that this is a deviation from upstream. > > But it doesn't really matter, the current usage of dirname() is fragile > and should be fixed. > > > > [1] https://git.musl-libc.org/cgit/musl/tree/src/misc/dirname.c > <https://git.musl-libc.org/cgit/musl/tree/src/misc/dirname.c> > > <https://git.musl-libc.org/cgit/musl/tree/src/misc/dirname.c > <https://git.musl-libc.org/cgit/musl/tree/src/misc/dirname.c>> > > > > On Fri, Feb 27, 2026 at 4:09 AM Florian Schmaus > > <florian.schmaus@codasip.com <mailto:florian.schmaus@codasip.com> > <mailto:florian.schmaus@codasip.com > <mailto:florian.schmaus@codasip.com>>> wrote: > > > > It is required to use the return value of dirname() instead of > > assuming that dirname() modifies the input string. While certain > > implementations of dirname(), for example glibc, *may* modify the > > input string, this is not guaranteed behavior. Musl, on the other > > hand, does not modify the input string but instead returns a > pointer > > to a statically allocated string. > > > > Previously, ptest-runner would fail on musl systems: > > > > $ ptest-runner busybox > > START: ptest-runner > > 2026-02-27T09:21 > > BEGIN: /usr/lib/busybox/ptest/run-ptest > > ERROR: Unable to chdir(/usr/lib/busybox/ptest/run-ptest), Not a > > directory > > > > ERROR: Exited from signal Killed (9) > > DURATION: 0 > > END: /usr/lib/busybox/ptest/run-ptest > > 2026-02-27T09:21 > > STOP: ptest-runner > > TOTAL: 1 FAIL: 1 > > > > Signed-off-by: Florian Schmaus <florian.schmaus@codasip.com > <mailto:florian.schmaus@codasip.com> > > <mailto:florian.schmaus@codasip.com > <mailto:florian.schmaus@codasip.com>>> > > --- > > utils.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/utils.c b/utils.c > > index 6cf7705586bc..7614dce99bd8 100644 > > --- a/utils.c > > +++ b/utils.c > > @@ -361,13 +361,19 @@ run_ptests(struct ptest_list *head, const > > struct ptest_options opts, > > > > fprintf(fp, "START: %s\n", progname); > > PTEST_LIST_ITERATE_START(head, p) > > - char ptest_dir[PATH_MAX] = {'\0'}; > > + char *ptest_dir; > > + char run_ptest[PATH_MAX] = {'\0'}; > > int pipefd_stdout[2] = {-1, -1}; > > int pipefd_stderr[2] = {-1, -1}; > > int pty[2] = {-1, -1}; > > > > - strcpy(ptest_dir, p->run_ptest); > > - dirname(ptest_dir); > > > > > > strcpy(ptest_dir, p->run_ptest); // This assumes ptest_dir and > run_ptest > > are both PATH_MAX; perhaps improve with a secure version of strcpy? > > Not sure if I understand the comment. My patch is using strlcpy() and > should be safe. > > > > > ptest_dir = dirname(ptest_dir); > > > I mean making this change should be enough That doesn't work as it would assign to an array type. - Florian
On Fri, Feb 27, 2026 at 2:37 PM Florian Schmaus <florian.schmaus@codasip.com> wrote: > On 27/02/2026 21.20, Anibal Limon wrote: > > > > > > On Fri, Feb 27, 2026 at 1:52 PM Florian Schmaus > > <florian.schmaus@codasip.com <mailto:florian.schmaus@codasip.com>> > wrote: > > > > On 27/02/2026 16.58, Anibal Limon wrote: > > > Hi Florian, > > > > Hi Anibal, > > > > > > > Thanks for the patch, comments below, > > > > Thanks for looking at my patch. > > > > > > > Can you refer me to the musl dirname(3) function that uses static > > > allocation?. > > > I reviewed [1] and found that modifying the same string anyway > > doing the > > > assignment is needed. > > > > Oh, right. It's the CHERI-enable musl libc that is using a static > > allocation [1]. I wasn't aware that this is a deviation from > upstream. > > > > But it doesn't really matter, the current usage of dirname() is > fragile > > and should be fixed. > > > > > > > [1] https://git.musl-libc.org/cgit/musl/tree/src/misc/dirname.c > > <https://git.musl-libc.org/cgit/musl/tree/src/misc/dirname.c> > > > <https://git.musl-libc.org/cgit/musl/tree/src/misc/dirname.c > > <https://git.musl-libc.org/cgit/musl/tree/src/misc/dirname.c>> > > > > > > On Fri, Feb 27, 2026 at 4:09 AM Florian Schmaus > > > <florian.schmaus@codasip.com <mailto:florian.schmaus@codasip.com> > > <mailto:florian.schmaus@codasip.com > > <mailto:florian.schmaus@codasip.com>>> wrote: > > > > > > It is required to use the return value of dirname() instead of > > > assuming that dirname() modifies the input string. While > certain > > > implementations of dirname(), for example glibc, *may* modify > the > > > input string, this is not guaranteed behavior. Musl, on the > other > > > hand, does not modify the input string but instead returns a > > pointer > > > to a statically allocated string. > > > > > > Previously, ptest-runner would fail on musl systems: > > > > > > $ ptest-runner busybox > > > START: ptest-runner > > > 2026-02-27T09:21 > > > BEGIN: /usr/lib/busybox/ptest/run-ptest > > > ERROR: Unable to chdir(/usr/lib/busybox/ptest/run-ptest), Not > a > > > directory > > > > > > ERROR: Exited from signal Killed (9) > > > DURATION: 0 > > > END: /usr/lib/busybox/ptest/run-ptest > > > 2026-02-27T09:21 > > > STOP: ptest-runner > > > TOTAL: 1 FAIL: 1 > > > > > > Signed-off-by: Florian Schmaus <florian.schmaus@codasip.com > > <mailto:florian.schmaus@codasip.com> > > > <mailto:florian.schmaus@codasip.com > > <mailto:florian.schmaus@codasip.com>>> > > > --- > > > utils.c | 12 +++++++++--- > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > diff --git a/utils.c b/utils.c > > > index 6cf7705586bc..7614dce99bd8 100644 > > > --- a/utils.c > > > +++ b/utils.c > > > @@ -361,13 +361,19 @@ run_ptests(struct ptest_list *head, > const > > > struct ptest_options opts, > > > > > > fprintf(fp, "START: %s\n", progname); > > > PTEST_LIST_ITERATE_START(head, p) > > > - char ptest_dir[PATH_MAX] = {'\0'}; > > > + char *ptest_dir; > > > + char run_ptest[PATH_MAX] = {'\0'}; > > > int pipefd_stdout[2] = {-1, -1}; > > > int pipefd_stderr[2] = {-1, -1}; > > > int pty[2] = {-1, -1}; > > > > > > - strcpy(ptest_dir, p->run_ptest); > > > - dirname(ptest_dir); > > > > > > > > > strcpy(ptest_dir, p->run_ptest); // This assumes ptest_dir and > > run_ptest > > > are both PATH_MAX; perhaps improve with a secure version of > strcpy? > > > > Not sure if I understand the comment. My patch is using strlcpy() and > > should be safe. > > > > > > > > > ptest_dir = dirname(ptest_dir); > > > > > > I mean making this change should be enough > That doesn't work as it would assign to an array type. > A possible implementation could be: ``` --- a/utils.c +++ b/utils.c @@ -361,13 +361,14 @@ run_ptests(struct ptest_list *head, const struct ptest_options opts, fprintf(fp, "START: %s\n", progname); PTEST_LIST_ITERATE_START(head, p) - char ptest_dir[PATH_MAX] = {'\0'}; + char ptest_dir_tmp[PATH_MAX] = {'\0'}; + char *ptest_dir = NULL; int pipefd_stdout[2] = {-1, -1}; int pipefd_stderr[2] = {-1, -1}; int pty[2] = {-1, -1}; - strcpy(ptest_dir, p->run_ptest); - dirname(ptest_dir); + strcpy(ptest_dir_tmp, p->run_ptest); + ptest_dir = dirname(ptest_dir_tmp); if (pipe2(pipefd_stdout, 0) == -1) { fprintf(fp, "ERROR: pipe2() failed with: %s.\n", strerror(errno)); ``` Anibal > > - Florian >
diff --git a/utils.c b/utils.c index 6cf7705586bc..7614dce99bd8 100644 --- a/utils.c +++ b/utils.c @@ -361,13 +361,19 @@ run_ptests(struct ptest_list *head, const struct ptest_options opts, fprintf(fp, "START: %s\n", progname); PTEST_LIST_ITERATE_START(head, p) - char ptest_dir[PATH_MAX] = {'\0'}; + char *ptest_dir; + char run_ptest[PATH_MAX] = {'\0'}; int pipefd_stdout[2] = {-1, -1}; int pipefd_stderr[2] = {-1, -1}; int pty[2] = {-1, -1}; - strcpy(ptest_dir, p->run_ptest); - dirname(ptest_dir); + /* Copy, since dirname() may modify its input buffer */ + if (strlcpy(run_ptest, p->run_ptest, sizeof(run_ptest)) >= sizeof(run_ptest)) { + fprintf(fp, "ERROR: %s exceeds PATH_MAX\n", p->run_ptest); + rc = -1; + goto ptest_list_fail1; + } + ptest_dir = dirname(run_ptest); if (pipe2(pipefd_stdout, 0) == -1) { fprintf(fp, "ERROR: pipe2() failed with: %s.\n", strerror(errno));
It is required to use the return value of dirname() instead of assuming that dirname() modifies the input string. While certain implementations of dirname(), for example glibc, *may* modify the input string, this is not guaranteed behavior. Musl, on the other hand, does not modify the input string but instead returns a pointer to a statically allocated string. Previously, ptest-runner would fail on musl systems: $ ptest-runner busybox START: ptest-runner 2026-02-27T09:21 BEGIN: /usr/lib/busybox/ptest/run-ptest ERROR: Unable to chdir(/usr/lib/busybox/ptest/run-ptest), Not a directory ERROR: Exited from signal Killed (9) DURATION: 0 END: /usr/lib/busybox/ptest/run-ptest 2026-02-27T09:21 STOP: ptest-runner TOTAL: 1 FAIL: 1 Signed-off-by: Florian Schmaus <florian.schmaus@codasip.com> --- utils.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)