Improve check against integer wraparound in hcreate_r [BZ #18240]
Commit Message
On 01/26/2016 01:06 AM, Paul Eggert wrote:
> On 01/25/2016 12:09 PM, Florian Weimer wrote:
>> - while (div * div < number && number % div != 0)
>> + while (div * (unsigned long long) div < number && number % div != 0)
>
> Good catch. But better yet, get rid of the '*' so that we needn't worry
> about whether the multiplication overflows. On typical platforms the
> divide instruction that the '%' is already doing will give us the
> information we need, so this is faster anyway. Something like the
> attached (untested) patch.
I looked at the generated assembly on x86-64, and confirm that there's
just one division. Rest looks good to me, too.
I'm attaching a real patch which also includes the test I wrote (it
still passes, but it really doesn't do anything).
Florian
Comments
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.
@@ -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
new file mode 100644
@@ -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"
@@ -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;