From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa1-x33.google.com (mail-oa1-x33.google.com [IPv6:2001:4860:4864:20::33]) by sourceware.org (Postfix) with ESMTPS id 408643858D35 for ; Tue, 23 May 2023 17:57:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 408643858D35 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-oa1-x33.google.com with SMTP id 586e51a60fabf-19e8a61fdbaso106249fac.0 for ; Tue, 23 May 2023 10:57:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1684864667; x=1687456667; h=content-transfer-encoding:in-reply-to:organization:from :content-language:references:cc:to:subject:user-agent:mime-version :date:message-id:from:to:cc:subject:date:message-id:reply-to; bh=5Byamh+/B3PSHStp6QWneJCU25OgJmRfb+d0t/zEpds=; b=Ov3IrMyPiueAmY68Lzb6h1nuTha2m6VOE8oRDJJvLmqcaAjMIjwluSYz5tv9Q0EdsY W3pAgdO7hYrVAOptHdLPfcJEM3T8s4cBJ7sQJWIUvKiWGqOL+gVMivKo6cY3BSKx6vSr zjmASHFM/H1QlN5mZO9AeZi+Ap6OmPbK+SaHpRFmB9/2DPU4DD/Z5Li9SSx7ZSsUkD7k nKdEukhv+MDwkWAJd3p4DlqrKs6Bf+NdwrngQsJ3r8XvHjDwng0zyswGX8LbCSNCcCpO Wu+LpRlw2TccAUL1O35C6QmEC/TmqJZWl56v+khqwQ42d/u+ZbOe187OeH4QvlVUzInY H9QQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684864667; x=1687456667; h=content-transfer-encoding:in-reply-to:organization:from :content-language:references:cc:to:subject:user-agent:mime-version :date:message-id:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=5Byamh+/B3PSHStp6QWneJCU25OgJmRfb+d0t/zEpds=; b=Qtz1bP1hwkkJ7BKA5H2AwcxJJPqmQWL7a9QLsuufXXubb/ON+AHny3cMVqwb14cqkS Hd6/pH78jhF7hOdbPCaBDVJkdJ/M3zzNEJ6t0qWJNSyTvYMCfi6H5lZ+9vNh5oyblztY /dB2QgC+Gt10V23+Uk2s8YKYqr5opdGN3qCj7V9grSdcSAymt7mucXbHUvaIUawgBbcK bL7lUiAbcS3I5BbdnPmw2ILzTy+puQUfj1dsd40bpycmPvG8/iqDaFtJNGAJoij3xDpC tr+L0ebRckEaUmB4xIl9A/byLDeqEsMMizxAZgs3BYEHx0qtRpC5Fi0i2NcezAnCyr6b toqw== X-Gm-Message-State: AC+VfDxaGCxhob8g8verlGFVTc3pzmswQO8tvzgFzclu5aplp2fqasPK 5Du9OqCzB3okv3GIZmd/f7tOjQ== X-Google-Smtp-Source: ACHHUZ5bEmUsApU3ixTgoM/+PU/DtkDBbfPARHas9YlJqFiBp8RGOfLYl2ZtOnZJ/EBz6KJfIyFzUw== X-Received: by 2002:a05:6870:f5a0:b0:192:5ffb:47ff with SMTP id eh32-20020a056870f5a000b001925ffb47ffmr8033637oab.27.1684864667450; Tue, 23 May 2023 10:57:47 -0700 (PDT) Received: from ?IPV6:2804:1b3:a7c1:2e05:a4ad:a873:8177:a796? ([2804:1b3:a7c1:2e05:a4ad:a873:8177:a796]) by smtp.gmail.com with ESMTPSA id i5-20020a056870220500b0019a291d1672sm1720016oaf.26.2023.05.23.10.57.45 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 23 May 2023 10:57:46 -0700 (PDT) Message-ID: <4163e51f-f983-d1a1-20e3-3ef2ae38fcde@linaro.org> Date: Tue, 23 May 2023 14:57:43 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [RFC PATCH 1/2] elf: Port ldconfig away from stack-allocated paths To: Sergey Bugaev , libc-alpha@sourceware.org, Florian Weimer Cc: bug-hurd@gnu.org References: <87jzx4jynj.fsf@oldenburg3.str.redhat.com> <20230520190306.4008296-1-bugaevc@gmail.com> Content-Language: en-US From: Adhemerval Zanella Netto Organization: Linaro In-Reply-To: <20230520190306.4008296-1-bugaevc@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.9 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,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 20/05/23 16:03, Sergey Bugaev wrote: > Hello, > > On Fri, May 19, 2023 at 3:25 PM Florian Weimer wrote: >> Missing error checking? > > Indeed, thanks. Fixed. > >> I think we use x* functions such a xstrdup elsewhere in ldconfig, so you >> could add xasprintf as well. > > Apparently the existing code didn't #include and link > to libsupport (xstrdup must be coming from somewhere else.) I opted to > check for asprintf returning < 0 and calling error () explicitly instead, > like other code in this same file does already. > > Sergey > > -- >8 -- > Subject: [PATCH 1/2 v2] elf: Port ldconfig away from stack-allocated paths > > ldconfig was allocating PATH_MAX bytes on the stack for the library file > name. The issues with PATH_MAX usage are well documented [0][1]; even if > a program does not rely on paths being limited to PATH_MAX bytes, > allocating 4096 bytes on the stack for paths that are typically rather > short (strlen ("/lib64/libc.so.6") is 16) is wasteful and dangerous. > > [0]: https://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html > [1]: https://eklitzke.org/path-max-is-tricky > > Instead, make use of asprintf to dynamically allocate memory of just the > right size on the heap. > > Reviewed-by: Adhemerval Zanella > Signed-off-by: Sergey Bugaev Still looks good to me, thanks. > --- > elf/ldconfig.c | 60 +++++++++++++++++++------------------------------- > 1 file changed, 23 insertions(+), 37 deletions(-) > > diff --git a/elf/ldconfig.c b/elf/ldconfig.c > index 2fc45ad8..e643dd57 100644 > --- a/elf/ldconfig.c > +++ b/elf/ldconfig.c > @@ -677,28 +677,18 @@ search_dir (const struct dir_entry *entry) > > char *dir_name; > char *real_file_name; > - size_t real_file_name_len; > - size_t file_name_len = PATH_MAX; > - char *file_name = alloca (file_name_len); > + char *file_name; > if (opt_chroot != NULL) > - { > - dir_name = chroot_canon (opt_chroot, entry->path); > - real_file_name_len = PATH_MAX; > - real_file_name = alloca (real_file_name_len); > - } > + dir_name = chroot_canon (opt_chroot, entry->path); > else > - { > - dir_name = entry->path; > - real_file_name_len = 0; > - real_file_name = file_name; > - } > + dir_name = entry->path; > > DIR *dir; > if (dir_name == NULL || (dir = opendir (dir_name)) == NULL) > { > if (opt_verbose) > error (0, errno, _("Can't open directory %s"), entry->path); > - if (opt_chroot != NULL && dir_name != NULL) > + if (opt_chroot != NULL) > free (dir_name); > return; > } > @@ -733,25 +723,16 @@ search_dir (const struct dir_entry *entry) > + 1, ".#prelink#.", sizeof (".#prelink#.") - 1) == 0) > continue; > } > - len += strlen (entry->path) + 2; > - if (len > file_name_len) > - { > - file_name_len = len; > - file_name = alloca (file_name_len); > - if (!opt_chroot) > - real_file_name = file_name; > - } > - sprintf (file_name, "%s/%s", entry->path, direntry->d_name); > + if (asprintf (&file_name, "%s/%s", entry->path, direntry->d_name) < 0) > + error (EXIT_FAILURE, errno, _("Could not form library path")); > if (opt_chroot != NULL) > { > - len = strlen (dir_name) + strlen (direntry->d_name) + 2; > - if (len > real_file_name_len) > - { > - real_file_name_len = len; > - real_file_name = alloca (real_file_name_len); > - } > - sprintf (real_file_name, "%s/%s", dir_name, direntry->d_name); > + if (asprintf (&real_file_name, "%s/%s", > + dir_name, direntry->d_name) < 0) > + error (EXIT_FAILURE, errno, _("Could not form library path")); > } > + else > + real_file_name = file_name; > > struct stat lstat_buf; > /* We optimize and try to do the lstat call only if needed. */ > @@ -761,7 +742,7 @@ search_dir (const struct dir_entry *entry) > if (__glibc_unlikely (lstat (real_file_name, &lstat_buf))) > { > error (0, errno, _("Cannot lstat %s"), file_name); > - continue; > + goto next; > } > > struct stat stat_buf; > @@ -778,7 +759,7 @@ search_dir (const struct dir_entry *entry) > { > if (strstr (file_name, ".so") == NULL) > error (0, 0, _("Input file %s not found.\n"), file_name); > - continue; > + goto next; > } > } > if (__glibc_unlikely (stat (target_name, &stat_buf))) > @@ -793,7 +774,7 @@ search_dir (const struct dir_entry *entry) > if (opt_chroot != NULL) > free (target_name); > > - continue; > + goto next; > } > > if (opt_chroot != NULL) > @@ -806,7 +787,7 @@ search_dir (const struct dir_entry *entry) > lstat_buf.st_ctime = stat_buf.st_ctime; > } > else if (!S_ISREG (lstat_buf.st_mode)) > - continue; > + goto next; > > char *real_name; > if (opt_chroot != NULL && is_link) > @@ -816,7 +797,7 @@ search_dir (const struct dir_entry *entry) > { > if (strstr (file_name, ".so") == NULL) > error (0, 0, _("Input file %s not found.\n"), file_name); > - continue; > + goto next; > } > } > else > @@ -828,7 +809,7 @@ search_dir (const struct dir_entry *entry) > && __builtin_expect (lstat (real_file_name, &lstat_buf), 0)) > { > error (0, errno, _("Cannot lstat %s"), file_name); > - continue; > + goto next; > } > > /* First search whether the auxiliary cache contains this > @@ -842,7 +823,7 @@ search_dir (const struct dir_entry *entry) > { > if (real_name != real_file_name) > free (real_name); > - continue; > + goto next; > } > else if (opt_build_cache) > add_to_aux_cache (&lstat_buf, flag, isa_level, soname); > @@ -948,6 +929,11 @@ search_dir (const struct dir_entry *entry) > dlib_ptr->next = dlibs; > dlibs = dlib_ptr; > } > + > + next: > + free (file_name); > + if (opt_chroot != NULL) > + free (real_file_name); > } > > closedir (dir);