From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from buffalo.birch.relay.mailchannels.net (buffalo.birch.relay.mailchannels.net [23.83.209.24]) by sourceware.org (Postfix) with ESMTPS id 162763858D1E for ; Tue, 8 Feb 2022 08:19:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 162763858D1E X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id 1FB1B28172C; Tue, 8 Feb 2022 08:19:08 +0000 (UTC) Received: from pdx1-sub0-mail-a305.dreamhost.com (unknown [127.0.0.6]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id AC239281633; Tue, 8 Feb 2022 08:19:07 +0000 (UTC) X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from pdx1-sub0-mail-a305.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384) by 100.126.0.25 (trex/6.4.3); Tue, 08 Feb 2022 08:19:08 +0000 X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|siddhesh@gotplt.org X-MailChannels-Auth-Id: dreamhost X-Tank-Average: 68233f964ffa8d88_1644308347936_3461659923 X-MC-Loop-Signature: 1644308347935:1689720579 X-MC-Ingress-Time: 1644308347935 Received: from [192.168.1.174] (unknown [1.186.224.172]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: siddhesh@gotplt.org) by pdx1-sub0-mail-a305.dreamhost.com (Postfix) with ESMTPSA id 4JtGBL1wvnz2Y; Tue, 8 Feb 2022 00:19:05 -0800 (PST) Message-ID: Date: Tue, 8 Feb 2022 13:49:00 +0530 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 Subject: Re: [PATCH] getaddrinfo: Fix leak with AI_ALL [BZ #28852] Content-Language: en-US To: Florian Weimer , Siddhesh Poyarekar via Libc-alpha References: <20220202163902.3596109-1-siddhesh@sourceware.org> <87y22qq1io.fsf@oldenburg.str.redhat.com> From: Siddhesh Poyarekar In-Reply-To: <87y22qq1io.fsf@oldenburg.str.redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3494.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_NONE, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NEUTRAL, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 08 Feb 2022 08:19:12 -0000 On 04/02/2022 17:36, Florian Weimer wrote: > * Siddhesh Poyarekar via Libc-alpha: > >> diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c >> index 18dccd5924..652a1a43d4 100644 >> --- a/sysdeps/posix/getaddrinfo.c >> +++ b/sysdeps/posix/getaddrinfo.c >> @@ -199,9 +199,6 @@ convert_hostent_to_gaih_addrtuple (const struct addrinfo *req, >> struct hostent *h, >> struct gaih_addrtuple **result) >> { >> - while (*result) >> - result = &(*result)->next; >> - >> /* Count the number of addresses in h->h_addr_list. */ >> size_t count = 0; >> for (char **p = h->h_addr_list; *p != NULL; ++p) >> @@ -212,10 +209,30 @@ convert_hostent_to_gaih_addrtuple (const struct addrinfo *req, >> if (count == 0 || h->h_length > sizeof (((struct gaih_addrtuple) {}).addr)) >> return true; >> >> - struct gaih_addrtuple *array = calloc (count, sizeof (*array)); >> + struct gaih_addrtuple *array = *result; >> + size_t old = 0; >> + >> + while (array) >> + { >> + old++; >> + array = array->next; >> + } >> + >> + array = realloc (*result, (old + count) * sizeof (*array)); >> + >> if (array == NULL) >> return false; >> >> + *result = array; >> + >> + /* Update the next pointers on reallocation. */ >> + for (size_t i = 0; i < old; i++) >> + array[i].next = array + i + 1; >> + >> + array += old; >> + >> + memset (array, 0, count * sizeof (*array)); >> + >> for (size_t i = 0; i < count; ++i) >> { >> if (family == AF_INET && req->ai_family == AF_INET6) >> @@ -235,7 +252,6 @@ convert_hostent_to_gaih_addrtuple (const struct addrinfo *req, >> array[0].name = h->h_name; >> array[count - 1].next = NULL; >> >> - *result = array; >> return true; >> } > > I think this assumes that the addrmem block (now managed by realloc) is > always at the end of the “at” tuples list (which is not always backed by > addrmem memory). If that is not the case, the tail after the addrmem > block is lost. I couldn't find a way in which a block not backed by addrmem would follow these realloc'd blocks. Every situation where a different method is used to allocate tuples (e.g. through gethostbyname4_r), the *pat is overwritten, causing older tuples to be lost. This could happen for example if we have SUCCESS=CONTINUE in nsswitch.conf (is that even allowed?) and a gethostbyname[23]_r is followed by gethostbyname4_r. I'm not sure if it is a situation we ought to support. Does that make sense? ISTM the whole thing could be simplified by dropping alloca and using malloc throughout; all this seems unnecessarily complicated. Let me take a stab at that. Thanks, Siddhesh