| Message ID | 20210511171627.360100-1-siddhesh@sourceware.org |
|---|---|
| State | New |
| Headers | show |
| Series | ldconfig: Fix memory leaks | expand |
Ping! On 5/11/21 10:46 PM, Siddhesh Poyarekar via Libc-alpha wrote: > Coverity discovered that paths allocated by chroot_canon are not freed > in a couple of routines in ldconfig. > --- > 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); > @@ -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! */ > 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; > } > > 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; > } > 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); > } > > > @@ -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); > + > continue; > } > + > + if (opt_chroot) > + free (target_name); > + > is_dir = S_ISDIR (stat_buf.st_mode); > > /* lstat_buf is later stored, update contents. */ >
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 <adhemerval.zanella@linaro.org> > --- > 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. */ >
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. > > Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > >> --- >> 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 ? You're right, it's unnecessary, I'll revert this hunk. > >> @@ -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. OK, fixed. Thanks, Siddhesh
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". Do you think there's still scope for both to be equivalent? Siddhesh
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. Siddhesh
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 <https://www.gnu.org/licenses/>. */ -#include <stdlib.h> -#include <string.h> -#include <unistd.h> -#include <limits.h> -#include <sys/stat.h> #include <errno.h> -#include <stddef.h> -#include <stdint.h> - -#include <eloop-threshold.h> #include <ldconfig.h> +#include <stdlib.h> +#include <string.h> #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; }
On 5/18/21 6:38 PM, Adhemerval Zanella wrote: > 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. OK, now I understand what you meant. > 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. But that's not what you're doing... > 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 <https://www.gnu.org/licenses/>. */ > > -#include <stdlib.h> > -#include <string.h> > -#include <unistd.h> > -#include <limits.h> > -#include <sys/stat.h> > #include <errno.h> > -#include <stddef.h> > -#include <stdint.h> > - > -#include <eloop-threshold.h> > #include <ldconfig.h> > +#include <stdlib.h> > +#include <string.h> > > #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) NAME is not a valid absolute path outside of the chroot; maybe what you need is: // XXX Free rpath before return. char *rpath = xmalloc (chroot_len + PATH_MAX + 1); char *rpath_ret = xmalloc (PATH_MAX); char *rpath_root = (char *) mempcpy (rpath, chroot, chroot_len) - 1; if (*rpath_root != '/') *++rpath_root = '/'; memcpy (rpath_root + 1, path, PATH_MAX); if (realpath (rpath, rpath_ret) == NULL) ... return rpath_ret; > { > - 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; > } >
On 19/05/2021 01:24, Siddhesh Poyarekar wrote: > On 5/18/21 6:38 PM, Adhemerval Zanella wrote: >> 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. > > OK, now I understand what you meant. > >> 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. > > But that's not what you're doing... Yeah, I noticed it when I realized that issuing realpath solely PATH does not really make sense. > >> 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 <https://www.gnu.org/licenses/>. */ >> -#include <stdlib.h> >> -#include <string.h> >> -#include <unistd.h> >> -#include <limits.h> >> -#include <sys/stat.h> >> #include <errno.h> >> -#include <stddef.h> >> -#include <stdint.h> >> - >> -#include <eloop-threshold.h> >> #include <ldconfig.h> >> +#include <stdlib.h> >> +#include <string.h> >> #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) > > NAME is not a valid absolute path outside of the chroot; maybe what you need is: > > // XXX Free rpath before return. > char *rpath = xmalloc (chroot_len + PATH_MAX + 1); > char *rpath_ret = xmalloc (PATH_MAX); > char *rpath_root = (char *) mempcpy (rpath, chroot, chroot_len) - 1; > if (*rpath_root != '/') > *++rpath_root = '/'; > memcpy (rpath_root + 1, path, PATH_MAX); > > if (realpath (rpath, rpath_ret) == NULL) > > ... > > return rpath_ret; It would be straightforward change, but I think there is no much gain in either supporting a path larger than PATH_MAX (it might fail in later usages anyway if the full path is used) or trying to add the optimization of just resolving the PATH. What I really dislike is that chroot_canon is quite similar to the old realpath implementation we had, and analyzing its usage I think we can just resolve the full path ([chroot,path]) with realpath instead of call chroot_canon. It would require more changes in ldconfig code though.
On 5/19/21 7:24 PM, Adhemerval Zanella wrote: > It would be straightforward change, but I think there is no much gain in > either supporting a path larger than PATH_MAX (it might fail in later > usages anyway if the full path is used) or trying to add the optimization > of just resolving the PATH. I don't think it can support paths larger than PATH_MAX because the later lstat64 will fail with ENAMETOOLONG. > What I really dislike is that chroot_canon is quite similar to the old > realpath implementation we had, and analyzing its usage I think we can > just resolve the full path ([chroot,path]) with realpath instead of > call chroot_canon. It would require more changes in ldconfig code > though. I think your patch with the changes I suggested ought to be a sufficient minimal change. Do you want to fix up and post? Siddhesh
On 19/05/2021 11:03, Siddhesh Poyarekar wrote: > On 5/19/21 7:24 PM, Adhemerval Zanella wrote: >> It would be straightforward change, but I think there is no much gain in >> either supporting a path larger than PATH_MAX (it might fail in later >> usages anyway if the full path is used) or trying to add the optimization >> of just resolving the PATH. > > I don't think it can support paths larger than PATH_MAX because the later lstat64 will fail with ENAMETOOLONG. > >> What I really dislike is that chroot_canon is quite similar to the old >> realpath implementation we had, and analyzing its usage I think we can >> just resolve the full path ([chroot,path]) with realpath instead of >> call chroot_canon. It would require more changes in ldconfig code >> though. > > I think your patch with the changes I suggested ought to be a sufficient minimal change. Do you want to fix up and post? Alright, I think it is the simplest approach.
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); @@ -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! */ 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; } 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; } 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); } @@ -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); + continue; } + + if (opt_chroot) + free (target_name); + is_dir = S_ISDIR (stat_buf.st_mode); /* lstat_buf is later stored, update contents. */