[PATCH] ldconfig: Fix memory leaks

[PATCH] ldconfig: Fix memory leaks

[Siddhesh Poyarekar]

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.  */
-- 
2.31.1

[PING][PATCH] ldconfig: Fix memory leaks

[Siddhesh Poyarekar]

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.  */
> 

Re: [PATCH] ldconfig: Fix memory leaks

[Adhemerval Zanella]

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.  */
> 

Re: [PATCH] ldconfig: Fix memory leaks

[Siddhesh Poyarekar]

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

Re: [PATCH] ldconfig: Fix memory leaks

[Siddhesh Poyarekar]

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

Re: [PATCH] ldconfig: Fix memory leaks

[Siddhesh Poyarekar]

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

Re: [PATCH] ldconfig: Fix memory leaks

[Adhemerval Zanella]

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;
 }

Re: [PATCH] ldconfig: Fix memory leaks

[Siddhesh Poyarekar]

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;
>   }
> 

Re: [PATCH] ldconfig: Fix memory leaks

[Adhemerval Zanella]

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.

Re: [PATCH] ldconfig: Fix memory leaks

[Siddhesh Poyarekar]

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

Re: [PATCH] ldconfig: Fix memory leaks

[Adhemerval Zanella]

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.