From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (wildebeest.demon.nl [212.238.236.112]) by sourceware.org (Postfix) with ESMTPS id 035B6386480A for ; Fri, 23 Jul 2021 18:34:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 035B6386480A Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=klomp.org Received: from tarox.wildebeest.org (83-87-18-245.cable.dynamic.v4.ziggo.nl [83.87.18.245]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id D619F300066F; Fri, 23 Jul 2021 20:34:06 +0200 (CEST) Received: by tarox.wildebeest.org (Postfix, from userid 1000) id 2678641531A8; Fri, 23 Jul 2021 20:34:06 +0200 (CEST) Message-ID: <7b021d11929e451d77961ab183cd97f3329a6dce.camel@klomp.org> Subject: Re: [Bug debuginfod/27983] ignore duplicate urls From: Mark Wielaard To: Noah Sanci , elfutils-devel@sourceware.org Date: Fri, 23 Jul 2021 20:34:05 +0200 In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Evolution 3.28.5 (3.28.5-10.el7) Mime-Version: 1.0 X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP 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: elfutils-devel@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Elfutils-devel mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Jul 2021 18:34:10 -0000 Hi Noah, On Thu, 2021-07-22 at 15:22 -0400, Noah Sanci via Elfutils-devel wrote: > PR27983 improvements attached Please do mention the improvements you made. It really does help others see what you thought of. So in this case use asprintf instead of calloc plus sprintf, and (re)use tmpurl instead of strduping it and freeing. Both good improvements. I really like this variant of the patch. Nice work. There is one possible issue with the reallocarray call (realloc[array] is terrible that way, sorry). See below. Also, could you please rebase this patch on top of master, there have been various changes to debuginfod-client.c and it would be nice to see how the patch interacts with those. > + else > + { > + num_urls++; > + server_url_list =3D reallocarray(server_url_list, num_urls, > + sizeof(char*)); > + if (server_url_list =3D=3D NULL) > + { > + rc =3D -ENOMEM; > + goto out1; > + } > + server_url_list[num_urls-1] =3D tmp_url; > + } Note how reallocarray returns NULL on failure. This means you lost the original server_url_list pointer. And so when you then do cleanup by going to out1... > + out1: > + for (int i =3D 0; i < num_urls; ++i) > + free(server_url_list[i]); > + free(server_url_list); You will crash because server_url_list is NULL here, so server_url_list[i] is a bad dereference... Bleah. Like I said, the realloc(array) interface is really bad :{ To use reallocarray safely you need to use a temp and only set it after the call succeeds. char **realloc_ptr =3D reallocarray (...); if (realloc_ptr =3D=3D NULL) { rc =3D -ENOMEM; goto out1; } server_url_list =3D realloc_ptr; server_url_list[num_urls-1] =3D ...; The rest of the patch looks good. Thanks, Mark