[patch] Fix path length overflow in realpath (BZ#22786)

[patch] Fix path length overflow in realpath (BZ#22786)

[Paul Pluzhnikov]

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

Greetings,

Attached is a trivial fix, and a test case.

Thanks,

2018-04-09  Paul Pluzhnikov  <ppluzhnikov@google.com>

         [BZ #22786]
         * stdlib/canonicalize.c (__realpath): Fix overflow in path length
         computation.
         * stdlib/Makefile (test-bz22786): New test.
         * stdlib/test-bz22786.c: New test.

-- 
Paul Pluzhnikov

[-- Attachment #2: glibc-bz22786-20180409.txt --]
[-- Type: text/plain, Size: 3802 bytes --]

diff --git a/stdlib/Makefile b/stdlib/Makefile
index af1643c0c4..d04afd62c8 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -84,7 +84,7 @@ tests		:= tst-strtol tst-strtod testmb testrand testsort testdiv   \
 		   tst-cxa_atexit tst-on_exit test-atexit-race 		    \
 		   test-at_quick_exit-race test-cxa_atexit-race             \
 		   test-on_exit-race test-dlclose-exit-race 		    \
-		   tst-makecontext-align
+		   tst-makecontext-align test-bz22786
 
 tests-internal	:= tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
 		   tst-tls-atexit tst-tls-atexit-nodelete
@@ -156,6 +156,9 @@ CFLAGS-tst-qsort.c += $(stack-align-test-flags)
 CFLAGS-tst-makecontext.c += -funwind-tables
 CFLAGS-tst-makecontext2.c += $(stack-align-test-flags)
 
+# suppress warnings about allocation size.
+CFLAGS-test-bz22786.c += $(+gcc-nowarn)
+
 # Run a test on the header files we use.
 tests-special += $(objpfx)isomac.out
 
diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c
index 4135f3f33c..390fb437a8 100644
--- a/stdlib/canonicalize.c
+++ b/stdlib/canonicalize.c
@@ -181,7 +181,7 @@ __realpath (const char *name, char *resolved)
 		extra_buf = __alloca (path_max);
 
 	      len = strlen (end);
-	      if ((long int) (n + len) >= path_max)
+	      if (path_max - n <= len)
 		{
 		  __set_errno (ENAMETOOLONG);
 		  goto error;
diff --git a/stdlib/test-bz22786.c b/stdlib/test-bz22786.c
new file mode 100644
index 0000000000..329c4297ce
--- /dev/null
+++ b/stdlib/test-bz22786.c
@@ -0,0 +1,80 @@
+/* Bug 22786: test for stack overflow in realpath.
+   Copyright (C) 2017-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
+   <http://www.gnu.org/licenses/>.  */
+
+/* This file must be run from within a directory called "stdlib".  */
+
+#include <errno.h>
+#include <limits.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+
+static int
+do_test (void)
+{
+  const char dir[] = "bz22786";
+  const char lnk[] = "bz22786/symlink";
+
+  rmdir (dir);
+  if (mkdir (dir, 0755) != 0 && errno != EEXIST)
+    {
+      printf ("mkdir %s: %m\n", dir);
+      return EXIT_FAILURE;
+    }
+  if (symlink (".", lnk) != 0 && errno != EEXIST)
+    {
+      printf ("symlink (%s, %s): %m\n", dir, lnk);
+      return EXIT_FAILURE;
+    }
+
+  const size_t path_len = (size_t) INT_MAX + 1;
+  char *path = malloc(path_len);
+
+  if (path == NULL)
+    {
+      printf ("malloc (%zu): %m\n", path_len);
+      return EXIT_FAILURE;
+    }
+
+  /* Construct very long path = "bz22786/symlink/aaaa....."  */
+  char *p = mempcpy (path, lnk, sizeof (lnk) - 1);
+  *(p++) = '/';
+  memset (p, 'a', path_len - (path - p) - 2);
+  p[path_len - (path - p) - 1] = '\0';
+
+  /* This call crashes before the fix for bz22786 on 32-bit platforms.  */
+  p = realpath (path, NULL);
+
+  if (p != NULL || errno != ENAMETOOLONG)
+    {
+      printf ("realpath: %s (%m)", p);
+      return EXIT_FAILURE;
+    }
+
+  /* Cleanup.  */
+  unlink (lnk);
+  rmdir (dir);
+
+  return 0;
+}
+
+#define TEST_FUNCTION do_test
+#include <support/test-driver.c>

Re: [patch] Fix path length overflow in realpath (BZ#22786)

[Paul Pluzhnikov]

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

+  const size_t path_len = (size_t) INT_MAX + 1;
+  char *path = malloc(path_len);

Sorry, missed space before parenth here.
Updated patch attached.

On Mon, Apr 9, 2018 at 4:01 PM Paul Pluzhnikov <ppluzhnikov@google.com>
wrote:

> Greetings,

> Attached is a trivial fix, and a test case.

> Thanks,

> 2018-04-09  Paul Pluzhnikov  <ppluzhnikov@google.com>

>           [BZ #22786]
>           * stdlib/canonicalize.c (__realpath): Fix overflow in path length
>           computation.
>           * stdlib/Makefile (test-bz22786): New test.
>           * stdlib/test-bz22786.c: New test.

> --
> Paul Pluzhnikov



--
Paul Pluzhnikov

[-- Attachment #2: glibc-bz22786-20180409a.txt --]
[-- Type: text/plain, Size: 3803 bytes --]

diff --git a/stdlib/Makefile b/stdlib/Makefile
index af1643c0c4..d04afd62c8 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -84,7 +84,7 @@ tests		:= tst-strtol tst-strtod testmb testrand testsort testdiv   \
 		   tst-cxa_atexit tst-on_exit test-atexit-race 		    \
 		   test-at_quick_exit-race test-cxa_atexit-race             \
 		   test-on_exit-race test-dlclose-exit-race 		    \
-		   tst-makecontext-align
+		   tst-makecontext-align test-bz22786
 
 tests-internal	:= tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
 		   tst-tls-atexit tst-tls-atexit-nodelete
@@ -156,6 +156,9 @@ CFLAGS-tst-qsort.c += $(stack-align-test-flags)
 CFLAGS-tst-makecontext.c += -funwind-tables
 CFLAGS-tst-makecontext2.c += $(stack-align-test-flags)
 
+# suppress warnings about allocation size.
+CFLAGS-test-bz22786.c += $(+gcc-nowarn)
+
 # Run a test on the header files we use.
 tests-special += $(objpfx)isomac.out
 
diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c
index 4135f3f33c..390fb437a8 100644
--- a/stdlib/canonicalize.c
+++ b/stdlib/canonicalize.c
@@ -181,7 +181,7 @@ __realpath (const char *name, char *resolved)
 		extra_buf = __alloca (path_max);
 
 	      len = strlen (end);
-	      if ((long int) (n + len) >= path_max)
+	      if (path_max - n <= len)
 		{
 		  __set_errno (ENAMETOOLONG);
 		  goto error;
diff --git a/stdlib/test-bz22786.c b/stdlib/test-bz22786.c
new file mode 100644
index 0000000000..504535bbbd
--- /dev/null
+++ b/stdlib/test-bz22786.c
@@ -0,0 +1,80 @@
+/* Bug 22786: test for stack overflow in realpath.
+   Copyright (C) 2017-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
+   <http://www.gnu.org/licenses/>.  */
+
+/* This file must be run from within a directory called "stdlib".  */
+
+#include <errno.h>
+#include <limits.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+
+static int
+do_test (void)
+{
+  const char dir[] = "bz22786";
+  const char lnk[] = "bz22786/symlink";
+
+  rmdir (dir);
+  if (mkdir (dir, 0755) != 0 && errno != EEXIST)
+    {
+      printf ("mkdir %s: %m\n", dir);
+      return EXIT_FAILURE;
+    }
+  if (symlink (".", lnk) != 0 && errno != EEXIST)
+    {
+      printf ("symlink (%s, %s): %m\n", dir, lnk);
+      return EXIT_FAILURE;
+    }
+
+  const size_t path_len = (size_t) INT_MAX + 1;
+  char *path = malloc (path_len);
+
+  if (path == NULL)
+    {
+      printf ("malloc (%zu): %m\n", path_len);
+      return EXIT_FAILURE;
+    }
+
+  /* Construct very long path = "bz22786/symlink/aaaa....."  */
+  char *p = mempcpy (path, lnk, sizeof (lnk) - 1);
+  *(p++) = '/';
+  memset (p, 'a', path_len - (path - p) - 2);
+  p[path_len - (path - p) - 1] = '\0';
+
+  /* This call crashes before the fix for bz22786 on 32-bit platforms.  */
+  p = realpath (path, NULL);
+
+  if (p != NULL || errno != ENAMETOOLONG)
+    {
+      printf ("realpath: %s (%m)", p);
+      return EXIT_FAILURE;
+    }
+
+  /* Cleanup.  */
+  unlink (lnk);
+  rmdir (dir);
+
+  return 0;
+}
+
+#define TEST_FUNCTION do_test
+#include <support/test-driver.c>

Re: [patch] Fix path length overflow in realpath (BZ#22786)

[Andreas Schwab]

On Apr 10 2018, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

> +  const size_t path_len = (size_t) INT_MAX + 1;
> +  char *path = malloc (path_len);
> +
> +  if (path == NULL)
> +    {
> +      printf ("malloc (%zu): %m\n", path_len);
> +      return EXIT_FAILURE;
> +    }

Trying to allocate a block of INT_MAX+1 is rather likely to fail on a
32-bit platform.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [patch] Fix path length overflow in realpath (BZ#22786)

[Paul Pluzhnikov]

On Tue, Apr 10, 2018 at 1:08 AM Andreas Schwab <schwab@suse.de> wrote:

> Trying to allocate a block of INT_MAX+1 is rather likely to fail on a
> 32-bit platform.

But that's the only way to test for this overflow AFAICT.

Should I submit the fix without the test?
Should I submit the fix and the test, but disabled?
Should I change the test to pass if allocation fails?

Thanks,
--
Paul Pluzhnikov

Re: [patch] Fix path length overflow in realpath (BZ#22786)

[Andreas Schwab]

On Apr 10 2018, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

> Should I change the test to pass if allocation fails?

No, unsupported.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [patch] Fix path length overflow in realpath (BZ#22786)

[Paul Pluzhnikov]

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

On Tue, Apr 10, 2018 at 8:15 AM Andreas Schwab <schwab@suse.de> wrote:

> On Apr 10 2018, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

> > Should I change the test to pass if allocation fails?

> No, unsupported.

Right. Revised patch attached.

--
Paul Pluzhnikov

[-- Attachment #2: glibc-bz22786-20180410.txt --]
[-- Type: text/plain, Size: 3836 bytes --]

diff --git a/stdlib/Makefile b/stdlib/Makefile
index af1643c0c4..d04afd62c8 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -84,7 +84,7 @@ tests		:= tst-strtol tst-strtod testmb testrand testsort testdiv   \
 		   tst-cxa_atexit tst-on_exit test-atexit-race 		    \
 		   test-at_quick_exit-race test-cxa_atexit-race             \
 		   test-on_exit-race test-dlclose-exit-race 		    \
-		   tst-makecontext-align
+		   tst-makecontext-align test-bz22786
 
 tests-internal	:= tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
 		   tst-tls-atexit tst-tls-atexit-nodelete
@@ -156,6 +156,9 @@ CFLAGS-tst-qsort.c += $(stack-align-test-flags)
 CFLAGS-tst-makecontext.c += -funwind-tables
 CFLAGS-tst-makecontext2.c += $(stack-align-test-flags)
 
+# suppress warnings about allocation size.
+CFLAGS-test-bz22786.c += $(+gcc-nowarn)
+
 # Run a test on the header files we use.
 tests-special += $(objpfx)isomac.out
 
diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c
index 4135f3f33c..390fb437a8 100644
--- a/stdlib/canonicalize.c
+++ b/stdlib/canonicalize.c
@@ -181,7 +181,7 @@ __realpath (const char *name, char *resolved)
 		extra_buf = __alloca (path_max);
 
 	      len = strlen (end);
-	      if ((long int) (n + len) >= path_max)
+	      if (path_max - n <= len)
 		{
 		  __set_errno (ENAMETOOLONG);
 		  goto error;
diff --git a/stdlib/test-bz22786.c b/stdlib/test-bz22786.c
new file mode 100644
index 0000000000..7b41a0fe5e
--- /dev/null
+++ b/stdlib/test-bz22786.c
@@ -0,0 +1,81 @@
+/* Bug 22786: test for stack overflow in realpath.
+   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
+   <http://www.gnu.org/licenses/>.  */
+
+/* This file must be run from within a directory called "stdlib".  */
+
+#include <errno.h>
+#include <limits.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <support/test-driver.h>
+
+static int
+do_test (void)
+{
+  const char dir[] = "bz22786";
+  const char lnk[] = "bz22786/symlink";
+
+  rmdir (dir);
+  if (mkdir (dir, 0755) != 0 && errno != EEXIST)
+    {
+      printf ("mkdir %s: %m\n", dir);
+      return EXIT_FAILURE;
+    }
+  if (symlink (".", lnk) != 0 && errno != EEXIST)
+    {
+      printf ("symlink (%s, %s): %m\n", dir, lnk);
+      return EXIT_FAILURE;
+    }
+
+  const size_t path_len = (size_t) INT_MAX + 1;
+  char *path = malloc (path_len);
+
+  if (path == NULL)
+    {
+      printf ("malloc (%zu): %m\n", path_len);
+      return EXIT_UNSUPPORTED;
+    }
+
+  /* Construct very long path = "bz22786/symlink/aaaa....."  */
+  char *p = mempcpy (path, lnk, sizeof (lnk) - 1);
+  *(p++) = '/';
+  memset (p, 'a', path_len - (path - p) - 2);
+  p[path_len - (path - p) - 1] = '\0';
+
+  /* This call crashes before the fix for bz22786 on 32-bit platforms.  */
+  p = realpath (path, NULL);
+
+  if (p != NULL || errno != ENAMETOOLONG)
+    {
+      printf ("realpath: %s (%m)", p);
+      return EXIT_FAILURE;
+    }
+
+  /* Cleanup.  */
+  unlink (lnk);
+  rmdir (dir);
+
+  return 0;
+}
+
+#define TEST_FUNCTION do_test
+#include <support/test-driver.c>

Re: [patch] Fix path length overflow in realpath (BZ#22786)

[Alexander Monakov]

On Tue, 10 Apr 2018, Paul Pluzhnikov wrote:
> On Tue, Apr 10, 2018 at 1:08 AM Andreas Schwab <schwab@suse.de> wrote:
> 
> > Trying to allocate a block of INT_MAX+1 is rather likely to fail on a
> > 32-bit platform.
> 
> But that's the only way to test for this overflow AFAICT.
> 
> Should I submit the fix without the test?
> Should I submit the fix and the test, but disabled?

Don't know for the above, but for this question:

> Should I change the test to pass if allocation fails?

I believe returning EXIT_UNSUPPORTED would be reasonable.


Note that the testcase requires not only 2GB of address space, but also
causes faults and allocation for the whole range while doing the memset;
that sounds like a fairly heavy requirement.

Personally I'd rather avoid that by mmap'ing the buffer with MAP_NORESERVE,
initializing its head/tail as appropriate, and duplicating the "aaaa..." in
the middle by mmapping over pages in the interior with MAP_FIXED.

Alexander

Re: [patch] Fix path length overflow in realpath (BZ#22786)

[Paul Pluzhnikov]

On Tue, Apr 10, 2018 at 8:31 AM Alexander Monakov <amonakov@ispras.ru>
wrote:

> I believe returning EXIT_UNSUPPORTED would be reasonable.

Already done in earlier message.

> Note that the testcase requires not only 2GB of address space, but also
> causes faults and allocation for the whole range while doing the memset;
> that sounds like a fairly heavy requirement.

Do people test GLIBC on machines where 2GB is heavy?

> Personally I'd rather avoid that by mmap'ing the buffer with
MAP_NORESERVE,
> initializing its head/tail as appropriate, and duplicating the "aaaa..."
in
> the middle by mmapping over pages in the interior with MAP_FIXED.

It already took me significantly longer to write the test than to write the
fix :-(

I am not sure complicating the test that much further is worth the effort,
but if people really do test on machines where 2GiB allocation succeeds,
but memset()ting 2GiB kills it, I will do it.

Thanks,
-- 
Paul Pluzhnikov

Re: [patch] Fix path length overflow in realpath (BZ#22786)

[Alexander Monakov]

On Tue, 10 Apr 2018, Paul Pluzhnikov wrote:
> It already took me significantly longer to write the test than to write the
> fix :-(
> 
> I am not sure complicating the test that much further is worth the effort,
> but if people really do test on machines where 2GiB allocation succeeds,
> but memset()ting 2GiB kills it, I will do it.

Sorry. To be clear, I didn't mean to imply an objection to the patch, just
providing an observation and a commentary how the "cost" could be reduced.
I'm not demanding the patch be updated to incorporate that, and I hope your
patch can go in soon.

And FWIW I did read the fix and I understand it to be correct: preceding code
should guarantee 0 <= n < path_max, from that we know that path_max - n does
not overflow and is positive (and will not change when promoted to size_t).

Hope that helps.
Alexander

Re: [patch] Fix path length overflow in realpath (BZ#22786)

[Andreas Schwab]

On Apr 10 2018, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

> It already took me significantly longer to write the test than to write the
> fix :-(

It's not unusual that writing meaningful tests is the hardest part.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [patch] Fix path length overflow in realpath (BZ#22786)

[Carlos O'Donell]

On 04/10/2018 11:47 AM, Andreas Schwab wrote:
> On Apr 10 2018, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> 
>> It already took me significantly longer to write the test than to write the
>> fix :-(
> 
> It's not unusual that writing meaningful tests is the hardest part.

100% Agreed.

If writing the test takes 100x longer than the fix it is *still* worth it.

Systems level programming is *all* about writing tests to exercise the system.

-- 
Cheers,
Carlos.

Re: [patch] Fix path length overflow in realpath (BZ#22786)

[Joseph Myers]

On Mon, 9 Apr 2018, Paul Pluzhnikov wrote:

> +# suppress warnings about allocation size.
> +CFLAGS-test-bz22786.c += $(+gcc-nowarn)

I see no current uses of $(+gcc-nowarn) in the source tree.  Rather than 
resurrecting this variable, we should remove it.  Warnings should be 
disabled as locally as possible in the sources - meaning using DIAG_* with 
appropriate detailed comments around the specific statements generating 
warnings, if possible, and failing that, with specific -Wno-* options in 
the makefile rather than the extremely blunt -w which would also hide all 
valid warnings, not just the one warning you intend to avoid on the one 
bit of code for which you intend to avoid it.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [patch] Fix path length overflow in realpath (BZ#22786)

[Paul Pluzhnikov]

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

On Tue, Apr 17, 2018 at 2:01 PM Joseph Myers <joseph@codesourcery.com>
wrote:

> On Mon, 9 Apr 2018, Paul Pluzhnikov wrote:

> > +# suppress warnings about allocation size.
> > +CFLAGS-test-bz22786.c += $(+gcc-nowarn)

> Warnings should be disabled as locally as possible in the sources

Revised patch attached. Thanks,

2018-04-17  Paul Pluzhnikov  <ppluzhnikov@google.com>

          [BZ #22786]
          * stdlib/canonicalize.c (__realpath): Fix overflow in path length
          computation.
          * stdlib/Makefile (test-bz22786): New test.
          * stdlib/test-bz22786.c: New test.

-- 
Paul Pluzhnikov

[-- Attachment #2: glibc-bz22786-20180417.txt --]
[-- Type: text/plain, Size: 3793 bytes --]

diff --git a/stdlib/Makefile b/stdlib/Makefile
index af1643c0c4..1ddb1f9d18 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -84,7 +84,7 @@ tests		:= tst-strtol tst-strtod testmb testrand testsort testdiv   \
 		   tst-cxa_atexit tst-on_exit test-atexit-race 		    \
 		   test-at_quick_exit-race test-cxa_atexit-race             \
 		   test-on_exit-race test-dlclose-exit-race 		    \
-		   tst-makecontext-align
+		   tst-makecontext-align test-bz22786
 
 tests-internal	:= tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
 		   tst-tls-atexit tst-tls-atexit-nodelete
diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c
index 4135f3f33c..390fb437a8 100644
--- a/stdlib/canonicalize.c
+++ b/stdlib/canonicalize.c
@@ -181,7 +181,7 @@ __realpath (const char *name, char *resolved)
 		extra_buf = __alloca (path_max);
 
 	      len = strlen (end);
-	      if ((long int) (n + len) >= path_max)
+	      if (path_max - n <= len)
 		{
 		  __set_errno (ENAMETOOLONG);
 		  goto error;
diff --git a/stdlib/test-bz22786.c b/stdlib/test-bz22786.c
new file mode 100644
index 0000000000..1b6331ac5c
--- /dev/null
+++ b/stdlib/test-bz22786.c
@@ -0,0 +1,90 @@
+/* Bug 22786: test for stack overflow in realpath.
+   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
+   <http://www.gnu.org/licenses/>.  */
+
+/* This file must be run from within a directory called "stdlib".  */
+
+#include <errno.h>
+#include <limits.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <support/test-driver.h>
+#include <libc-diag.h>
+
+static int
+do_test (void)
+{
+  const char dir[] = "bz22786";
+  const char lnk[] = "bz22786/symlink";
+
+  rmdir (dir);
+  if (mkdir (dir, 0755) != 0 && errno != EEXIST)
+    {
+      printf ("mkdir %s: %m\n", dir);
+      return EXIT_FAILURE;
+    }
+  if (symlink (".", lnk) != 0 && errno != EEXIST)
+    {
+      printf ("symlink (%s, %s): %m\n", dir, lnk);
+      return EXIT_FAILURE;
+    }
+
+  const size_t path_len = (size_t) INT_MAX + 1;
+
+  DIAG_PUSH_NEEDS_COMMENT;
+#if __GNUC_PREREQ (7, 0)
+  /* GCC 7 warns about too-large allocations; here we need such
+     allocation to succeed for the test to work.  */
+  DIAG_IGNORE_NEEDS_COMMENT (7, "-Walloc-size-larger-than=");
+#endif
+  char *path = malloc (path_len);
+  DIAG_POP_NEEDS_COMMENT;
+
+  if (path == NULL)
+    {
+      printf ("malloc (%zu): %m\n", path_len);
+      return EXIT_UNSUPPORTED;
+    }
+
+  /* Construct very long path = "bz22786/symlink/aaaa....."  */
+  char *p = mempcpy (path, lnk, sizeof (lnk) - 1);
+  *(p++) = '/';
+  memset (p, 'a', path_len - (path - p) - 2);
+  p[path_len - (path - p) - 1] = '\0';
+
+  /* This call crashes before the fix for bz22786 on 32-bit platforms.  */
+  p = realpath (path, NULL);
+
+  if (p != NULL || errno != ENAMETOOLONG)
+    {
+      printf ("realpath: %s (%m)", p);
+      return EXIT_FAILURE;
+    }
+
+  /* Cleanup.  */
+  unlink (lnk);
+  rmdir (dir);
+
+  return 0;
+}
+
+#define TEST_FUNCTION do_test
+#include <support/test-driver.c>

Re: [patch] Fix path length overflow in realpath (BZ#22786)

[Paul Pluzhnikov]

Ping?

On Tue, Apr 17, 2018 at 4:50 PM Paul Pluzhnikov <ppluzhnikov@google.com>
wrote:

> Revised patch attached. Thanks,

> 2018-04-17  Paul Pluzhnikov  <ppluzhnikov@google.com>



-- 
Paul Pluzhnikov

Re: [patch] Fix path length overflow in realpath (BZ#22786)

[Paul Pluzhnikov]

On Mon, Apr 30, 2018 at 2:41 PM Paul Pluzhnikov <ppluzhnikov@google.com>
wrote:

> Ping?

Ping x2?

> On Tue, Apr 17, 2018 at 4:50 PM Paul Pluzhnikov <ppluzhnikov@google.com>
> wrote:

> > Revised patch attached. Thanks,

> > 2018-04-17  Paul Pluzhnikov  <ppluzhnikov@google.com>


-- 
Paul Pluzhnikov

Re: [patch] Fix path length overflow in realpath (BZ#22786)

[Andreas Schwab]

On Apr 17 2018, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

> diff --git a/stdlib/test-bz22786.c b/stdlib/test-bz22786.c
> new file mode 100644
> index 0000000000..1b6331ac5c
> --- /dev/null
> +++ b/stdlib/test-bz22786.c
> @@ -0,0 +1,90 @@
> +/* Bug 22786: test for stack overflow in realpath.

This is actually a buffer overflow.  Ok with that change.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [patch] Fix path length overflow in realpath (BZ#22786)

[Paul Pluzhnikov]

On Tue, May 8, 2018 at 7:59 AM Andreas Schwab <schwab@suse.de> wrote:

> On Apr 17 2018, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

> > diff --git a/stdlib/test-bz22786.c b/stdlib/test-bz22786.c
> > new file mode 100644
> > index 0000000000..1b6331ac5c
> > --- /dev/null
> > +++ b/stdlib/test-bz22786.c
> > @@ -0,0 +1,90 @@
> > +/* Bug 22786: test for stack overflow in realpath.

> This is actually a buffer overflow.  Ok with that change.

I am not sure what you mean by that.

The (stack) allocated buffer is large enough, so technically there is no
buffer overflow here (at least not in the sense that "allocated buffer was
too small"). But the stack is not large enough to hold the buffer of that
size.

-- 
Paul Pluzhnikov

Re: [patch] Fix path length overflow in realpath (BZ#22786)

[Andreas Schwab]

On Mai 08 2018, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

> On Tue, May 8, 2018 at 7:59 AM Andreas Schwab <schwab@suse.de> wrote:
>
>> On Apr 17 2018, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>
>> > diff --git a/stdlib/test-bz22786.c b/stdlib/test-bz22786.c
>> > new file mode 100644
>> > index 0000000000..1b6331ac5c
>> > --- /dev/null
>> > +++ b/stdlib/test-bz22786.c
>> > @@ -0,0 +1,90 @@
>> > +/* Bug 22786: test for stack overflow in realpath.
>
>> This is actually a buffer overflow.  Ok with that change.
>
> I am not sure what you mean by that.
>
> The (stack) allocated buffer is large enough

Is it?  The condition is about the limit of the buffer being written,
and about overflow missing the limit.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [patch] Fix path length overflow in realpath (BZ#22786)

[Paul Pluzhnikov]

On Tue, May 8, 2018 at 8:29 AM Andreas Schwab <schwab@suse.de> wrote:

> > The (stack) allocated buffer is large enough

> Is it?  The condition is about the limit of the buffer being written,
> and about overflow missing the limit.

Oops, I mixed up this patch with the other overflow (BZ 20419).
You are absolutely right. Will retest and commit shortly.

Thanks,
-- 
Paul Pluzhnikov

Re: [patch] Fix path length overflow in realpath (BZ#22786)

[Dmitry V. Levin]

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

On Mon, Apr 09, 2018 at 11:01:50PM +0000, Paul Pluzhnikov wrote:
> Greetings,
> 
> Attached is a trivial fix, and a test case.

Paul, could you format commit messages in a git friendly way
(the first line is a terse summary, the second line is empty,
the third line starts a descriptive text) like others do, please?


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [patch] Fix path length overflow in realpath (BZ#22786)

[Paul Pluzhnikov]

On Wed, May 9, 2018 at 4:11 AM Dmitry V. Levin <ldv@altlinux.org> wrote:

> Paul, could you format commit messages in a git friendly way
> (the first line is a terse summary, the second line is empty,
> the third line starts a descriptive text) like others do, please?

Will do. Sorry about that.


-- 
Paul Pluzhnikov