From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x831.google.com (mail-qt1-x831.google.com [IPv6:2607:f8b0:4864:20::831]) by sourceware.org (Postfix) with ESMTPS id EB9703854812 for ; Tue, 18 May 2021 13:08:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org EB9703854812 Received: by mail-qt1-x831.google.com with SMTP id a10so544321qtp.7 for ; Tue, 18 May 2021 06:08:40 -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=saiatwEO2MX4eX+as1eEs1R0wjxOsWCTkoyuyQF5Ogs=; b=ukbPS4PRQkFA/iVBEUreGvsK5R8KS3hVy2h8rzw9c/2yNl+IX7+EyaSr3Bno78Ayw6 ew0N46DKokBploQbqDvbsbau6fFbvGvA8isXyoe8ODXo00UMqAMAL93R+v4K6PiY9mFZ SA2I35xjxLVQvZfG741jCJFgkXSCZQDn8Q0QklcmNST9UdT6ePiyvoS6ppJ8g5UFKuhx BbpeVJazZo2IOH3gugRmMPX+iOb3SZCAOrtxS/SB8InX82U07yAk0sbMb8yL1ZUbAaCH sUsBcPq5bSh4kxLSQvsGqdTeG7rpDJiFWyP1almVlr/ViibWRXXj19nyL3HrIbqRkF3c 38FA== X-Gm-Message-State: AOAM5320Q4USJnHGEk5xzoNa5DhXpXhXS9d47oTQY6h3G3pb42J6Sqav EDJtP6up+yptvwGk7O58z3jPenwV2y7Cwg== X-Google-Smtp-Source: ABdhPJxQrh9gl+tBf7Q6IwDNuEl/XHCEFthhXUQBjseacF6nuUdUX5/tcMeRdH0uFcwLtMVSlTOPZQ== X-Received: by 2002:ac8:6909:: with SMTP id e9mr4644941qtr.338.1621343320193; Tue, 18 May 2021 06:08:40 -0700 (PDT) Received: from [192.168.1.4] ([177.194.37.86]) by smtp.gmail.com with ESMTPSA id d21sm12412912qtp.74.2021.05.18.06.08.38 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 18 May 2021 06:08:39 -0700 (PDT) Subject: Re: [PATCH] ldconfig: Fix memory leaks To: Siddhesh Poyarekar , Siddhesh Poyarekar , libc-alpha@sourceware.org References: <20210511171627.360100-1-siddhesh@sourceware.org> <1ba04d0c-6b76-94e4-4c60-ad2246aa1357@linaro.org> <65de069f-72f6-e8c5-b5e6-1930bdfae607@gotplt.org> <2cf48d15-86b7-4def-fc9f-80e7fd5a5dae@sourceware.org> From: Adhemerval Zanella Message-ID: <8bb09ee0-c85c-1e16-aa56-7a4b57982c71@linaro.org> Date: Tue, 18 May 2021 10:08:37 -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: <2cf48d15-86b7-4def-fc9f-80e7fd5a5dae@sourceware.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=unavailable 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: Tue, 18 May 2021 13:08:51 -0000 On 18/05/2021 01:52, Siddhesh Poyarekar wrote: > On 5/18/21 10:19 AM, Siddhesh Poyarekar wrote: >> On 5/18/21 9:13 AM, Siddhesh Poyarekar via Libc-alpha wrote: >>> On 5/17/21 10:45 PM, Adhemerval Zanella wrote: >>>> >>>> >>>> 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. >>> >>> I'll post a separate patch for it. >>> >> >> After taking a closer look, I'm not sure if this is possible. chroot_canon does return the real path, but as if it were in a chroot. So if you have opt_chroot="/var/chroot" and path is "/var/chroot/usr/bin" then realpath(3) will give "/var/chroot/usr/bin" while chroot_canon() will return "/usr/bin". > > > Ahh wait, no.  The returned path includes the CHROOT prefix, so in this example they should in fact be equivalent.  Let me look at this again. I think the straightforwards fix would be the below. My understanding is chroot_canon tries to support a path larger than PATH_MAX, since it basically prepends the CHROOT argument and then resolve the NAME. I am not sure if this is really necessary, maybe we just allocate a PATH_MAX buffer and use realpath on the [chroot,name] string. diff --git a/elf/chroot_canon.c b/elf/chroot_canon.c index 045611e730..1b10a3037f 100644 --- a/elf/chroot_canon.c +++ b/elf/chroot_canon.c @@ -15,17 +15,10 @@ You should have received a copy of the GNU General Public License along with this program; if not, see . */ -#include -#include -#include -#include -#include #include -#include -#include - -#include #include +#include +#include #ifndef PATH_MAX #define PATH_MAX 1024 @@ -40,138 +33,18 @@ char * chroot_canon (const char *chroot, const char *name) { - char *rpath; - char *dest; - char *extra_buf = NULL; - char *rpath_root; - const char *start; - const char *end; - const char *rpath_limit; - int num_links = 0; size_t chroot_len = strlen (chroot); - if (chroot_len < 1) { __set_errno (EINVAL); return NULL; } - - rpath = xmalloc (chroot_len + PATH_MAX); - - rpath_limit = rpath + chroot_len + PATH_MAX; - - rpath_root = (char *) mempcpy (rpath, chroot, chroot_len) - 1; - if (*rpath_root != '/') - *++rpath_root = '/'; - dest = rpath_root + 1; - - for (start = end = name; *start; start = end) + char *rpath = xmalloc (chroot_len + PATH_MAX); + char *rpath_root = (char *) mempcpy (rpath, chroot, chroot_len) - 1; + if (realpath (name, rpath_root) == NULL) { - struct stat64 st; - - /* Skip sequence of multiple path-separators. */ - while (*start == '/') - ++start; - - /* Find end of path component. */ - for (end = start; *end && *end != '/'; ++end) - /* Nothing. */; - - if (end - start == 0) - break; - else if (end - start == 1 && start[0] == '.') - /* nothing */; - else if (end - start == 2 && start[0] == '.' && start[1] == '.') - { - /* Back up to previous component, ignore if at root already. */ - if (dest > rpath_root + 1) - while ((--dest)[-1] != '/'); - } - else - { - size_t new_size; - - if (dest[-1] != '/') - *dest++ = '/'; - - if (dest + (end - start) >= rpath_limit) - { - ptrdiff_t dest_offset = dest - rpath; - char *new_rpath; - - new_size = rpath_limit - rpath; - if (end - start + 1 > PATH_MAX) - new_size += end - start + 1; - else - new_size += PATH_MAX; - new_rpath = (char *) xrealloc (rpath, new_size); - rpath = new_rpath; - rpath_limit = rpath + new_size; - - dest = rpath + dest_offset; - } - - dest = mempcpy (dest, start, end - start); - *dest = '\0'; - - if (lstat64 (rpath, &st) < 0) - { - if (*end == '\0') - goto done; - goto error; - } - - if (S_ISLNK (st.st_mode)) - { - char *buf = alloca (PATH_MAX); - size_t len; - - if (++num_links > __eloop_threshold ()) - { - __set_errno (ELOOP); - goto error; - } - - ssize_t n = readlink (rpath, buf, PATH_MAX - 1); - if (n < 0) - { - if (*end == '\0') - goto done; - goto error; - } - buf[n] = '\0'; - - if (!extra_buf) - extra_buf = alloca (PATH_MAX); - - len = strlen (end); - if (len >= PATH_MAX - n) - { - __set_errno (ENAMETOOLONG); - goto error; - } - - /* Careful here, end may be a pointer into extra_buf... */ - memmove (&extra_buf[n], end, len + 1); - name = end = memcpy (extra_buf, buf, n); - - if (buf[0] == '/') - dest = rpath_root + 1; /* It's an absolute symlink */ - else - /* Back up to previous component, ignore if at root already: */ - if (dest > rpath_root + 1) - while ((--dest)[-1] != '/'); - } - } + free (rpath); + return NULL; } - done: - if (dest > rpath_root + 1 && dest[-1] == '/') - --dest; - *dest = '\0'; - return rpath; - - error: - free (rpath); - return NULL; }