[PATCH 0/2] elf: dl-load error handling refactoring

[PATCH 0/2] elf: dl-load error handling refactoring

[Szabolcs Nagy]

Fix some failure handling in _dl_map_object_from_fd and
slightly improve maintainability.

This used to be part of the BTI _dl_process_gnu_property
changes, but those no longer depend on failure handling here.
However if the code is ever changed so that note processing
can propagate failures this patchset would help.

Szabolcs Nagy (2):
  elf: inline lose for error handling
  elf: Fix failure handling in _dl_map_object_from_fd

 elf/dl-load.c | 110 ++++++++++++++++++++++++--------------------------
 1 file changed, 53 insertions(+), 57 deletions(-)

-- 
2.17.1

[PATCH 1/2] elf: inline lose for error handling

[Szabolcs Nagy]

_dl_map_object_from_fd has complex error handling with cleanups.
It was managed by a separate function to avoid code bloat at
every failure case, but since the code was changed to use gotos
there is no longer such code bloat from inlining.

Maintaining a separate error handling function is harder as it
needs to access local state which has to be passed down. And the
same lose function was used in open_verify which is error prone.

The goto labels are changed since there is no longer a call.
The new code generates slightly smaller binary.
---
 elf/dl-load.c | 87 +++++++++++++++++++++++----------------------------
 1 file changed, 39 insertions(+), 48 deletions(-)

diff --git a/elf/dl-load.c b/elf/dl-load.c
index e9afad544a..10ccca20d0 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -838,30 +838,6 @@ _dl_init_paths (const char *llp, const char *source,
 }
 
 
-static void
-__attribute__ ((noreturn, noinline))
-lose (int code, int fd, const char *name, char *realname, struct link_map *l,
-      const char *msg, struct r_debug *r, Lmid_t nsid)
-{
-  /* The file might already be closed.  */
-  if (fd != -1)
-    (void) __close_nocancel (fd);
-  if (l != NULL && l->l_origin != (char *) -1l)
-    free ((char *) l->l_origin);
-  free (l);
-  free (realname);
-
-  if (r != NULL)
-    {
-      r->r_state = RT_CONSISTENT;
-      _dl_debug_state ();
-      LIBC_PROBE (map_failed, 2, nsid, r);
-    }
-
-  _dl_signal_error (code, name, NULL, msg);
-}
-
-
 /* Process PT_GNU_PROPERTY program header PH in module L after
    PT_LOAD segments are mapped.  Only one NT_GNU_PROPERTY_TYPE_0
    note is handled which contains processor specific properties.
@@ -973,11 +949,25 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
       if (__glibc_unlikely (!_dl_get_file_id (fd, &id)))
 	{
 	  errstring = N_("cannot stat shared object");
-	call_lose_errno:
+	lose_errno:
 	  errval = errno;
-	call_lose:
-	  lose (errval, fd, name, realname, l, errstring,
-		make_consistent ? r : NULL, nsid);
+	lose:
+	  /* The file might already be closed.  */
+	  if (fd != -1)
+	    (void) __close_nocancel (fd);
+	  if (l != NULL && l->l_origin != (char *) -1l)
+	    free ((char *) l->l_origin);
+	  free (l);
+	  free (realname);
+
+	  if (make_consistent && r != NULL)
+	    {
+	      r->r_state = RT_CONSISTENT;
+	      _dl_debug_state ();
+	      LIBC_PROBE (map_failed, 2, nsid, r);
+	    }
+
+	  _dl_signal_error (errval, name, NULL, errstring);
 	}
 
       /* Look again to see if the real name matched another already loaded.  */
@@ -1084,7 +1074,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
     fail_new:
 #endif
       errstring = N_("cannot create shared object descriptor");
-      goto call_lose_errno;
+      goto lose_errno;
     }
 
   /* Extract the remaining details we need from the ELF header
@@ -1103,7 +1093,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
 				       header->e_phoff) != maplength)
 	{
 	  errstring = N_("cannot read file data");
-	  goto call_lose_errno;
+	  goto lose_errno;
 	}
     }
 
@@ -1149,14 +1139,14 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
 	  if (__glibc_unlikely ((ph->p_align & (GLRO(dl_pagesize) - 1)) != 0))
 	    {
 	      errstring = N_("ELF load command alignment not page-aligned");
-	      goto call_lose;
+	      goto lose;
 	    }
 	  if (__glibc_unlikely (((ph->p_vaddr - ph->p_offset)
 				 & (ph->p_align - 1)) != 0))
 	    {
 	      errstring
 		= N_("ELF load command address/offset not properly aligned");
-	      goto call_lose;
+	      goto lose;
 	    }
 
 	  struct loadcmd *c = &loadcmds[nloadcmds++];
@@ -1235,7 +1225,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
 	   another error below.  But we don't want to go through the
 	   calculations below using NLOADCMDS - 1.  */
 	errstring = N_("object file has no loadable segments");
-	goto call_lose;
+	goto lose;
       }
 
     /* dlopen of an executable is not valid because it is not possible
@@ -1248,7 +1238,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
 	/* This object is loaded at a fixed address.  This must never
 	   happen for objects loaded with dlopen.  */
 	errstring = N_("cannot dynamically load executable");
-	goto call_lose;
+	goto lose;
       }
 
     /* Length of the sections to be loaded.  */
@@ -1261,7 +1251,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
     errstring = _dl_map_segments (l, fd, header, type, loadcmds, nloadcmds,
 				  maplength, has_holes, loader);
     if (__glibc_unlikely (errstring != NULL))
-      goto call_lose;
+      goto lose;
 
     /* Process program headers again after load segments are mapped in
        case processing requires accessing those segments.  Scan program
@@ -1284,7 +1274,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
       if (__glibc_unlikely (type == ET_DYN))
 	{
 	  errstring = N_("object file has no dynamic section");
-	  goto call_lose;
+	  goto lose;
 	}
     }
   else
@@ -1313,7 +1303,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
 	  = N_("cannot dynamically load position-independent executable");
       else
 	errstring = N_("shared object cannot be dlopen()ed");
-      goto call_lose;
+      goto lose;
     }
 
   if (l->l_phdr == NULL)
@@ -1326,7 +1316,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
       if (newp == NULL)
 	{
 	  errstring = N_("cannot allocate memory for program header");
-	  goto call_lose_errno;
+	  goto lose_errno;
 	}
 
       l->l_phdr = memcpy (newp, phdr,
@@ -1359,7 +1349,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
 	      if (__mprotect ((void *) p, s, PROT_READ|PROT_WRITE) < 0)
 		{
 		  errstring = N_("cannot change memory protections");
-		  goto call_lose_errno;
+		  goto lose_errno;
 		}
 	      __stack_prot |= PROT_READ|PROT_WRITE|PROT_EXEC;
 	      __mprotect ((void *) p, s, PROT_READ);
@@ -1380,7 +1370,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
 	{
 	  errstring = N_("\
 cannot enable executable stack as shared object requires");
-	  goto call_lose;
+	  goto lose;
 	}
     }
 
@@ -1407,7 +1397,7 @@ cannot enable executable stack as shared object requires");
   if (__glibc_unlikely (__close_nocancel (fd) != 0))
     {
       errstring = N_("cannot close file descriptor");
-      goto call_lose_errno;
+      goto lose_errno;
     }
   /* Signal that we closed the file.  */
   fd = -1;
@@ -1686,14 +1676,15 @@ open_verify (const char *name, int fd,
 	  errval = errno;
 	  errstring = (errval == 0
 		       ? N_("file too short") : N_("cannot read file data"));
-	call_lose:
+	lose:
 	  if (free_name)
 	    {
 	      char *realname = (char *) name;
 	      name = strdupa (realname);
 	      free (realname);
 	    }
-	  lose (errval, fd, name, NULL, NULL, errstring, NULL, 0);
+	  (void) __close_nocancel (fd);
+	  _dl_signal_error (errval, name, NULL, errstring);
 	}
 
       /* See whether the ELF header is what we expect.  */
@@ -1753,13 +1744,13 @@ open_verify (const char *name, int fd,
 	    /* Otherwise we don't know what went wrong.  */
 	    errstring = N_("internal error");
 
-	  goto call_lose;
+	  goto lose;
 	}
 
       if (__glibc_unlikely (ehdr->e_version != EV_CURRENT))
 	{
 	  errstring = N_("ELF file version does not match current one");
-	  goto call_lose;
+	  goto lose;
 	}
       if (! __glibc_likely (elf_machine_matches_host (ehdr)))
 	goto close_and_out;
@@ -1767,12 +1758,12 @@ open_verify (const char *name, int fd,
 				 && ehdr->e_type != ET_EXEC))
 	{
 	  errstring = N_("only ET_DYN and ET_EXEC can be loaded");
-	  goto call_lose;
+	  goto lose;
 	}
       else if (__glibc_unlikely (ehdr->e_phentsize != sizeof (ElfW(Phdr))))
 	{
 	  errstring = N_("ELF file's phentsize not the expected size");
-	  goto call_lose;
+	  goto lose;
 	}
 
       maplength = ehdr->e_phnum * sizeof (ElfW(Phdr));
@@ -1787,7 +1778,7 @@ open_verify (const char *name, int fd,
 	    read_error:
 	      errval = errno;
 	      errstring = N_("cannot read file data");
-	      goto call_lose;
+	      goto lose;
 	    }
 	}
 
-- 
2.17.1

[PATCH 2/2] elf: Fix failure handling in _dl_map_object_from_fd

[Szabolcs Nagy]

The failure paths in _dl_map_object_from_fd did not clean every
potentially allocated resource up.

Handle l_phdr, l_libname and mapped segments in the common failure
handling code.

There are various bits that may not be cleaned properly on failure
(e.g. executable stack, incomplete dl_map_segments) fixing those
need further changes.
---
 elf/dl-load.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/elf/dl-load.c b/elf/dl-load.c
index 10ccca20d0..a9a2a1b63f 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -955,8 +955,14 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
 	  /* The file might already be closed.  */
 	  if (fd != -1)
 	    (void) __close_nocancel (fd);
+	  if (l != NULL && l->l_map_start != 0)
+	    _dl_unmap_segments (l);
 	  if (l != NULL && l->l_origin != (char *) -1l)
 	    free ((char *) l->l_origin);
+	  if (l != NULL && !l->l_libname->dont_free)
+	    free (l->l_libname);
+	  if (l != NULL && l->l_phdr_allocated)
+	    free ((void *) l->l_phdr);
 	  free (l);
 	  free (realname);
 
@@ -1251,7 +1257,11 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
     errstring = _dl_map_segments (l, fd, header, type, loadcmds, nloadcmds,
 				  maplength, has_holes, loader);
     if (__glibc_unlikely (errstring != NULL))
-      goto lose;
+      {
+	/* Mappings can be in an inconsistent state: avoid unmap.  */
+	l->l_map_start = l->l_map_end = 0;
+	goto lose;
+      }
 
     /* Process program headers again after load segments are mapped in
        case processing requires accessing those segments.  Scan program
@@ -1289,15 +1299,6 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
       || (__glibc_unlikely (l->l_flags_1 & DF_1_PIE)
 	  && __glibc_unlikely ((mode & __RTLD_OPENEXEC) == 0)))
     {
-      /* We are not supposed to load this object.  Free all resources.  */
-      _dl_unmap_segments (l);
-
-      if (!l->l_libname->dont_free)
-	free (l->l_libname);
-
-      if (l->l_phdr_allocated)
-	free ((void *) l->l_phdr);
-
       if (l->l_flags_1 & DF_1_PIE)
 	errstring
 	  = N_("cannot dynamically load position-independent executable");
@@ -1402,6 +1403,10 @@ cannot enable executable stack as shared object requires");
   /* Signal that we closed the file.  */
   fd = -1;
 
+  /* Failures before this point are handled locally via lose.
+     There are no more failures in this function until return,
+     to change that the cleanup handling needs to be updated.  */
+
   /* If this is ET_EXEC, we should have loaded it as lt_executable.  */
   assert (type != ET_EXEC || l->l_type == lt_executable);
 
-- 
2.17.1

Re: [PATCH 1/2] elf: inline lose for error handling

[Adhemerval Zanella]

On 11/12/2020 13:35, Szabolcs Nagy via Libc-alpha wrote:
> _dl_map_object_from_fd has complex error handling with cleanups.
> It was managed by a separate function to avoid code bloat at
> every failure case, but since the code was changed to use gotos
> there is no longer such code bloat from inlining.
> 
> Maintaining a separate error handling function is harder as it
> needs to access local state which has to be passed down. And the
> same lose function was used in open_verify which is error prone.
> 
> The goto labels are changed since there is no longer a call.
> The new code generates slightly smaller binary.

LGTM, thanks.  

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>


> ---
>  elf/dl-load.c | 87 +++++++++++++++++++++++----------------------------
>  1 file changed, 39 insertions(+), 48 deletions(-)
> 
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index e9afad544a..10ccca20d0 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -838,30 +838,6 @@ _dl_init_paths (const char *llp, const char *source,
>  }
>  
>  
> -static void
> -__attribute__ ((noreturn, noinline))
> -lose (int code, int fd, const char *name, char *realname, struct link_map *l,
> -      const char *msg, struct r_debug *r, Lmid_t nsid)
> -{
> -  /* The file might already be closed.  */
> -  if (fd != -1)
> -    (void) __close_nocancel (fd);
> -  if (l != NULL && l->l_origin != (char *) -1l)
> -    free ((char *) l->l_origin);
> -  free (l);
> -  free (realname);
> -
> -  if (r != NULL)
> -    {
> -      r->r_state = RT_CONSISTENT;
> -      _dl_debug_state ();
> -      LIBC_PROBE (map_failed, 2, nsid, r);
> -    }
> -
> -  _dl_signal_error (code, name, NULL, msg);
> -}
> -
> -

Ok.

>  /* Process PT_GNU_PROPERTY program header PH in module L after
>     PT_LOAD segments are mapped.  Only one NT_GNU_PROPERTY_TYPE_0
>     note is handled which contains processor specific properties.
> @@ -973,11 +949,25 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>        if (__glibc_unlikely (!_dl_get_file_id (fd, &id)))
>  	{
>  	  errstring = N_("cannot stat shared object");
> -	call_lose_errno:
> +	lose_errno:
>  	  errval = errno;
> -	call_lose:
> -	  lose (errval, fd, name, realname, l, errstring,
> -		make_consistent ? r : NULL, nsid);
> +	lose:
> +	  /* The file might already be closed.  */
> +	  if (fd != -1)
> +	    (void) __close_nocancel (fd);

I think you can remove the (void) cast here.	

> +	  if (l != NULL && l->l_origin != (char *) -1l)
> +	    free ((char *) l->l_origin);
> +	  free (l);
> +	  free (realname);
> +
> +	  if (make_consistent && r != NULL)
> +	    {
> +	      r->r_state = RT_CONSISTENT;
> +	      _dl_debug_state ();
> +	      LIBC_PROBE (map_failed, 2, nsid, r);
> +	    }
> +
> +	  _dl_signal_error (errval, name, NULL, errstring);
>  	}
>  
>        /* Look again to see if the real name matched another already loaded.  */

Ok.

> @@ -1084,7 +1074,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>      fail_new:
>  #endif
>        errstring = N_("cannot create shared object descriptor");
> -      goto call_lose_errno;
> +      goto lose_errno;
>      }
>  
>    /* Extract the remaining details we need from the ELF header

Ok.

> @@ -1103,7 +1093,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>  				       header->e_phoff) != maplength)
>  	{
>  	  errstring = N_("cannot read file data");
> -	  goto call_lose_errno;
> +	  goto lose_errno;
>  	}
>      }
>  

Ok.

> @@ -1149,14 +1139,14 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>  	  if (__glibc_unlikely ((ph->p_align & (GLRO(dl_pagesize) - 1)) != 0))
>  	    {
>  	      errstring = N_("ELF load command alignment not page-aligned");
> -	      goto call_lose;
> +	      goto lose;
>  	    }
>  	  if (__glibc_unlikely (((ph->p_vaddr - ph->p_offset)
>  				 & (ph->p_align - 1)) != 0))
>  	    {
>  	      errstring
>  		= N_("ELF load command address/offset not properly aligned");
> -	      goto call_lose;
> +	      goto lose;
>  	    }
>  
>  	  struct loadcmd *c = &loadcmds[nloadcmds++];

Ok.

> @@ -1235,7 +1225,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>  	   another error below.  But we don't want to go through the
>  	   calculations below using NLOADCMDS - 1.  */
>  	errstring = N_("object file has no loadable segments");
> -	goto call_lose;
> +	goto lose;
>        }
>  
>      /* dlopen of an executable is not valid because it is not possible
> @@ -1248,7 +1238,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>  	/* This object is loaded at a fixed address.  This must never
>  	   happen for objects loaded with dlopen.  */
>  	errstring = N_("cannot dynamically load executable");
> -	goto call_lose;
> +	goto lose;
>        }
>  
>      /* Length of the sections to be loaded.  */
> @@ -1261,7 +1251,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>      errstring = _dl_map_segments (l, fd, header, type, loadcmds, nloadcmds,
>  				  maplength, has_holes, loader);
>      if (__glibc_unlikely (errstring != NULL))
> -      goto call_lose;
> +      goto lose;
>  
>      /* Process program headers again after load segments are mapped in
>         case processing requires accessing those segments.  Scan program
> @@ -1284,7 +1274,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>        if (__glibc_unlikely (type == ET_DYN))
>  	{
>  	  errstring = N_("object file has no dynamic section");
> -	  goto call_lose;
> +	  goto lose;
>  	}
>      }
>    else
> @@ -1313,7 +1303,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>  	  = N_("cannot dynamically load position-independent executable");
>        else
>  	errstring = N_("shared object cannot be dlopen()ed");
> -      goto call_lose;
> +      goto lose;
>      }
>  
>    if (l->l_phdr == NULL)
> @@ -1326,7 +1316,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>        if (newp == NULL)
>  	{
>  	  errstring = N_("cannot allocate memory for program header");
> -	  goto call_lose_errno;
> +	  goto lose_errno;
>  	}
>  
>        l->l_phdr = memcpy (newp, phdr,
> @@ -1359,7 +1349,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>  	      if (__mprotect ((void *) p, s, PROT_READ|PROT_WRITE) < 0)
>  		{
>  		  errstring = N_("cannot change memory protections");
> -		  goto call_lose_errno;
> +		  goto lose_errno;
>  		}
>  	      __stack_prot |= PROT_READ|PROT_WRITE|PROT_EXEC;
>  	      __mprotect ((void *) p, s, PROT_READ);
> @@ -1380,7 +1370,7 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>  	{
>  	  errstring = N_("\
>  cannot enable executable stack as shared object requires");
> -	  goto call_lose;
> +	  goto lose;
>  	}
>      }
>  
> @@ -1407,7 +1397,7 @@ cannot enable executable stack as shared object requires");
>    if (__glibc_unlikely (__close_nocancel (fd) != 0))
>      {
>        errstring = N_("cannot close file descriptor");
> -      goto call_lose_errno;
> +      goto lose_errno;
>      }
>    /* Signal that we closed the file.  */
>    fd = -1;
> @@ -1686,14 +1676,15 @@ open_verify (const char *name, int fd,
>  	  errval = errno;
>  	  errstring = (errval == 0
>  		       ? N_("file too short") : N_("cannot read file data"));
> -	call_lose:
> +	lose:
>  	  if (free_name)
>  	    {
>  	      char *realname = (char *) name;
>  	      name = strdupa (realname);
>  	      free (realname);
>  	    }
> -	  lose (errval, fd, name, NULL, NULL, errstring, NULL, 0);
> +	  (void) __close_nocancel (fd);

Same as before.

> +	  _dl_signal_error (errval, name, NULL, errstring);
>  	}
>  
>        /* See whether the ELF header is what we expect.  */

Ok.

> @@ -1753,13 +1744,13 @@ open_verify (const char *name, int fd,
>  	    /* Otherwise we don't know what went wrong.  */
>  	    errstring = N_("internal error");
>  
> -	  goto call_lose;
> +	  goto lose;
>  	}
>  
>        if (__glibc_unlikely (ehdr->e_version != EV_CURRENT))
>  	{
>  	  errstring = N_("ELF file version does not match current one");
> -	  goto call_lose;
> +	  goto lose;
>  	}
>        if (! __glibc_likely (elf_machine_matches_host (ehdr)))
>  	goto close_and_out;
> @@ -1767,12 +1758,12 @@ open_verify (const char *name, int fd,
>  				 && ehdr->e_type != ET_EXEC))
>  	{
>  	  errstring = N_("only ET_DYN and ET_EXEC can be loaded");
> -	  goto call_lose;
> +	  goto lose;
>  	}
>        else if (__glibc_unlikely (ehdr->e_phentsize != sizeof (ElfW(Phdr))))
>  	{
>  	  errstring = N_("ELF file's phentsize not the expected size");
> -	  goto call_lose;
> +	  goto lose;
>  	}
>  
>        maplength = ehdr->e_phnum * sizeof (ElfW(Phdr));
> @@ -1787,7 +1778,7 @@ open_verify (const char *name, int fd,
>  	    read_error:
>  	      errval = errno;
>  	      errstring = N_("cannot read file data");
> -	      goto call_lose;
> +	      goto lose;
>  	    }
>  	}
>  
> 

Ok.

Re: [PATCH 2/2] elf: Fix failure handling in _dl_map_object_from_fd

[Adhemerval Zanella]

On 11/12/2020 13:35, Szabolcs Nagy via Libc-alpha wrote:
> The failure paths in _dl_map_object_from_fd did not clean every
> potentially allocated resource up.
> 
> Handle l_phdr, l_libname and mapped segments in the common failure
> handling code.
> 
> There are various bits that may not be cleaned properly on failure
> (e.g. executable stack, incomplete dl_map_segments) fixing those
> need further changes.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>


> ---
>  elf/dl-load.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 10ccca20d0..a9a2a1b63f 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -955,8 +955,14 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>  	  /* The file might already be closed.  */
>  	  if (fd != -1)
>  	    (void) __close_nocancel (fd);
> +	  if (l != NULL && l->l_map_start != 0)
> +	    _dl_unmap_segments (l);
>  	  if (l != NULL && l->l_origin != (char *) -1l)
>  	    free ((char *) l->l_origin);
> +	  if (l != NULL && !l->l_libname->dont_free)
> +	    free (l->l_libname);
> +	  if (l != NULL && l->l_phdr_allocated)
> +	    free ((void *) l->l_phdr);
>  	  free (l);
>  	  free (realname);
>  

Ok.

> @@ -1251,7 +1257,11 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>      errstring = _dl_map_segments (l, fd, header, type, loadcmds, nloadcmds,
>  				  maplength, has_holes, loader);
>      if (__glibc_unlikely (errstring != NULL))
> -      goto lose;
> +      {
> +	/* Mappings can be in an inconsistent state: avoid unmap.  */
> +	l->l_map_start = l->l_map_end = 0;
> +	goto lose;
> +      }
>  
>      /* Process program headers again after load segments are mapped in
>         case processing requires accessing those segments.  Scan program

Ok.

> @@ -1289,15 +1299,6 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>        || (__glibc_unlikely (l->l_flags_1 & DF_1_PIE)
>  	  && __glibc_unlikely ((mode & __RTLD_OPENEXEC) == 0)))
>      {
> -      /* We are not supposed to load this object.  Free all resources.  */
> -      _dl_unmap_segments (l);
> -
> -      if (!l->l_libname->dont_free)
> -	free (l->l_libname);
> -
> -      if (l->l_phdr_allocated)
> -	free ((void *) l->l_phdr);
> -
>        if (l->l_flags_1 & DF_1_PIE)
>  	errstring
>  	  = N_("cannot dynamically load position-independent executable");

Ok.

> @@ -1402,6 +1403,10 @@ cannot enable executable stack as shared object requires");
>    /* Signal that we closed the file.  */
>    fd = -1;
>  
> +  /* Failures before this point are handled locally via lose.
> +     There are no more failures in this function until return,
> +     to change that the cleanup handling needs to be updated.  */
> +
>    /* If this is ET_EXEC, we should have loaded it as lt_executable.  */
>    assert (type != ET_EXEC || l->l_type == lt_executable);
>  
> 

Ok.

Re: [PATCH 1/2] elf: inline lose for error handling

[Szabolcs Nagy]

The 12/14/2020 15:07, Adhemerval Zanella via Libc-alpha wrote:
> On 11/12/2020 13:35, Szabolcs Nagy via Libc-alpha wrote:
> > +	lose:
> > +	  /* The file might already be closed.  */
> > +	  if (fd != -1)
> > +	    (void) __close_nocancel (fd);
> 
> I think you can remove the (void) cast here.	

hm yes i will remove that void. there seem to be

  __close_nocancel

and

  __close_nocancel_nostatus

as well with the only diffeence that the latter has void return.

i think this can be cleaned up to only use __close_nocancel.

Re: [PATCH 1/2] elf: inline lose for error handling

[Florian Weimer]

* Szabolcs Nagy via Libc-alpha:

> The 12/14/2020 15:07, Adhemerval Zanella via Libc-alpha wrote:
>> On 11/12/2020 13:35, Szabolcs Nagy via Libc-alpha wrote:
>> > +	lose:
>> > +	  /* The file might already be closed.  */
>> > +	  if (fd != -1)
>> > +	    (void) __close_nocancel (fd);
>> 
>> I think you can remove the (void) cast here.	
>
> hm yes i will remove that void. there seem to be
>
>   __close_nocancel
>
> and
>
>   __close_nocancel_nostatus
>
> as well with the only diffeence that the latter has void return.
>
> i think this can be cleaned up to only use __close_nocancel.

Or more precisely, remove the errno clobber from __close_nocancel and
__close_nocancel_nostatus.  Currently, the implementation does not match
its documented behavior.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill