Re: [PATCH] ldconfig: Fix memory leaks

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

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