Re: [patch] Correct range checking in mallopt/mxfast/tcache [BZ #25194]

[Jonathan Nieder <jrnieder at gmail dot com>]

DJ Delorie wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> So the only intended callers are set_tcache_max and set_mxfast (today)
>> and __libc_mallopt (in the future)?  Makes sense.
>
> Well, mallopt already calls do_set_mxfast, which is where the original
> bug came from.
>
>     case M_MXFAST:
>       res = do_set_mxfast (value);
>       break;

Ah.  You can ignore most of what I wrote except the bit about C99
section 6.3.1.3, then.

This patch feels like three different logical patches squashed
together:

 1. Removing the redundant comparison against zero, which is a no-op
    and thus a nice cleanup.

 2. Updating the return value from do_set_tcache_max and
    do_set_tcache_count, which no caller observes.  This makes it
    more consistent with do_set_mmap_threshold, so it seems like a good
    change (though it also makes me wonder if the tunable interface
    could change to allow these to return void).

 3. Propagating the return value from do_set_mxfast in mallopt.

All three are good changes, so LGTM.

It also makes me wonder: should we propagate the return value from the
other do_set_* calls in mallopt?  Even if they always succeed, this
seems less error-prone in case they're changed later to not always
succeed.

Thanks,
Jonathan