| Message ID | 56A8F5C0.2070307@redhat.com |
|---|---|
| State | New |
| Headers | show |
Thanks, that looks good to me.
On 01/27/2016 06:25 PM, Paul Eggert wrote:
> Thanks, that looks good to me.
Great.
Adhemerval, may I check this on the master branch?
Thanks,
Florian
On 28-01-2016 08:35, Florian Weimer wrote: > On 01/27/2016 06:25 PM, Paul Eggert wrote: >> Thanks, that looks good to me. > > Great. > > Adhemerval, may I check this on the master branch? Yes please, thanks.
On 27/01/16 16:52, Florian Weimer wrote: > 2016-01-27 Florian Weimer <fweimer@redhat.com> > > [BZ #18240] > * misc/bug18240.c: New test. this test now times out for me on aarch64 hw > + test_size (INT_MAX - 2); this test case allocates a big buffer (INT_MAX*24) with calloc and it actually tries to memset it to 0. when i tried a simple example manually that worked: calloc did not try to memset 48 GB to 0. i think the test-skeleton changes malloc behaviour in weird ways, but i'm not yet sure what's going on. (i did not notice earlier as in my normal test environment memory overcommit is off so the test case just fails with ENOMEM, i'm not sure how to handle such tests where huge memory is allocated)
Szabolcs Nagy <szabolcs.nagy@arm.com> writes: > i think the test-skeleton changes malloc behaviour > in weird ways, but i'm not yet sure what's going on. mallopt (M_PERTURB, 42); Andreas.
On 01/02/16 17:05, Andreas Schwab wrote: > Szabolcs Nagy <szabolcs.nagy@arm.com> writes: > >> i think the test-skeleton changes malloc behaviour >> in weird ways, but i'm not yet sure what's going on. > > mallopt (M_PERTURB, 42); > ok, so if the only line in main() is calloc(INT_MAX-2, 24); then it does the memset, but if i do calloc (500, 24); calloc (INT_MAX-2, 24); then it does not, however mallopt (M_PERTURB, 42); calloc (500, 24); calloc (INT_MAX-2, 24); again does the memset (and this is what happens in the actual test). given the unreliable calloc behaviour i think glibc should avoid such tests. does it affect other 64bit targets?
Szabolcs Nagy <szabolcs.nagy@arm.com> writes: > (i did not notice earlier as in my normal test > environment memory overcommit is off so the > test case just fails with ENOMEM, i'm not sure > how to handle such tests where huge memory is > allocated) Perhaps the test should set a VM limit so that the allocation fails even if you have huge amount of available memory. Performing the allocation isn't the main point of the test. Andreas.
On 02/01/2016 07:26 PM, Andreas Schwab wrote: > Szabolcs Nagy <szabolcs.nagy@arm.com> writes: > >> (i did not notice earlier as in my normal test >> environment memory overcommit is off so the >> test case just fails with ENOMEM, i'm not sure >> how to handle such tests where huge memory is >> allocated) > > Perhaps the test should set a VM limit so that the allocation fails even > if you have huge amount of available memory. Performing the allocation > isn't the main point of the test. Yes, this seems like a reasonable improvement. I didn't see this because I apparently do not have sufficient amounts of memory in my test machine. Florian
On 02/01/2016 06:05 PM, Andreas Schwab wrote: > Szabolcs Nagy <szabolcs.nagy@arm.com> writes: > >> i think the test-skeleton changes malloc behaviour >> in weird ways, but i'm not yet sure what's going on. > > mallopt (M_PERTURB, 42); Yeah, this invalidates some of the malloc tests as well. Florian
On Mon, 1 Feb 2016, Florian Weimer wrote: > On 02/01/2016 06:05 PM, Andreas Schwab wrote: > > Szabolcs Nagy <szabolcs.nagy@arm.com> writes: > > > >> i think the test-skeleton changes malloc behaviour > >> in weird ways, but i'm not yet sure what's going on. > > > > mallopt (M_PERTURB, 42); > > Yeah, this invalidates some of the malloc tests as well. I suppose we should look back at the bulk conversions to test-skeleton.c, to see if any tests were changed for which this is an issue?
2016-01-27 Paul Eggert <eggert@cs.ucla.edu> [BZ #18240] * misc/hsearch_r.c (isprime, __hcreate_r): Protect against unsigned int wraparound. 2016-01-27 Florian Weimer <fweimer@redhat.com> [BZ #18240] * misc/bug18240.c: New test. * misc/Makefile (tests): Add it. diff --git a/misc/Makefile b/misc/Makefile index b9f854e..d7bbc85 100644 --- a/misc/Makefile +++ b/misc/Makefile @@ -77,7 +77,7 @@ gpl2lgpl := error.c error.h tests := tst-dirname tst-tsearch tst-fdset tst-efgcvt tst-mntent tst-hsearch \ tst-error1 tst-pselect tst-insremque tst-mntent2 bug-hsearch1 \ - tst-mntent-blank-corrupt tst-mntent-blank-passno + tst-mntent-blank-corrupt tst-mntent-blank-passno bug18240 ifeq ($(run-built-tests),yes) tests-special += $(objpfx)tst-error1-mem.out endif diff --git a/misc/bug18240.c b/misc/bug18240.c new file mode 100644 index 0000000..4b26865 --- /dev/null +++ b/misc/bug18240.c @@ -0,0 +1,75 @@ +/* Test integer wraparound in hcreate. + Copyright (C) 2016 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/>. */ + +#include <errno.h> +#include <limits.h> +#include <search.h> +#include <stdbool.h> +#include <stdio.h> +#include <stdlib.h> + +static void +test_size (size_t size) +{ + int res = hcreate (size); + if (res == 0) + { + if (errno == ENOMEM) + return; + printf ("error: hcreate (%zu): %m\n", size); + exit (1); + } + char *keys[100]; + for (int i = 0; i < 100; ++i) + { + if (asprintf (keys + i, "%d", i) < 0) + { + printf ("error: asprintf: %m\n"); + exit (1); + } + ENTRY e = { keys[i], (char *) "value" }; + if (hsearch (e, ENTER) == NULL) + { + printf ("error: hsearch (\"%s\"): %m\n", keys[i]); + exit (1); + } + } + hdestroy (); + + for (int i = 0; i < 100; ++i) + free (keys[i]); +} + +static int +do_test (void) +{ + test_size (500); + test_size (-1); + test_size (-3); + test_size (INT_MAX - 2); + test_size (INT_MAX - 1); + test_size (INT_MAX); + test_size (((unsigned) INT_MAX) + 1); + test_size (UINT_MAX - 2); + test_size (UINT_MAX - 1); + test_size (UINT_MAX); + return 0; +} + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" diff --git a/misc/hsearch_r.c b/misc/hsearch_r.c index f6f16ed..1fca6b3 100644 --- a/misc/hsearch_r.c +++ b/misc/hsearch_r.c @@ -46,15 +46,12 @@ static int isprime (unsigned int number) { /* no even number will be passed */ - unsigned int div = 3; - - while (div * div < number && number % div != 0) - div += 2; - - return number % div != 0; + for (unsigned int div = 3; div <= number / div; div += 2) + if (number % div == 0) + return 0; + return 1; } - /* Before using the hash table we must allocate memory for it. Test for an existing table are done. We allocate one element more as the found prime number says. This is done for more effective @@ -71,13 +68,6 @@ __hcreate_r (size_t nel, struct hsearch_data *htab) return 0; } - if (nel >= SIZE_MAX / sizeof (_ENTRY)) - { - __set_errno (ENOMEM); - return 0; - } - - /* There is still another table active. Return with error. */ if (htab->table != NULL) return 0; @@ -86,10 +76,19 @@ __hcreate_r (size_t nel, struct hsearch_data *htab) use will not work. */ if (nel < 3) nel = 3; - /* Change nel to the first prime number not smaller as nel. */ - nel |= 1; /* make odd */ - while (!isprime (nel)) - nel += 2; + + /* Change nel to the first prime number in the range [nel, UINT_MAX - 2], + The '- 2' means 'nel += 2' cannot overflow. */ + for (nel |= 1; ; nel += 2) + { + if (UINT_MAX - 2 < nel) + { + __set_errno (ENOMEM); + return 0; + } + if (isprime (nel)) + break; + } htab->size = nel; htab->filled = 0;