Re: [PATCH v2] Don't fall back to mmap if the original arena is not corrupt

[Mike Frysinger <vapier at gentoo dot org>]

On 19 Aug 2015 22:55, Siddhesh Poyarekar wrote:
> --- a/malloc/arena.c
> +++ b/malloc/arena.c
> @@ -823,16 +823,20 @@ reused_arena (mstate avoid_arena)
>  
>    /* Make sure that the arena we get is not corrupted.  */
>    mstate begin = result;
> +  bool looped = false;
> +
>    while (arena_is_corrupt (result) || result == avoid_arena)
>      {
>        result = result->next;
>        if (result == begin)
> -	break;
> +	{
> +	  looped = true;
> +	  break;
> +	}
>      }
>  
> -  /* We could not find any arena that was either not corrupted or not the one
> -     we wanted to avoid.  */
> -  if (result == begin || result == avoid_arena)
> +  /* We could not find any arena that was not corrupted.  */
> +  if (looped)
>      return NULL;

i'm not sure this fixes the stated bug.  say you have two arenas:
	a -> b -> a
b is corrupt and a is avoided.
	avoid_arena = a;
	result = a;
	begin = a;

so the first loop runs because result == avoid_arena, so we move result=b,
and then the second loop runs because arena_is_corrupt(b), so we move
result=a, and then we hit the if statement which sets looped=true, and then
we break out.  then we return NULL even though there is a non-corrupt region.

looks like this applies whenever the first region is avoided and the others
are corrupt.

maybe instead of setting a looped variable, perhaps you want:
	while (arena_is_corrupt (result) || result == avoid_arena) {
		result = result->next;
		if (result == begin)
			break;
	}
	if (__glibc_unlikely (arena_is_corrupt (result))) {
		if (arena_is_corrupt (avoid_arena))
			return NULL;
		result = avoid_arena;
	}
-mike

Attachment: signature.asc
Description: Digital signature