diff mbox series

ldconfig: Fix memory leaks

Message ID 20210511171627.360100-1-siddhesh@sourceware.org
State Superseded
Headers show
Series ldconfig: Fix memory leaks | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Siddhesh Poyarekar May 11, 2021, 5:16 p.m. UTC
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

Siddhesh Poyarekar May 17, 2021, 4 p.m. UTC | #1
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.  */
>
Adhemerval Zanella May 17, 2021, 5:15 p.m. UTC | #2
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.  */
>
Siddhesh Poyarekar May 18, 2021, 3:43 a.m. UTC | #3
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
Siddhesh Poyarekar May 18, 2021, 4:49 a.m. UTC | #4
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
Siddhesh Poyarekar May 18, 2021, 4:52 a.m. UTC | #5
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
Adhemerval Zanella May 18, 2021, 1:08 p.m. UTC | #6
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;
 }
Siddhesh Poyarekar May 19, 2021, 4:24 a.m. UTC | #7
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;
>   }
>
Adhemerval Zanella May 19, 2021, 1:54 p.m. UTC | #8
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.
Siddhesh Poyarekar May 19, 2021, 2:03 p.m. UTC | #9
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
Adhemerval Zanella May 19, 2021, 2:22 p.m. UTC | #10
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 mbox series

Patch

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