Patchwork linux: make getcwd(3) fail if it cannot obtain an absolute path [BZ #22679]

login
register
mail settings
Submitter Dmitry Levin
Date Jan. 7, 2018, 3:05 a.m.
Message ID <20180107030543.GA7248@altlinux.org>
Download mbox | patch
Permalink /patch/25258/
State New
Headers show

Comments

Dmitry Levin - Jan. 7, 2018, 3:05 a.m.
As the underlying getcwd syscall, starting with linux commit
v2.6.36-rc1~96^2~2, may succeed without returning an absolute path,
check for the path returned by syscall and fail with EACCES if the path
is not absolute.

[BZ #22679]
* sysdeps/unix/sysv/linux/getcwd.c (__getcwd): Set errno to EACCES and
return NULL if the path returned by getcwd syscall is not absolute.
* sysdeps/unix/sysv/linux/tst-getcwd-abspath.c: New test.
* sysdeps/unix/sysv/linux/Makefile (tests): Add tst-getcwd-abspath.
---
 ChangeLog                                    |  8 ++++
 sysdeps/unix/sysv/linux/Makefile             |  2 +-
 sysdeps/unix/sysv/linux/getcwd.c             | 10 +++++
 sysdeps/unix/sysv/linux/tst-getcwd-abspath.c | 59 ++++++++++++++++++++++++++++
 4 files changed, 78 insertions(+), 1 deletion(-)
 create mode 100644 sysdeps/unix/sysv/linux/tst-getcwd-abspath.c
Andreas Schwab - Jan. 7, 2018, 8:23 a.m.
On Jan 07 2018, "Dmitry V. Levin" <ldv@altlinux.org> wrote:

> As the underlying getcwd syscall, starting with linux commit
> v2.6.36-rc1~96^2~2, may succeed without returning an absolute path,
> check for the path returned by syscall and fail with EACCES if the path
> is not absolute.

When we still used /proc we just fell through to the generic getcwd in
that case.

Andreas.
Dmitry Levin - Jan. 7, 2018, 11:33 a.m.
On Sun, Jan 07, 2018 at 09:23:26AM +0100, Andreas Schwab wrote:
> On Jan 07 2018, "Dmitry V. Levin" <ldv@altlinux.org> wrote:
> 
> > As the underlying getcwd syscall, starting with linux commit
> > v2.6.36-rc1~96^2~2, may succeed without returning an absolute path,
> > check for the path returned by syscall and fail with EACCES if the path
> > is not absolute.
> 
> When we still used /proc we just fell through to the generic getcwd in
> that case.

Sure, but the generic getcwd sets errno to ENOENT in this case.
Do you suppose that ENOENT is more appropriate than EACCES?
Andreas Schwab - Jan. 7, 2018, 12:20 p.m.
On Jan 07 2018, "Dmitry V. Levin" <ldv@altlinux.org> wrote:

> Sure, but the generic getcwd sets errno to ENOENT in this case.
> Do you suppose that ENOENT is more appropriate than EACCES?

Yes, the anchor no longer exists.

Andreas.
Dmitry Levin - Jan. 7, 2018, 12:36 p.m.
On Sun, Jan 07, 2018 at 01:20:42PM +0100, Andreas Schwab wrote:
> On Jan 07 2018, "Dmitry V. Levin" <ldv@altlinux.org> wrote:
> 
> > Sure, but the generic getcwd sets errno to ENOENT in this case.
> > Do you suppose that ENOENT is more appropriate than EACCES?
> 
> Yes, the anchor no longer exists.

OK, I'll submit the second edition shortly.
Zack Weinberg - Jan. 7, 2018, 3:53 p.m.
On Sun, Jan 7, 2018 at 7:20 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> On Jan 07 2018, "Dmitry V. Levin" <ldv@altlinux.org> wrote:
>
>> Sure, but the generic getcwd sets errno to ENOENT in this case.
>> Do you suppose that ENOENT is more appropriate than EACCES?
>
> Yes, the anchor no longer exists.

http://pubs.opengroup.org/onlinepubs/9699919799/functions/getcwd.html
does not license getcwd to fail with ENOENT.

zw
Andreas Schwab - Jan. 7, 2018, 4:07 p.m.
On Jan 07 2018, Zack Weinberg <zackw@panix.com> wrote:

> http://pubs.opengroup.org/onlinepubs/9699919799/functions/getcwd.html
> does not license getcwd to fail with ENOENT.

That's not true.  It doesn't specify any condition for ENOENT, thus we
can use it for our purpose.

Andreas.
Zack Weinberg - Jan. 7, 2018, 4:21 p.m.
On Sun, Jan 7, 2018 at 11:07 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> On Jan 07 2018, Zack Weinberg <zackw@panix.com> wrote:
>
>> http://pubs.opengroup.org/onlinepubs/9699919799/functions/getcwd.html
>> does not license getcwd to fail with ENOENT.
>
> That's not true.  It doesn't specify any condition for ENOENT, thus we
> can use it for our purpose.

I thought the ERRORS sections were meant to be exhaustive - no other
errors are allowed.

zw
Dmitry Levin - Jan. 7, 2018, 4:24 p.m.
On Sun, Jan 07, 2018 at 05:07:25PM +0100, Andreas Schwab wrote:
> On Jan 07 2018, Zack Weinberg <zackw@panix.com> wrote:
> 
> > http://pubs.opengroup.org/onlinepubs/9699919799/functions/getcwd.html
> > does not license getcwd to fail with ENOENT.
> 
> That's not true.  It doesn't specify any condition for ENOENT, thus we
> can use it for our purpose.

In fact, we already use ENOENT when the current directory is unlinked,
and making getcwd(3) fail with ENOENT when it cannot obtain an absolute
path would be consistent with that traditional case.
Dmitry Levin - Jan. 7, 2018, 5:07 p.m.
On Sun, Jan 07, 2018 at 11:21:19AM -0500, Zack Weinberg wrote:
> On Sun, Jan 7, 2018 at 11:07 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> > On Jan 07 2018, Zack Weinberg <zackw@panix.com> wrote:
> >
> >> http://pubs.opengroup.org/onlinepubs/9699919799/functions/getcwd.html
> >> does not license getcwd to fail with ENOENT.
> >
> > That's not true.  It doesn't specify any condition for ENOENT, thus we
> > can use it for our purpose.
> 
> I thought the ERRORS sections were meant to be exhaustive - no other
> errors are allowed.

Just the otherwise: "Implementations may support additional errors not
included in this list, may generate errors included in this list under
circumstances other than those described here, or may contain extensions
or limitations that prevent some errors from occurring."
Zack Weinberg - Jan. 7, 2018, 8:04 p.m.
On Sun, Jan 7, 2018 at 12:07 PM, Dmitry V. Levin <ldv@altlinux.org> wrote:
> On Sun, Jan 07, 2018 at 11:21:19AM -0500, Zack Weinberg wrote:
>>
>> I thought the ERRORS sections were meant to be exhaustive - no other
>> errors are allowed.
>
> Just the otherwise: "Implementations may support additional errors not
> included in this list, may generate errors included in this list under
> circumstances other than those described here, or may contain extensions
> or limitations that prevent some errors from occurring."

OK, objection withdrawn.
Florian Weimer - Feb. 5, 2018, 6:49 p.m.
On 01/07/2018 04:05 AM, Dmitry V. Levin wrote:
> As the underlying getcwd syscall, starting with linux commit
> v2.6.36-rc1~96^2~2, may succeed without returning an absolute path,
> check for the path returned by syscall and fail with EACCES if the path
> is not absolute.

This appears to cause a failure in a gluster component:

https://lists.gluster.org/pipermail/gluster-users/2018-January/033293.html

I have a longer strace, but it is quite difficult to read because of the 
way chdir is used in a multi-threaded process.

What seems to happen is that we execve rsync with a current directory 
that has been lazily unmounted.  The current directory after the execve 
is unreachable from theroot, and we run into this rsync failure path

/* Like chdir(), but it keeps track of the current directory (in the
  * global "curr_dir"), and ensures that the path size doesn't overflow.
  * Also cleans the path using the clean_fname() function. */
int change_dir(const char *dir, int set_path_only)
{
	static int initialised, skipped_chdir;
	unsigned int len;

	if (!initialised) {
		initialised = 1;
		if (getcwd(curr_dir, sizeof curr_dir - 1) == NULL) {
			rsyserr(FERROR, errno, "getcwd()");
			exit_cleanup(RERR_FILESELECT);
		}
		curr_dir_len = strlen(curr_dir);
	}

during initialization of the rsync process.  This happens even if the 
current directory is never used because all rsync paths are absolute, 
and also with --ignore-missing-args:

There has been another issue in this area:

http://lists.gluster.org/pipermail/gluster-users/2017-April/030534.html

This was worked around with --ignore-missing-args, but rsync never gets 
to this point after the getcwd change.

I don't see how we can avoid fixing rsync (probably by caching the 
getcwd failure during initialization and reporting it only if the 
directory is used afterwards).

Any comments?

Thanks,
Florian
Andreas Schwab - Feb. 5, 2018, 7:03 p.m.
On Feb 05 2018, Florian Weimer <fweimer@redhat.com> wrote:

> /* Like chdir(), but it keeps track of the current directory (in the
>  * global "curr_dir"), and ensures that the path size doesn't overflow.
>  * Also cleans the path using the clean_fname() function. */
> int change_dir(const char *dir, int set_path_only)
> {
> 	static int initialised, skipped_chdir;
> 	unsigned int len;
>
> 	if (!initialised) {
> 		initialised = 1;
> 		if (getcwd(curr_dir, sizeof curr_dir - 1) == NULL) {
> 			rsyserr(FERROR, errno, "getcwd()");
> 			exit_cleanup(RERR_FILESELECT);
> 		}
> 		curr_dir_len = strlen(curr_dir);
> 	}

This will also fail when rsync is started from a directory that has
since been removed, where getcwd (even the syscall) has always returned
an error.  So this isn't a new failure.

Andreas.
Florian Weimer - Feb. 5, 2018, 7:18 p.m.
On 02/05/2018 08:03 PM, Andreas Schwab wrote:
> On Feb 05 2018, Florian Weimer <fweimer@redhat.com> wrote:
> 
>> /* Like chdir(), but it keeps track of the current directory (in the
>>   * global "curr_dir"), and ensures that the path size doesn't overflow.
>>   * Also cleans the path using the clean_fname() function. */
>> int change_dir(const char *dir, int set_path_only)
>> {
>> 	static int initialised, skipped_chdir;
>> 	unsigned int len;
>>
>> 	if (!initialised) {
>> 		initialised = 1;
>> 		if (getcwd(curr_dir, sizeof curr_dir - 1) == NULL) {
>> 			rsyserr(FERROR, errno, "getcwd()");
>> 			exit_cleanup(RERR_FILESELECT);
>> 		}
>> 		curr_dir_len = strlen(curr_dir);
>> 	}
> 
> This will also fail when rsync is started from a directory that has
> since been removed, where getcwd (even the syscall) has always returned
> an error.  So this isn't a new failure.

That's a good point.  I still don't see how we can avoid fixing rsync.

Thanks,
Florian
Dmitry Levin - Feb. 5, 2018, 11:58 p.m.
On Mon, Feb 05, 2018 at 08:18:46PM +0100, Florian Weimer wrote:
> On 02/05/2018 08:03 PM, Andreas Schwab wrote:
> > On Feb 05 2018, Florian Weimer <fweimer@redhat.com> wrote:
> > 
> >> /* Like chdir(), but it keeps track of the current directory (in the
> >>   * global "curr_dir"), and ensures that the path size doesn't overflow.
> >>   * Also cleans the path using the clean_fname() function. */
> >> int change_dir(const char *dir, int set_path_only)
> >> {
> >> 	static int initialised, skipped_chdir;
> >> 	unsigned int len;
> >>
> >> 	if (!initialised) {
> >> 		initialised = 1;
> >> 		if (getcwd(curr_dir, sizeof curr_dir - 1) == NULL) {
> >> 			rsyserr(FERROR, errno, "getcwd()");
> >> 			exit_cleanup(RERR_FILESELECT);
> >> 		}
> >> 		curr_dir_len = strlen(curr_dir);
> >> 	}
> > 
> > This will also fail when rsync is started from a directory that has
> > since been removed, where getcwd (even the syscall) has always returned
> > an error.  So this isn't a new failure.
> 
> That's a good point.  I still don't see how we can avoid fixing rsync.

I don't see why rsync shouldn't be fixed.
Florian Weimer - Feb. 6, 2018, 1:56 p.m.
On 02/06/2018 12:58 AM, Dmitry V. Levin wrote:
> On Mon, Feb 05, 2018 at 08:18:46PM +0100, Florian Weimer wrote:
>> On 02/05/2018 08:03 PM, Andreas Schwab wrote:
>>> This will also fail when rsync is started from a directory that has
>>> since been removed, where getcwd (even the syscall) has always returned
>>> an error.  So this isn't a new failure.
>>
>> That's a good point.  I still don't see how we can avoid fixing rsync.
> 
> I don't see why rsync shouldn't be fixed.

Well, it was a bit of a struggle, but I finally posted:

   https://lists.samba.org/archive/rsync/2018-February/031478.html

Florian

Patch

diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index 8f19e0e..34c5c39 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -45,7 +45,7 @@  sysdep_headers += sys/mount.h sys/acct.h sys/sysctl.h \
 tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \
 	 tst-quota tst-sync_file_range tst-sysconf-iov_max tst-ttyname \
 	 test-errno-linux tst-memfd_create tst-mlock2 tst-pkey \
-	 tst-rlimit-infinity
+	 tst-rlimit-infinity tst-getcwd-abspath
 
 # Generate the list of SYS_* macros for the system calls (__NR_*
 # macros).  The file syscall-names.list contains all possible system
diff --git a/sysdeps/unix/sysv/linux/getcwd.c b/sysdeps/unix/sysv/linux/getcwd.c
index f545106..2a4320d 100644
--- a/sysdeps/unix/sysv/linux/getcwd.c
+++ b/sysdeps/unix/sysv/linux/getcwd.c
@@ -78,6 +78,16 @@  __getcwd (char *buf, size_t size)
   retval = INLINE_SYSCALL (getcwd, 2, path, alloc_size);
   if (retval >= 0)
     {
+      if (retval == 0 || path[0] != '/')
+	{
+#ifndef NO_ALLOCATION
+	  if (buf == NULL && size == 0)
+	    free (path);
+#endif
+	  __set_errno (EACCES);
+	  return NULL;
+	}
+
 #ifndef NO_ALLOCATION
       if (buf == NULL && size == 0)
 	/* Ensure that the buffer is only as large as necessary.  */
diff --git a/sysdeps/unix/sysv/linux/tst-getcwd-abspath.c b/sysdeps/unix/sysv/linux/tst-getcwd-abspath.c
new file mode 100644
index 0000000..2a81bbd
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/tst-getcwd-abspath.c
@@ -0,0 +1,59 @@ 
+/* BZ #22679 getcwd(3) does not succeed without returning an absolute path.
+
+   Copyright (C) 2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <support/check.h>
+#include <support/namespace.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/test-driver.h>
+#include <support/xunistd.h>
+#include <unistd.h>
+
+static char *chroot_dir;
+
+/* The actual test.  Run it in a subprocess, so that the test harness
+   can remove the temporary directory in --direct mode.  */
+static void
+getcwd_callback (void *closure)
+{
+  xchroot (chroot_dir);
+  errno = 0;
+  char *cwd = getcwd (NULL, 0);
+  TEST_VERIFY (cwd == NULL);
+  TEST_VERIFY (errno == EACCES);
+  _exit (0);
+}
+
+static int
+do_test (void)
+{
+  support_become_root ();
+  if (!support_can_chroot ())
+    return EXIT_UNSUPPORTED;
+
+  chroot_dir = support_create_temp_directory ("tst-getcwd-abspath-");
+  support_isolate_in_subprocess (getcwd_callback, NULL);
+
+  return 0;
+}
+
+#include <support/test-driver.c>