From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1aViLE-000125-0H for mharc-grub-devel@gnu.org; Tue, 16 Feb 2016 11:19:08 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38158) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aViLA-0000wL-SL for grub-devel@gnu.org; Tue, 16 Feb 2016 11:19:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aViL5-00033y-Tq for grub-devel@gnu.org; Tue, 16 Feb 2016 11:19:04 -0500 Received: from mx0b-00082601.pphosted.com ([67.231.153.30]:23238) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aViL5-00033k-OS for grub-devel@gnu.org; Tue, 16 Feb 2016 11:18:59 -0500 Received: from pps.filterd (m0001303.ppops.net [127.0.0.1]) by m0001303.ppops.net (8.15.0.59/8.15.0.59) with SMTP id u1GGGpbk023709; Tue, 16 Feb 2016 08:18:57 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=fb.com; h=subject : to : references : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=facebook; bh=vvaWAqhPmTGNAhbFJmSX6mRQJttum1JBYmlanCH1DZk=; b=rPtRtFtgED81j3nsfgvfmYGoxKx1wwjqI/yZN7iHpoWv3fj93Db5srU9ybD10k+ScJmb /5hiB1kXj5GVDgX2jrAXfKliR2jsymaR/IPLRWVbg8g8mUQGPeRjUX1gttdZA4VS+ogG Apo8RuZpkdIkjCCyD8nLsnAdhhLf56NuqNo= Received: from mail.thefacebook.com ([199.201.64.23]) by m0001303.ppops.net with ESMTP id 21211ebekx-2 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NOT); Tue, 16 Feb 2016 08:18:57 -0800 Received: from localhost.localdomain (192.168.54.13) by mail.thefacebook.com (192.168.16.11) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 16 Feb 2016 08:18:53 -0800 Subject: Re: [PATCH 11/14] dns: reset data->naddresses for every packet we receive To: Andrei Borzenkov , The development of GNU GRUB , References: <1455139268-3241273-1-git-send-email-jbacik@fb.com> <1455139268-3241273-12-git-send-email-jbacik@fb.com> <56BF5430.5000508@gmail.com> From: Josef Bacik Message-ID: <56C34BEB.3050409@fb.com> 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 MIME-Version: 1.0 In-Reply-To: <56BF5430.5000508@gmail.com> Content-Type: text/plain; charset="utf-8"; format=flowed X-Originating-IP: [192.168.54.13] X-Proofpoint-Spam-Reason: safe X-FB-Internal: Safe X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2016-02-16_09:, , signatures=0 Content-Transfer-Encoding: quoted-printable X-MIME-Autoconverted: from 8bit to quoted-printable by m0001303.ppops.net id u1GGGpbk023709 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [generic] X-Received-From: 67.231.153.30 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 16 Feb 2016 16:19:06 -0000 On 02/13/2016 11:05 AM, Andrei Borzenkov wrote: > 11.02.2016 00:21, Josef Bacik =D0=BF=D0=B8=D1=88=D0=B5=D1=82: >> 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 nu= mber 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 =3D=3D 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 not= icing if >> we already have an address and free the old memory and reset naddresse= s so we >> don't overflow our new array. >> >> Signed-off-by: Josef Bacik >> --- >> 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 +=3D 4; >> } >> + if (*data->naddresses) >> + grub_free (*data->addresses); >> + *data->naddresses =3D 0; >> *data->addresses =3D grub_malloc (sizeof ((*data->addresses)[0]) >> * grub_be_to_cpu16 (head->ancount)); > > Hmm ... cannot we resize it? > > *data->addresses =3D grub_realloc (*data->addresses, > sizeof ((*data->addresses)[0]) * (*data->naddresses +=3D grub_be_to_cpu= 16 > (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=20 at the bottom and I felt like keeping track of where we currently were=20 in the array was overly complicated when in the end we're ending up with=20 two entries in the cache anyway. But if I do the multiple request thing=20 then this patch will be rendered useless anyway so we can just ignore it=20 for now. Thanks, Josef