ldconfig: Fix memory leaks

Message ID 20210511171627.360100-1-siddhesh@sourceware.org
State New
Headers show
Series
  • ldconfig: Fix memory leaks
Related show

Commit Message

naohirot--- via Libc-alpha May 11, 2021, 5:16 p.m.
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(-)

-- 
2.31.1

Comments

naohirot--- via Libc-alpha May 17, 2021, 4 p.m. | #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.  */

>
naohirot--- via Libc-alpha May 17, 2021, 5:15 p.m. | #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.  */

>
naohirot--- via Libc-alpha May 18, 2021, 3:43 a.m. | #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. | #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
naohirot--- via Libc-alpha May 18, 2021, 4:52 a.m. | #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
naohirot--- via Libc-alpha May 18, 2021, 1:08 p.m. | #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;
 }
naohirot--- via Libc-alpha May 19, 2021, 4:24 a.m. | #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;

>   }

>
naohirot--- via Libc-alpha May 19, 2021, 1:54 p.m. | #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.
naohirot--- via Libc-alpha May 19, 2021, 2:03 p.m. | #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
naohirot--- via Libc-alpha May 19, 2021, 2:22 p.m. | #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.

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