#13820 closed bug (fixed)

Curious behaviour with calloc(-1, -1)

Reported by: ohnx56 Owned by: nobody
Priority: normal Milestone: Unscheduled
Component: System/libroot.so Version: R1/Development
Keywords: Cc: axeld
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

I noticed some curious behaviour with calloc(-1, -1) on hrev51615.

Since calloc uses unsigned ints, -1 = 4294967295.

4294967295 * 4294967295 overflows when multiplying to obtain 1.

Haiku does not check the overflow after multiplying here: https://github.com/haiku/haiku/blob/17b2a3cfcbc4fb1eb25d7eeb61e8fac997d7d835/src/system/libroot/posix/malloc/wrapper.cpp#L330

This results in Haiku assuming that the user wants 1 byte of memory allocated, which is technically incorrect...

A way to check for overflow would be to divide the result by one of the inputs (n or elem size) and ensure it equals the other input. From my understanding, glibc uses this: https://github.com/lattera/glibc/blob/master/malloc/malloc.c#L3170

This issue is notable because there are other ways for a 32-bit unsigned integer multiplication to overflow, possibly resulting in incorrect values being returned from Haiku.

Bottom line is this: When I call calloc() with impossibly large amounts of memory requirements, I expect a return value of NULL, and not a pointer :)

Attachments (2)

13820.patch (1.6 KB) - added by ohnx56 17 months ago.
13820-test.patch (1.9 KB) - added by ohnx56 16 months ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 17 months ago by ohnx56

Thanks to sphinxbase for having a weird test like this that demonstrates this issue...

https://github.com/cmusphinx/sphinxbase/blob/master/test/unit/test_alloc/test_ckd_alloc_catch.c

comment:2 Changed 17 months ago by ohnx56

Has a Patch: set

comment:3 Changed 17 months ago by waddlesplash

Cc: axeld added
Component: - GeneralSystem/libroot.so
Platform: x86-64All

Patch seems good to me. CC'ing axeld so he can take a look.

comment:4 Changed 17 months ago by ohnx56

Update patch to meet coding guidelines

Changed 17 months ago by ohnx56

Attachment: 13820.patch added

comment:5 Changed 17 months ago by ohnx56

Hmm, looks like sometimes calloc() gets called with 0-values... This patch now also checks to ensure it does not end up diving by zero.

I have tested this patch applied onto hrev51641 by booting to the patched Haiku and writing a program using calloc(-1, -1) as well as calloc(4294967295, 123). Now, calloc() behaves as expected by returning a NULL to silly requests for memory like these ones. No application appears to be broken by this change.

comment:6 Changed 17 months ago by pulkomandy

Adding a test in src/tests would be nice, too (testing one or the other parameter being 0, the -1 -1 case, and the "incredibly large" case, as well as a normal allocation).

comment:7 Changed 17 months ago by ohnx56

Where should the test go? I can write one and submit a patch for the test as well.

comment:8 Changed 17 months ago by pulkomandy

src/tests/system/libroot/posix sounds appropriate.

comment:9 Changed 16 months ago by waddlesplash

Resolution: fixed
Status: newclosed

Applied in hrev51689. Thanks!

Changed 16 months ago by ohnx56

Attachment: 13820-test.patch added
Note: See TracTickets for help on using tickets.