Re: [PATCH 3/4] Don't allocate a new address buffer if we receive multiple DNS responses

[Andrei Borzenkov]

24.01.2017 03:36, Matthew Garrett пишет:
> The current logic in the DNS resolution code allocates an address buffer
> based on the number of addresses in the response packet. If we receive
> multiple response packets in response to a single query packet, this means
> that we will reallocate a new buffer large enough for only the addresses in
> that specific packet, discarding any previous results in the process. Worse,
> we still keep track of the *total* number of addresses resolved in response
> to this query, not merely the number in the packet being currently processed.
> Use realloc() rather than malloc() to avoid overwriting the existing data,
> and allocate a buffer large enough for the total set of addresses rather
> than merely the number in this specific response.
> ---
>  grub-core/net/dns.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/grub-core/net/dns.c b/grub-core/net/dns.c
> index 5d9afe0..5deb1ef 100644
> --- a/grub-core/net/dns.c
> +++ b/grub-core/net/dns.c
> @@ -285,8 +285,8 @@ recv_hook (grub_net_udp_socket_t sock __attribute__ 
> ((unused)),
>        ptr++;
>        ptr += 4;
>      }
> -  *data->addresses = grub_malloc (sizeof ((*data->addresses)[0])
> -                              * grub_be_to_cpu16 (head->ancount));
> +  *data->addresses = grub_realloc (*data->addresses, sizeof 
> ((*data->addresses)[0])
> +                  * (grub_be_to_cpu16 (head->ancount) + *data->naddresses));

If *data->addresses was not NULL, we should not reach this code.

  /* Code apparently assumed that only one packet is received as response.
     We may get multiple responses due to network condition, so check here
     and quit early. */
  if (*data->addresses)
    {
      grub_netbuff_free (nb);
      return GRUB_ERR_NONE;
    }

This was noted previously by Josef, we discussed it and my position is
that resolver code requires redesign to correctly merge multiple answers
and prioritize A vs AAAA requests.

Do you get actual errors with current master? If yes, could you provide
more information what this patch fixes?

>    if (!*data->addresses)
>      {
>        grub_errno = GRUB_ERR_NONE;
>