Message ID | 98caa13064bb554288e3e2984bc11b81ed3d7393.1607703178.git.szabolcs.nagy@arm.com |
---|---|
State | New |
Headers | show |
Series | elf: dl-load error handling refactoring | expand |
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.
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.
* 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 --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; } }