On Sun, Feb 16, 2025 at 11:00:56AM +0100, Alejandro Colomar wrote:
> Hi Askar, Carlos,
>
> On Sun, Feb 16, 2025 at 06:18:28AM +0000, Askar Safin wrote:
> > I verified using expirement that modern glibc wrapper getcwd actually never returns "(unreachable)".
> > Also I have read modern glibc sources for all 3 functions documented here.
> > All they don't return "(unreachable)".
> > Also I changed "pathname" to "pathnames".
> >
> > Now let me describe my expirement. I compiled this C source:
> >
> > ===
> > #include <sys/syscall.h>
> > #include <unistd.h>
> > #include <stdio.h>
> >
> > int
> > main (void)
> > {
> > char buf[1000];
> > if(syscall(SYS_getcwd, buf, sizeof(buf)) == -1)
> > {
> > perror ("SYS_getcwd");
> > }
> > else
> > {
> > printf ("SYS_getcwd: %s\n", buf);
> > }
> > if(getcwd(buf, sizeof(buf)) == NULL)
> > {
> > perror ("getcwd");
> > }
> > else
> > {
> > printf ("getcwd: %s\n", buf);
> > }
> > return 0;
> > }
> > ===
> >
> > to a binary /tmp/a and run the following command:
> >
> > $ sudo unshare --mount bash -c 'set -e; mkdir /tmp/dir; mount -t tmpfs tmpfs /tmp/dir; cd /tmp/dir; umount -l .; /tmp/a'
> >
> > The output was:
> >
> > SYS_getcwd: (unreachable)/
> > getcwd: No such file or directory
>
> Thanks! I prefer if we show a continuous shell session, which is easier
> to follow than code mixed with explanations. So I would add the
> following (and indented, to show it's code; that also makes it easier to
> keep the #include lines in git(1)):
>
> alx@devuan:~/tmp$ cat getcwd.c;
> #include <unistd.h>
> #include <stdio.h>
> #include <sys/syscall.h>
>
> int
> main(void)
> {
> char buf[1000];
>
> if (syscall(SYS_getcwd, buf, sizeof(buf)) == -1)
> perror("SYS_getcwd");
> else
> printf("SYS_getcwd: %s\n", buf);
>
> if (getcwd(buf, sizeof(buf)) == NULL)
> perror("getcwd");
> else
> printf("getcwd: %s\n", buf);
>
> return 0;
> }
> alx@devuan:~/tmp$ gcc -Wall -Wextra -o /tmp/getcwd getcwd.c;
> alx@devuan:~/tmp$ sudo unshare --mount bash;
> root@devuan:/home/alx/tmp# set -e;
> root@devuan:/home/alx/tmp# mkdir /tmp/dir;
> root@devuan:/home/alx/tmp# mount -t tmpfs tmpfd /tmp/dir/;
> root@devuan:/home/alx/tmp# cd /tmp/dir/;
> root@devuan:/tmp/dir# umount -l .;
> root@devuan:/tmp/dir# /tmp/getcwd;
> SYS_getcwd: (unreachable)/
> getcwd: No such file or directory
> root@devuan:/tmp/dir# exit;
> exit
>
> I have also adapted the example program to the C coding style of the
> project:
> <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/CONTRIBUTING.d/style/c>
>
> >
> > Signed-off-by: Askar Safin <safinaskar@xxxxxxxxxxxx>
>
> Please add a line for Carlos:
>
> Cc: Carlos O'Donell <carlos@xxxxxxxxxx>
Oh, and please add two entries for the links provided by Carlos.
They'll be useful too:
Link: <https://sourceware.org/bugzilla/show_bug.cgi?id=18203>
Link: <https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=52a713fdd0a30e1bd79818e2e3c4ab44ddca1a94>
>
> > ---
> >
> > Changes since v1:
> > - I added that old glibc versions are buggy, too
> > - Added sources for my expirement to commit message (and did small tweaks to commit message)
> >
> > man/man3/getcwd.3 | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/man/man3/getcwd.3 b/man/man3/getcwd.3
> > index 685585a60..97e3c766f 100644
> > --- a/man/man3/getcwd.3
> > +++ b/man/man3/getcwd.3
> > @@ -245,8 +245,11 @@ process (e.g., because the process set a new filesystem root using
> > without changing its current directory into the new root).
> > Such behavior can also be caused by an unprivileged user by changing
> > the current directory into another mount namespace.
> > -When dealing with pathname from untrusted sources, callers of the
> > -functions described in this page
> > +When dealing with pathnames from untrusted sources, callers of the
>
> As Carlos said, I prefer if this patch doesn't include the change
> s/pathname/&s/. That would reduce the diff.
>
> Thanks for the help with this, Carlos!
>
> > +functions described in this page (until glibc 2.27)
> > +or raw
>
> I think it would be more appropriate to say 's/or raw/or the raw/'.
>
> > +.BR getcwd ()
> > +system call
>
> Other than those minor comments, I think this LGTM. Please address
> those, and I'll take the patch.
>
> Carlos do you have any further comments?
>
>
> Have a lovely day!
> Alex
>
> > should consider checking whether the returned pathname starts
> > with '/' or '(' to avoid misinterpreting an unreachable path
> > as a relative pathname.
> > --
> > 2.39.5
> >
> >
>
> --
> <https://www.alejandro-colomar.es/>
--
<https://www.alejandro-colomar.es/>
Attachment:
signature.asc
Description: PGP signature
