grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 11/14] dns: reset data->naddresses for every packet we receiv


From: Josef Bacik
Subject: Re: [PATCH 11/14] dns: reset data->naddresses for every packet we receive
Date: Tue, 16 Feb 2016 11:18:51 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0

On 02/13/2016 11:05 AM, Andrei Borzenkov wrote:
11.02.2016 00:21, Josef Bacik пишет:
I noticed when debugging a problem that we'd corrupt memory if our dns server
didn't respond fast enough and we ended up asking for both an AAAA and A record
for a server.  The problem is we alloc data->addresses based on the number of
addresses in the packet, but we populate it based on data->naddresses.  So we
get the AAAA record with one address, and we add that, then we get the A record
with one address and now data->naddresses == 1 but the ancount is 1, so we
allocate data->addresses to hold one address but write the new address outside
the array.  We also leak the old addresses memory.  So fix this by noticing if
we already have an address and free the old memory and reset naddresses so we
don't overflow our new array.

Signed-off-by: Josef Bacik <address@hidden>
---
  grub-core/net/dns.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/grub-core/net/dns.c b/grub-core/net/dns.c
index 86e609b..7a6c4b4 100644
--- a/grub-core/net/dns.c
+++ b/grub-core/net/dns.c
@@ -276,6 +276,9 @@ recv_hook (grub_net_udp_socket_t sock __attribute__ 
((unused)),
        ptr++;
        ptr += 4;
      }
+  if (*data->naddresses)
+    grub_free (*data->addresses);
+  *data->naddresses = 0;
    *data->addresses = grub_malloc (sizeof ((*data->addresses)[0])
                                 * grub_be_to_cpu16 (head->ancount));

Hmm ... cannot we resize it?

*data->addresses = grub_realloc (*data->addresses,
sizeof ((*data->addresses)[0]) * (*data->naddresses += grub_be_to_cpu16
(head->ancount)))

as adjusted to not leak old pointer.

This way answers we got before would not be lost.


So I did it this way because we copy the whole array into the dns cache at the bottom and I felt like keeping track of where we currently were in the array was overly complicated when in the end we're ending up with two entries in the cache anyway. But if I do the multiple request thing then this patch will be rendered useless anyway so we can just ignore it for now. Thanks,

Josef




reply via email to

[Prev in Thread] Current Thread [Next in Thread]