From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x82c.google.com (mail-qt1-x82c.google.com [IPv6:2607:f8b0:4864:20::82c]) by sourceware.org (Postfix) with ESMTPS id E21D5389246F for ; Mon, 17 May 2021 17:15:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E21D5389246F Received: by mail-qt1-x82c.google.com with SMTP id j19so5368309qtp.7 for ; Mon, 17 May 2021 10:15:50 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=8yJAFDFueTbP2ZgKYAdq32erU+fhQtRlNCUTlwKCWpo=; b=W231PsOFTUUbIUiSYO8OVqiWxNrtMLUyfFUQZ2FcEwR5LA6TVFI70Vw+8QOYY3/hBY mnlOZKxhPX6mLdNkEsG01UQl/A8xOtEN7k/u3Cdh5ga6rME7MJRjaDD5ePdFkMGNgsjd L2x+Fzj3exnrXHdQ/g33jUu9XFQk4e+So43oCJdFf6NvbA/BE/vfCMY0v/WLnNHJVFF1 249l4H9OYOQyVYMX0xSgf+WwG1nExcCfNcCG/v9LuFDhK46ZO1O76L2vjr2iDex/u3fn dUlPKn09an2KfOiwKsA7ZgyP/REUOVmmN7BheFb/r4ImlLJd+vVssC2d8LaCdMaCoHML zjiQ== X-Gm-Message-State: AOAM531TnTkk/A6GRDKDkVaZPF59kb9WQLygT0KCOuTqs/ZGfxwqKYND 7cNeEXH82Y53lwl6mc/XcrReku/NfKrWpg== X-Google-Smtp-Source: ABdhPJzZQ4ovd/URi0JenfmqjAK+mykEvwW7Z2/3xy7xE0ZWzlesIIIlLce5TCHa9Cf03fR5PEvYYw== X-Received: by 2002:ac8:5ac7:: with SMTP id d7mr500854qtd.173.1621271750297; Mon, 17 May 2021 10:15:50 -0700 (PDT) Received: from [192.168.1.4] ([177.194.37.86]) by smtp.gmail.com with ESMTPSA id c14sm10955784qtc.5.2021.05.17.10.15.49 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 17 May 2021 10:15:50 -0700 (PDT) Subject: Re: [PATCH] ldconfig: Fix memory leaks To: Siddhesh Poyarekar , libc-alpha@sourceware.org References: <20210511171627.360100-1-siddhesh@sourceware.org> From: Adhemerval Zanella Message-ID: <1ba04d0c-6b76-94e4-4c60-ad2246aa1357@linaro.org> Date: Mon, 17 May 2021 14:15:47 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: <20210511171627.360100-1-siddhesh@sourceware.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Mon, 17 May 2021 17:15:52 -0000 On 11/05/2021 14:16, Siddhesh Poyarekar via Libc-alpha wrote: > Coverity discovered that paths allocated by chroot_canon are not freed > in a couple of routines in ldconfig. LGTM, just a clarification about a specific change below. As a side note, reviewing this patch I think chroot_canon can be replaced with realpath. Reviewed-by: Adhemerval Zanella > --- > elf/ldconfig.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/elf/ldconfig.c b/elf/ldconfig.c > index 28ed637a29..3192aa49d9 100644 > --- a/elf/ldconfig.c > +++ b/elf/ldconfig.c > @@ -693,8 +693,7 @@ manual_link (char *library) > if (real_path == NULL) > { > error (0, errno, _("Can't find %s"), path); > - free (path); > - return; > + goto out; > } > real_library = alloca (strlen (real_path) + strlen (libname) + 2); > sprintf (real_library, "%s/%s", real_path, libname); Why do you need this since 'real_path' does not need to be freed here ? > @@ -709,16 +708,14 @@ manual_link (char *library) > if (lstat64 (real_library, &stat_buf)) > { > error (0, errno, _("Cannot lstat %s"), library); > - free (path); > - return; > + goto out; > } > /* We don't want links here! */ Ok. > else if (!S_ISREG (stat_buf.st_mode)) > { > error (0, 0, _("Ignored file %s since it is not a regular file."), > library); > - free (path); > - return; > + goto out; > } > Ok. > if (process_file (real_library, library, libname, &flag, &osversion, > @@ -726,14 +723,16 @@ manual_link (char *library) > { > error (0, 0, _("No link created since soname could not be found for %s"), > library); > - free (path); > - return; > + goto out; > } Ok. > if (soname == NULL) > soname = implicit_soname (libname, flag); > create_links (real_path, path, libname, soname); > free (soname); > +out: > free (path); > + if (path != real_path) > + free (real_path); > } > > Ok. > @@ -920,8 +919,16 @@ search_dir (const struct dir_entry *entry) > /* Remove stale symlinks. */ > if (opt_link && strstr (direntry->d_name, ".so.")) > unlink (real_file_name); > + > + if (opt_chroot) > + free (target_name); > + Ok, 'opt_chroot' being not null means it was allocated using chroot_canon instead of alloca. No implicit checks though. > continue; > } > + > + if (opt_chroot) > + free (target_name); > + Ok. > is_dir = S_ISDIR (stat_buf.st_mode); > > /* lstat_buf is later stored, update contents. */ >