ldconfig: Fix memory leaks
Checks
Commit Message
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(-)
Comments
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.
@@ -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. */