Re: [PATCH] Fix integer overflows in internal memalign and malloc functions [BZ #22343]

[Arjun Shankar <arjun.is@lostca.se>]

Carlos wrote:

> * Arjun, give Paul's patch a try, with my suggestions below and see
>   if it passes your regression test.
> * Repost v2, please include Paul's name in the ChangeLog for all the suggestions
>   e.g. your name and Paul's.

I tried this and it failed with the new test case in my original patch
because it caused alignment to be added twice: first with the new use of
checked_requestalign2size then immediately after in the call to _int_malloc
around line 4688 in _int_memalign.

#1:
-  checked_request2size (bytes, nb);
+  checked_requestalign2size (bytes, alignment, nb);

#2:
> m = (char *) (_int_malloc (av, nb + alignment + MINSIZE));

I thought this was a simple matter of removing the unnecessary addition...

> m = (char *) (_int_malloc (av, nb + MINSIZE));

...but that triggered several testsuite failures:

> FAIL: malloc/tst-malloc-thread-fail
> FAIL: malloc/tst-memalign
> FAIL: malloc/tst-posix_memalign
> FAIL: malloc/tst-pvalloc
> FAIL: malloc/tst-valloc

Before this change, this code was using checked_request2size which adds
MALLOC_ALIGNMENT and then aligns up which - while it makes no sense (to me)
in cases where the alignment is isn't MALLOC_ALIGNMENT - somehow seems to
work. It seems to me that cleaning this up might not be simple. In addition,
Paul has already said:

> Perhaps the best fix would be to remove INTERNAL_SIZE_T and replace
> it with size_t everywhere; there's no real reason for
> INTERNAL_SIZE_T. Alternatively, we could change the documentation for
> INTERNAL_SIZE_T to say that it must be an unsigned type; this would
> be a simpler patch.

Given this and given that, as Carlos says:

> The reason I didn't think this is that we have *tons* of checks that
> would break if we actually made this signed, so the fix you request
> is correct.

I vote that we consider reviewing and checking in my original attempt at a
fix prior to the 2.27 release, and in lieu of this we file a separate bug to
track the removal of technical debt that already exists in the code. I'll be
happy to take Paul's patch and run with it once master opens for development
again.

Cheers,
Arjun