Re: [PATCH][BZ 15089] Fixes malloc_trim always trims for large padding

[Will Newton <will dot newton at linaro dot org>]

On 20 November 2013 04:38, Fernando J V da Silva
<fernandojvdasilva@gmail.com> wrote:
> Hi all!
>
> I'm starting learning about glibc code and I've began trying to fix
> the bug 15089. I've put some comments on Bugzilla and I wrote a patch
> (attached) for fixing it, based on the solution proposed by Thiago
> Ize' comment in Bugzilla as well
> (https://sourceware.org/bugzilla/show_bug.cgi?id=15089).
>
> That solution really fixes the problem, but the systrim() code still
> mixes some signed and unsigned operations to calculate the size used
> to call sbrk(), which may be dangerous (and in fact caused this bug).
> I think the most correct solution would be to refactor this function
> for only using unsigned variables but, to be honest, I haven't tried
> it as I'm new on that (may be someone could give me some advices about
> it?).
>
> I would be glad if someone could have some time to comment it! :-)

Hi Fernando,

Thanks for the patch!

In general the ternary operator tends to be quite hard to read so in
my opinion is best avoided outside of macros. The overflow case should
be quite simple to handle with a return 0, e.g.:

if (top_area <= pad)
  return 0;

Then we can remove the whole "extra > 0" conditional below as well.

Also you need a GNU changelog entry to accompany your change (include
it in the commit message rather than as a patch to ChangeLog as it
makes it easier to merge) which mentions the chnages made and the BZ
number that is being closed (see ChangeLog for examples of this).

Finally, and perhaps the most difficult, I think this change may need
a copyright assignment on file to be accepted (see
https://sourceware.org/glibc/wiki/Contribution%20checklist).

-- 
Will Newton
Toolchain Working Group, Linaro