diff mbox series

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

Message ID 98caa13064bb554288e3e2984bc11b81ed3d7393.1607703178.git.szabolcs.nagy@arm.com
State Committed
Delegated to: Adhemerval Zanella Netto
Headers
Series elf: dl-load error handling refactoring |

Commit Message

Szabolcs Nagy Dec. 11, 2020, 4:35 p.m. UTC 
  _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(-)

Comments

Adhemerval Zanella Netto Dec. 14, 2020, 6:07 p.m. UTC | #1 
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.
Szabolcs Nagy Dec. 15, 2020, 10:04 a.m. UTC | #2 
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.
Florian Weimer Dec. 15, 2020, 10:41 a.m. UTC | #3 
* 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
diff mbox series

Patch

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