NetBSD Problem Report #56398

From www@netbsd.org  Mon Sep 13 01:32:16 2021
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id AC0EF1A9239
	for <gnats-bugs@gnats.NetBSD.org>; Mon, 13 Sep 2021 01:32:16 +0000 (UTC)
Message-Id: <20210913013215.0F1B31A923A@mollari.NetBSD.org>
Date: Mon, 13 Sep 2021 01:32:14 +0000 (UTC)
From: carenas@gmail.com
Reply-To: carenas@gmail.com
To: gnats-bugs@NetBSD.org
Subject: `mkdir -p -m` fails to assign the correct mode with trailing slashes
X-Send-Pr-Version: www-1.0

>Number:         56398
>Category:       bin
>Synopsis:       `mkdir -p -m` fails to assign the correct mode with trailing slashes
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    bin-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Sep 13 01:35:00 +0000 2021
>Closed-Date:    Thu Sep 23 11:54:05 +0000 2021
>Last-Modified:  Thu Sep 23 11:54:05 +0000 2021
>Originator:     Carlo Arenas
>Release:        all versions since 2003
>Organization:
>Environment:
NetBSD somehost 9.99.88 NetBSD 9.99.88 (GENERIC) #0: Sun Sep 12 02:20:36 UTC 2021  mkrepro@mkrepro.NetBSD.org:/usr/src/sys/arch/amd64/compile/GENERIC amd64
>Description:
as part of the git testsuite, a test creates a directory where a secure daemon is then started and communicated through a Unix Socket.

the creation of the directory uses :

  mkdir -p -m 700 $HOME/dir/

but the daemon fails to start because its directory is not secure, therefore failing the test.

removing the trailing '/' workarounds the problem of course, but the issue should be better fixed at the source.

the problem is NetBSD specific AFAIK, and was probably introduced in revision 1.33 
>How-To-Repeat:
diff -urN tests/bin/mkdir/Makefile tests/bin/mkdir/Makefile
--- tests/bin/mkdir/Makefile    1970-01-01 00:00:00.000000000 +0000
+++ tests/bin/mkdir/Makefile    2021-09-12 13:47:31.100158695 +0000
@@ -0,0 +1,6 @@
+.include <bsd.own.mk>
+
+TESTSDIR=      ${TESTSBASE}/bin/mkdir
+TESTS_SH=      t_mkdir
+
+.include <bsd.test.mk>
diff -urN tests/bin/mkdir/t_mkdir.sh tests/bin/mkdir/t_mkdir.sh
--- tests/bin/mkdir/t_mkdir.sh  1970-01-01 00:00:00.000000000 +0000
+++ tests/bin/mkdir/t_mkdir.sh  2021-09-12 14:43:39.719053893 +0000
@@ -0,0 +1,20 @@
+atf_test_case p_and_m
+p_and_m_head() {
+       atf_set "descr" "Ensure that mkdir -p -m keeps track correctly" \
+                       "of the last entry even when trailing /"
+       atf_set "use.fs" "true"
+}
+p_and_m_body() {
+       mode="700"
+       atf_check -s eq:0 -o empty -e empty mkdir -p -m $mode foo/bar/
+
+       # ensure that mode was correctly set
+       xmode=$(stat -f "%p" foo/bar)
+       xmode=${xmode#40}
+       atf_check_equal $mode $xmode
+}
+
+atf_init_test_cases()
+{
+       atf_add_test_case p_and_m
+}
>Fix:
Index: bin/mkdir/mkdir.c
===================================================================
RCS file: /cvsroot/src/bin/mkdir/mkdir.c,v
retrieving revision 1.38
diff -u -r1.38 mkdir.c
--- bin/mkdir/mkdir.c   29 Aug 2011 14:45:28 -0000      1.38
+++ bin/mkdir/mkdir.c   13 Sep 2021 00:45:47 -0000
@@ -106,16 +106,6 @@
        }

        for (exitval = EXIT_SUCCESS; *argv != NULL; ++argv) {
-#ifdef notdef
-               char *slash;
-
-               /* Kernel takes care of this */
-               /* Remove trailing slashes, per POSIX. */
-               slash = strrchr(*argv, '\0');
-               while (--slash > *argv && *slash == '/')
-                       *slash = '\0';
-#endif
-
                if (pflag) {
                        if (mkpath(*argv, mode, dir_mode) < 0)
                                exitval = EXIT_FAILURE;
@@ -155,6 +145,10 @@
        char *slash;
        int done, rv;

+       slash = strrchr(path, '\0');
+       while (--slash > path && *slash == '/')
+               *slash = '\0';
+
        done = 0;
        slash = path;

Index: etc/mtree/NetBSD.dist.tests
===================================================================
RCS file: /cvsroot/src/etc/mtree/NetBSD.dist.tests,v
retrieving revision 1.188
diff -u -r1.188 NetBSD.dist.tests
--- etc/mtree/NetBSD.dist.tests 29 Aug 2021 09:54:18 -0000      1.188
+++ etc/mtree/NetBSD.dist.tests 13 Sep 2021 00:45:49 -0000
@@ -197,6 +197,7 @@
 ./usr/tests/bin/dd
 ./usr/tests/bin/df
 ./usr/tests/bin/expr
+./usr/tests/bin/mkdir
 ./usr/tests/bin/pax
 ./usr/tests/bin/ps
 ./usr/tests/bin/sleep
Index: tests/bin/Makefile
===================================================================
RCS file: /cvsroot/src/tests/bin/Makefile,v
retrieving revision 1.4
diff -u -r1.4 Makefile
--- tests/bin/Makefile  3 Jul 2020 03:59:18 -0000       1.4
+++ tests/bin/Makefile  13 Sep 2021 00:46:03 -0000
@@ -4,6 +4,6 @@

 TESTSDIR=       ${TESTSBASE}/bin

-TESTS_SUBDIRS= cat cp date dd df expr pax ps sh sleep tar
+TESTS_SUBDIRS= cat cp date dd df expr mkdir pax ps sh sleep tar

 .include <bsd.test.mk>

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: bin-bug-people->kern-bug-people
Responsible-Changed-By: martin@NetBSD.org
Responsible-Changed-When: Mon, 13 Sep 2021 05:54:31 +0000
Responsible-Changed-Why:
This is a kernel bug


From: mlelstv@serpens.de (Michael van Elst)
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: kern/56398 (`mkdir -p -m` fails to assign the correct mode with trailing slashes)
Date: Mon, 13 Sep 2021 07:40:44 -0000 (UTC)

 martin@NetBSD.org writes:

 >Synopsis: `mkdir -p -m` fails to assign the correct mode with trailing slashes

 >Responsible-Changed-From-To: bin-bug-people->kern-bug-people
 >Responsible-Changed-By: martin@NetBSD.org
 >Responsible-Changed-When: Mon, 13 Sep 2021 05:54:31 +0000
 >Responsible-Changed-Why:
 >This is a kernel bug

 I don't think so.

 mkdir -p creates upper directories with 'dir_mode' and the last
 element with 'mode'.

 When parsing a path like a/b/c you get:

 mkdir a        with 'dir_mode'
 mkdir a/b      with 'dir_mode'
 mkdir a/b/c    <- last element with 'mode'

 The mode of each element is set by the mkdir() call (unless, for
 the last element stick,setuid or setgid bits are included).

 When parsing a path like a/b/c/ you get:

 mkdir a        with 'dir_mode'
 mkdir a/b      with 'dir_mode'
 mkdir a/b/c    with 'dir_mode'
 mkdir a/b/c/   <- last element with 'mode'

 The last path already exists, mkdir checks if the path exists
 and is a directory and ignores it then.

 There is a 'notdef' part that strips trailing slashes which would
 avoid this issue. Not sure why it is commented out, probably some
 unwanted side effect. It's probably better to improve the mkpath
 parser.

Responsible-Changed-From-To: kern-bug-people->bin-bug-people
Responsible-Changed-By: kre@NetBSD.org
Responsible-Changed-When: Mon, 13 Sep 2021 09:31:20 +0000
Responsible-Changed-Why:
I agree with mlelstv@, not a kernel bug
returning responsibility for this to bin-bug-people


From: Martin Husemann <martin@duskware.de>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: kern/56398 (`mkdir -p -m` fails to assign the correct mode with
 trailing slashes)
Date: Mon, 13 Sep 2021 11:53:53 +0200

 I agree with mlelstv too (I overlooked the -p intermediate elements handling).

 I think the change was done after the kernel was fixed to ignore trailing
 slash on directory components, i.e. in ancient times there were
 differences between

 	chmod a-wx test/dir/.
 	chmod a-wx test/dir/
 	chmod a-wx test/dir

 and nowadys there shouldn't - but unrelated to this, as mlelstv said:
 the parser should be enhanced instead of plain stripping the trailing slashes.

 Martin

From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: kern/56398 (`mkdir -p -m` fails to assign the correct mode with trailing slashes)
Date: Tue, 14 Sep 2021 00:21:39 +0700

     Date:        Mon, 13 Sep 2021 07:50:01 +0000 (UTC)
     From:        mlelstv@serpens.de (Michael van Elst)
     Message-ID:  <20210913075001.B2EB71A923A@mollari.NetBSD.org>

   | It's probably better to improve the mkpath parser.

 Agreed, I am currently building a system with a trivial change to that
 which I think will solve the problem (without stripping trailing slashes
 anywhere).   It simply alters the computation to determine "done"

 Assuming all is fine after it is built and tested, I will commit the
 change.



State-Changed-From-To: open->pending-pullups
State-Changed-By: kre@NetBSD.org
State-Changed-When: Mon, 13 Sep 2021 22:47:16 +0000
State-Changed-Why:
Neets pullups to -9 and -8


From: "Robert Elz" <kre@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56398 CVS commit: src/bin/mkdir
Date: Mon, 13 Sep 2021 22:46:02 +0000

 Module Name:	src
 Committed By:	kre
 Date:		Mon Sep 13 22:46:02 UTC 2021

 Modified Files:
 	src/bin/mkdir: mkdir.c

 Log Message:
 PR bin/56398

 The final component of both a/b/c and a/b/c/ is "c", so that's the one
 to which the -m arg applies.


 To generate a diff of this commit:
 cvs rdiff -u -r1.38 -r1.39 src/bin/mkdir/mkdir.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: bin/56398: `mkdir -p -m` fails to assign the correct mode with trailing slashes
Date: Tue, 14 Sep 2021 06:04:53 +0700

 Note that I did not just forget the ATF test supplied in the PR,
 I shall add one of those soon (a day or two I hope) - but it needs
 to be rather more than the one supplied.

 That part of this probably won't get pulled up to earlier releases.


State-Changed-From-To: pending-pullups->needs-pullups
State-Changed-By: kre@NetBSD.org
State-Changed-When: Wed, 22 Sep 2021 22:57:41 +0000
State-Changed-Why:
Oops, this should have been in needs-pullups state, move it
back to that so I can then put it back to pending-pullups with
a note containing the pullup request numbers...


State-Changed-From-To: needs-pullups->pending-pullups
State-Changed-By: kre@NetBSD.org
State-Changed-When: Wed, 22 Sep 2021 22:58:12 +0000
State-Changed-Why:
Pullups requested: [pullup-9 #1347] [pullup-8 #1697]


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56398 CVS commit: [netbsd-8] src/bin/mkdir
Date: Thu, 23 Sep 2021 10:04:53 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Thu Sep 23 10:04:53 UTC 2021

 Modified Files:
 	src/bin/mkdir [netbsd-8]: mkdir.c

 Log Message:
 Pull up following revision(s) (requested by kre in ticket #1697):

 	bin/mkdir/mkdir.c: revision 1.39

 PR bin/56398

 The final component of both a/b/c and a/b/c/ is "c", so that's the one
 to which the -m arg applies.


 To generate a diff of this commit:
 cvs rdiff -u -r1.38 -r1.38.36.1 src/bin/mkdir/mkdir.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56398 CVS commit: [netbsd-9] src/bin/mkdir
Date: Thu, 23 Sep 2021 10:09:20 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Thu Sep 23 10:09:20 UTC 2021

 Modified Files:
 	src/bin/mkdir [netbsd-9]: mkdir.c

 Log Message:
 Pull up following revision(s) (requested by kre in ticket #1347):

 	bin/mkdir/mkdir.c: revision 1.39

 PR bin/56398

 The final component of both a/b/c and a/b/c/ is "c", so that's the one
 to which the -m arg applies.


 To generate a diff of this commit:
 cvs rdiff -u -r1.38 -r1.38.46.1 src/bin/mkdir/mkdir.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: pending-pullups->closed
State-Changed-By: kre@NetBSD.org
State-Changed-When: Thu, 23 Sep 2021 11:54:05 +0000
State-Changed-Why:
Pullups completed


>Unformatted:

Home	PR Database Search