| Message ID | CALoOobMWCHXYh-3rDeSfHauaOtpMmVMpv_obPA8Dk9ubTvfK=Q@mail.gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
On Apr 10 2018, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> Should I change the test to pass if allocation fails?
No, unsupported.
Andreas.
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
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,
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
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.
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.
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>