| Message ID | dc70486d-358a-c979-def3-f48778c32147@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jul 12, 2017 at 8:58 AM, Florian Weimer <fweimer@redhat.com> wrote: > On 07/12/2017 02:29 PM, Andreas Schwab wrote: >> On Jul 12 2017, Florian Weimer <fweimer@redhat.com> wrote: >> >>> The attached patch adds an assert which reveals to the GCC optimizers >>> that the global_max_fast can never MAX_FAST_SIZE, so this particular >>> issue goes away. However, there is a cost in terms of code size because >>> it affects many places in malloc. >> >> Can __builtin_unreachable avoid the runtime overhead? > > Interesting idea. I have never used __builtin_unreachable before, I think. I think this code should be defensive against the possibility of global_max_fast somehow getting corrupted. What about +static inline INTERNAL_SIZE_T +get_max_fast (void) +{ + /* If this function ever returns a value larger than MAX_FAST_SIZE, + _int_malloc will make out-of-bounds array accesses. It should be + impossible for global_max_fast to become larger than than + MAX_FAST_SIZE, but as an extra precaution, limit the value here + as well. */ + if (global_max_fast > MAX_FAST_SIZE) + return MAX_FAST_SIZE; + return global_max_fast; +}
On Wed, Jul 12, 2017 at 10:27 AM, Zack Weinberg <zackw@panix.com> wrote:
> + impossible for global_max_fast to become larger than than
Or without the double 'than',
+static inline INTERNAL_SIZE_T
+get_max_fast (void)
+{
+ /* If this function ever returns a value larger than MAX_FAST_SIZE,
+ _int_malloc will make out-of-bounds array accesses. It should be
+ impossible for global_max_fast to become larger than MAX_FAST_SIZE,
+ but as an extra precaution, limit the value here as well. */
+ if (global_max_fast > MAX_FAST_SIZE)
+ return MAX_FAST_SIZE;
+ return global_max_fast;
+}
On 08/03/2017 03:31 AM, Victor Rodriguez wrote: > On Wed, Jul 12, 2017 at 5:43 PM, DJ Delorie <dj@redhat.com> wrote: > >> I tested this patch and can measure no discernable performance >> difference, so LGTM :-) >> >> > Thanks for the patch , it works fine with 2.26 release > > Any reason why is not merged on master for 2.26 ? I didn't get an ack from the RM. Anyway, I pushed it now. I will also backport it to the 2.26 branch. Thanks, Florian
diff --git a/malloc/malloc.c b/malloc/malloc.c index 54e406b..da7876a 100644 --- a/malloc/malloc.c +++ b/malloc/malloc.c @@ -1658,6 +1658,9 @@ typedef struct malloc_chunk *mfastbinptr; #define arena_is_corrupt(A) (((A)->flags & ARENA_CORRUPTION_BIT)) #define set_arena_corrupt(A) ((A)->flags |= ARENA_CORRUPTION_BIT) +/* Maximum size of memory handled in fastbins. */ +static INTERNAL_SIZE_T global_max_fast; + /* Set value of max_fast. Use impossibly small value if 0. @@ -1668,8 +1671,20 @@ typedef struct malloc_chunk *mfastbinptr; #define set_max_fast(s) \ global_max_fast = (((s) == 0) \ ? SMALLBIN_WIDTH : ((s + SIZE_SZ) & ~MALLOC_ALIGN_MASK)) -#define get_max_fast() global_max_fast +static inline INTERNAL_SIZE_T +get_max_fast (void) +{ + /* Tell the glibc optimizers that global_max_fast is never larger + than MAX_FAST_SIZE. This avoids out-of-bounds array accesses in + _int_malloc after constant propagation of the size parameter. + (The code never executes because malloc preserves the + global_max_fast invariant, but the optimizers may not recognize + this.) */ + if (global_max_fast > MAX_FAST_SIZE) + __builtin_unreachable (); + return global_max_fast; +} /* ----------- Internal state representation and initialization ----------- @@ -1797,9 +1812,6 @@ static struct malloc_par mp_ = #endif }; -/* Maximum size of memory handled in fastbins. */ -static INTERNAL_SIZE_T global_max_fast; - /* Initialize a malloc_state struct.