It's really difficult to occur in practice because the sum of the lower
and higher limits must overflow an int variable, but it can occur when
working with large arrays. We'd better safe than sorry by avoiding this
overflow situation when computing the middle element for comparison.
See:
http://googleresearch.blogspot.com/2006/06/extra-extra-read-all-about-it-nearly.html
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=5045582
The program below demonstrates the issue:
$ ./a.out
(glibc) Element is at the last position
(patched) Element is at the last position
Segmentation fault
===
#include <search.h>
#include <stdlib.h>
#include <stdio.h>
/* A number that when doubled will be bigger than 2^31 - 1 */
#define BIG_NUMBER_TO_OVERFLOW_INT (1100000000)
static int cmp_char(const void *p1, const void *p2)
{
char v1 = (*(char *)p1);
char v2 = (*(char *)p2);
if (v1 < v2)
return -1;
else if (v1 > v2)
return 1;
else
return 0;
}
void *lib_bsearch(const void *key, const void *base, size_t num, size_t size
, int (*cmp)(const void *key, const void *elt))
{
int start = 0, end = num - 1, mid, result;
while (start <= end) {
mid = (start + end) / 2;
result = cmp(key, base + mid * size);
if (result < 0)
end = mid - 1;
else if (result > 0)
start = mid + 1;
else
return (void *)base + mid * size;
}
return NULL;
}
void *patch_lib_bsearch(const void *key, const void *base, size_t num, size_t size
, int (*cmp)(const void *key, const void *elt))
{
size_t start = 0, end = num;
int result;
while (start < end) {
size_t mid = start + (end - start) / 2;
result = cmp(key, base + mid * size);
if (result < 0)
end = mid;
else if (result > 0)
start = mid + 1;
else
return (void *)base + mid * size;
}
return NULL;
}
int main(void)
{
char key = 1;
char *array = calloc(BIG_NUMBER_TO_OVERFLOW_INT, sizeof(char));
void *ptr;
if (!array)
{
printf("%s\n", "no memory");
return EXIT_FAILURE;
}
array[BIG_NUMBER_TO_OVERFLOW_INT - 1] = 1;
ptr = bsearch(&key, array, BIG_NUMBER_TO_OVERFLOW_INT, sizeof(char), cmp_char);
printf("(glibc) Element is%sat the last position\n"
, (ptr == &array[BIG_NUMBER_TO_OVERFLOW_INT - 1]) ? " " : " NOT ");
ptr = patch_lib_bsearch(&key, array, BIG_NUMBER_TO_OVERFLOW_INT, sizeof(char), cmp_char);
printf("(patched) Element is%sat the last position\n"
, (ptr == &array[BIG_NUMBER_TO_OVERFLOW_INT - 1]) ? " " : " NOT ");
ptr = lib_bsearch(&key, array, BIG_NUMBER_TO_OVERFLOW_INT, sizeof(char), cmp_char);
printf("(unpatched) Element is%sat the last position\n"
, (ptr == &array[BIG_NUMBER_TO_OVERFLOW_INT - 1]) ? " " : " NOT ");
free(array);
return EXIT_SUCCESS;
}
Signed-off-by: André Goddard Rosa <[email protected]>
---
lib/bsearch.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/lib/bsearch.c b/lib/bsearch.c
index 33cbba6..29233ed 100644
--- a/lib/bsearch.c
+++ b/lib/bsearch.c
@@ -33,10 +33,12 @@
void *bsearch(const void *key, const void *base, size_t num, size_t size,
int (*cmp)(const void *key, const void *elt))
{
- int start = 0, end = num, mid, result;
+ size_t start = 0, end = num;
+ int result;
while (start < end) {
- mid = (start + end) / 2;
+ size_t mid = start + (end - start) / 2;
+
result = cmp(key, base + mid * size);
if (result < 0)
end = mid;
--
1.6.5.2.153.g6e31f.dirty
On Tue, Nov 10, 2009 at 1:00 PM, André Goddard Rosa
<[email protected]> wrote:
> void *bsearch(const void *key, const void *base, size_t num, size_t size,
> int (*cmp)(const void *key, const void *elt))
> {
> - int start = 0, end = num, mid, result;
> + size_t start = 0, end = num;
> + int result;
>
> while (start < end) {
> - mid = (start + end) / 2;
> + size_t mid = start + (end - start) / 2;
I think it is more readable if you could keep the mid variable outside
of the while loop.
On Wed, Nov 11, 2009 at 1:09 PM, Thiago Farina <[email protected]> wrote:
> On Tue, Nov 10, 2009 at 1:00 PM, André Goddard Rosa
> <[email protected]> wrote:
>> void *bsearch(const void *key, const void *base, size_t num, size_t size,
>> int (*cmp)(const void *key, const void *elt))
>> {
>> - int start = 0, end = num, mid, result;
>> + size_t start = 0, end = num;
>> + int result;
>>
>> while (start < end) {
>> - mid = (start + end) / 2;
>> + size_t mid = start + (end - start) / 2;
> I think it is more readable if you could keep the mid variable outside
> of the while loop.
>
I mean: size_t start = 0, end = num, mid;
On Wed, 11 Nov 2009 01:30:25 am André Goddard Rosa wrote:
> It's really difficult to occur in practice because the sum of the lower
> and higher limits must overflow an int variable, but it can occur when
> working with large arrays. We'd better safe than sorry by avoiding this
> overflow situation when computing the middle element for comparison.
I applied all these, after testing. In future would have been nice for you
to have posted a test patch so I didn't have make my own...
Thanks,
Rusty.
diff --git a/lib/bsearch.c b/lib/bsearch.c
--- a/lib/bsearch.c
+++ b/lib/bsearch.c
@@ -51,3 +51,50 @@ void *bsearch(const void *key, const voi
return NULL;
}
EXPORT_SYMBOL(bsearch);
+
+#if 1
+static int test_cmp(const void *_key, const void *_elt)
+{
+ int key = *(int *)_key, elt = *(int *)_elt;
+
+ if (key < elt)
+ return -1;
+ else if (key > elt)
+ return 1;
+ return 0;
+}
+
+static int test_bsearch(void)
+{
+ const int arr[] = { INT_MIN, 0, 1, 2, 3, 4, 5, 6, INT_MAX };
+ unsigned int start, num, i, total = 0;
+ int key;
+
+ for (start = 0; start < ARRAY_SIZE(arr); start++) {
+ for (num = 0; num < ARRAY_SIZE(arr) - start; num++) {
+ key = 7;
+ BUG_ON(bsearch(&key, &arr[start], num, sizeof(arr[0]),
+ test_cmp));
+ total++;
+ for (i = start; i < start+num; i++) {
+ int *ret;
+ key = arr[i];
+ ret = bsearch(&key, &arr[start], num,
+ sizeof(arr[0]), test_cmp);
+ if (!ret) {
+ printk("Could not find %i in %u-%u"
+ "(between %i and %i)\n",
+ key, start, start+num,
+ arr[start], arr[start+num]);
+ }
+ BUG_ON(!ret);
+ BUG_ON(*ret != key);
+ total++;
+ }
+ }
+ }
+ printk("Tested %u bsearches\n", total);
+ return 0;
+}
+module_init(test_bsearch);
+#endif
On Thu, Nov 12, 2009 at 11:06 AM, Rusty Russell <[email protected]> wrote:
> On Wed, 11 Nov 2009 01:30:25 am Andr? Goddard Rosa wrote:
>> It's really difficult to occur in practice because the sum of the lower
>> and higher limits must overflow an int variable, but it can occur when
>> working with large arrays. We'd better safe than sorry by avoiding this
>> overflow situation when computing the middle element for comparison.
>
> I applied all these, after testing. ?In future would have been nice for you
> to have posted a test patch so I didn't have make my own...
>
Sure, no problem, thanks!
Kind regards,
Andr?
Hi Rusty,
On Thu, Nov 12, 2009 at 11:06 AM, Rusty Russell <[email protected]> wrote:
> On Wed, 11 Nov 2009 01:30:25 am André Goddard Rosa wrote:
>> It's really difficult to occur in practice because the sum of the lower
>> and higher limits must overflow an int variable, but it can occur when
>> working with large arrays. We'd better safe than sorry by avoiding this
>> overflow situation when computing the middle element for comparison.
>
> I applied all these, after testing. In future would have been nice for you
> to have posted a test patch so I didn't have make my own...
Where did you apply this patch?
On Sat, 14 Nov 2009 01:33:04 am Thiago Farina wrote:
> Hi Rusty,
>
> On Thu, Nov 12, 2009 at 11:06 AM, Rusty Russell <[email protected]> wrote:
> > On Wed, 11 Nov 2009 01:30:25 am André Goddard Rosa wrote:
> >> It's really difficult to occur in practice because the sum of the lower
> >> and higher limits must overflow an int variable, but it can occur when
> >> working with large arrays. We'd better safe than sorry by avoiding this
> >> overflow situation when computing the middle element for comparison.
> >
> > I applied all these, after testing. In future would have been nice for you
> > to have posted a test patch so I didn't have make my own...
>
> Where did you apply this patch?
To my kernel series, which means it is now in linux-next.
Hope that helps,
Rusty.