On Sun, Jul 14, 2013 at 08:22:45PM +0200, J??r??mie Courr??ges-Anglas wrote:
> Kenneth R Westerback <kwesterb...@rogers.com> writes:
>
> > On Sun, Jul 14, 2013 at 05:56:46PM +0200, J??r??mie Courr??ges-Anglas wrote:
> >> Kenneth R Westerback <kwesterb...@rogers.com> writes:
> >>
> >> > On Sun, Jul 14, 2013 at 03:13:32PM +0200, J??r??mie Courr??ges-Anglas
> >> > wrote:
> >> >> Kenneth R Westerback <kwesterb...@rogers.com> writes:
> >> >>
> >> >> > On Sun, Jul 14, 2013 at 09:23:53AM +0200, J??r??mie Courr??ges-Anglas
> >> >> > wrote:
> >> >> >> David Hill <dh...@mindcry.org> writes:
> >> >> >>
> >> >> >> > remove unused variables.
> >> >> >>
> >> >> >> Makes sense. ok?
> >> >> >>
> >> >>
> >> >> [...]
> >> >>
> >> >> >> > lseek(fd, -((off_t)sizeof(struct prebind_footer)), SEEK_END);
> >> >> >> > - len = read(fd, &footer, sizeof(struct prebind_footer));
> >> >> >> > + read(fd, &footer, sizeof(struct prebind_footer));
> >> >> >
> >> >> > Here I would consider actually using len to check for failure. And of
> >> >> > course changing the type of len to ssize_t to allow such checking.
> >> >> >
> >> >> > .... Ken
> >> >>
> >> >> Sure (assuming that an undetected lseek() error would be caught by
> >> >> read()).
> >> >
> >> > I guess lseek() should also have a result check, as the rabbit hole
> >> > yawns wider. :-)
> >>
> >> So let's do that.
> >>
> >> > But to concentrate on the read, I think the other
> >> > error to check for is a short read. But not being a ld.so hacker I
> >> > have no feel for how much trouble could be caused by only reading
> >> > in a partial object. I'm guessing the file would have to be truly
> >> > pathological.
> >>
> >> I wondered about that case too, but got lazy. Turns out footer is 80
> >> bytes, while black box testing shows that this code path isn't hit
> >> for files "invalid" - not looking like ELF enough or smaller than 475
> >> bytes (on my i386 machine).
>
> Hmm, I should have added that feeding it a valid executable truncated to
> 475 bytes or more produces reproducible segfaults. O:)
>
> >> In any case, I don't think a check hurts much.
> >> (elf_sum_reloc() ansified while here, must stop adding stuff over diffs...)
> >>
> >> Index: prebind.c
> >> ===================================================================
> >> RCS file: /cvs/src/libexec/ld.so/ldconfig/prebind.c,v
> >> retrieving revision 1.21
> >> diff -u -p -p -u -r1.21 prebind.c
> >> --- prebind.c 5 Jul 2013 21:10:50 -0000 1.21
> >> +++ prebind.c 14 Jul 2013 15:50:24 -0000
> >> @@ -152,7 +152,7 @@ struct elf_object * elf_lookup_object_de
> >> void elf_free_curbin_list(struct elf_object *obj);
> >> void elf_resolve_curbin(void);
> >> struct proglist *elf_newbin(void);
> >> -void elf_sum_reloc();
> >> +void elf_sum_reloc(void);
> >> int elf_prep_lib_prebind(struct elf_object *object);
> >> int elf_prep_bin_prebind(struct proglist *pl);
> >> void add_fixup_prog(struct elf_object *prog, struct elf_object *obj,
> >> int idx,
> >> @@ -475,12 +475,10 @@ done:
> >> int
> >> elf_check_note(void *buf, Elf_Phdr *phdr)
> >> {
> >> - Elf_Ehdr *ehdr;
> >> u_long address;
> >> u_int *pint;
> >> char *osname;
> >>
> >> - ehdr = (Elf_Ehdr *)buf;
> >> address = phdr->p_offset;
> >> pint = (u_int *)((char *)buf + address);
> >> osname = (char *)buf + address + sizeof(*pint) * 3;
> >> @@ -1425,7 +1423,7 @@ elf_init_objarray(void)
> >> }
> >>
> >> void
> >> -elf_sum_reloc()
> >> +elf_sum_reloc(void)
> >> {
> >> int numobjs;
> >> int err = 0;
> >> @@ -1715,7 +1713,7 @@ elf_write_lib(struct elf_object *object,
> >> u_int32_t next_start, *fixuptab = NULL;
> >> struct stat ifstat;
> >> off_t base_offset;
> >> - size_t len;
> >> + ssize_t len;
> >> int fd = -1, i;
> >> int readonly = 0;
> >>
> >> @@ -1731,8 +1729,24 @@ elf_write_lib(struct elf_object *object,
> >> }
> >> readonly = 1;
> >> }
> >> - lseek(fd, -((off_t)sizeof(struct prebind_footer)), SEEK_END);
> >> + if (lseek(fd, -((off_t)sizeof(struct prebind_footer)), SEEK_END)
> >> + == -1) {
> >> + perror(object->load_name);
> >> + close(fd);
> >> + return 1;
> >> + }
> >> +
> >> len = read(fd, &footer, sizeof(struct prebind_footer));
> >> + if (len >= -1 && len < sizeof(struct prebind_footer)) {
> >
> > I think this condition is incorrect.
>
> It checks whether len is in the [-1; 80] range. I was perhaps a bit too
> paranoid about the fact that read(2) can return a value between
> SSIZE_MAX and SIZE_MAX -2 (see CAVEATS). Since I've read this section
> I kinda wired it into my brain; I don't even know if that applies to
> OpenBSD.
>
> > if (len == -1 || len < sizeof())
>
> Of course here the maximum (and desired) return value is 80 so we won't
> hit that problem. Maybe a clearer check would be:
>
> if (len != sizeof()) {
> if (len == -1)
> /* handle error */
> else
> /* handle short read */
> }
>
> which I find clearer.
>
> Does this look reasonable?
Works for me. ok krw@
.... Ken
>
> Index: prebind.c
> ===================================================================
> RCS file: /cvs/src/libexec/ld.so/ldconfig/prebind.c,v
> retrieving revision 1.21
> diff -u -p -p -u -r1.21 prebind.c
> --- prebind.c 5 Jul 2013 21:10:50 -0000 1.21
> +++ prebind.c 14 Jul 2013 18:18:53 -0000
> @@ -152,7 +152,7 @@ struct elf_object * elf_lookup_object_de
> void elf_free_curbin_list(struct elf_object *obj);
> void elf_resolve_curbin(void);
> struct proglist *elf_newbin(void);
> -void elf_sum_reloc();
> +void elf_sum_reloc(void);
> int elf_prep_lib_prebind(struct elf_object *object);
> int elf_prep_bin_prebind(struct proglist *pl);
> void add_fixup_prog(struct elf_object *prog, struct elf_object *obj, int idx,
> @@ -475,12 +475,10 @@ done:
> int
> elf_check_note(void *buf, Elf_Phdr *phdr)
> {
> - Elf_Ehdr *ehdr;
> u_long address;
> u_int *pint;
> char *osname;
>
> - ehdr = (Elf_Ehdr *)buf;
> address = phdr->p_offset;
> pint = (u_int *)((char *)buf + address);
> osname = (char *)buf + address + sizeof(*pint) * 3;
> @@ -1425,7 +1423,7 @@ elf_init_objarray(void)
> }
>
> void
> -elf_sum_reloc()
> +elf_sum_reloc(void)
> {
> int numobjs;
> int err = 0;
> @@ -1715,7 +1713,7 @@ elf_write_lib(struct elf_object *object,
> u_int32_t next_start, *fixuptab = NULL;
> struct stat ifstat;
> off_t base_offset;
> - size_t len;
> + ssize_t len;
> int fd = -1, i;
> int readonly = 0;
>
> @@ -1731,8 +1729,24 @@ elf_write_lib(struct elf_object *object,
> }
> readonly = 1;
> }
> - lseek(fd, -((off_t)sizeof(struct prebind_footer)), SEEK_END);
> + if (lseek(fd, -((off_t)sizeof(struct prebind_footer)), SEEK_END)
> + == -1) {
> + perror(object->load_name);
> + close(fd);
> + return 1;
> + }
> +
> len = read(fd, &footer, sizeof(struct prebind_footer));
> + if (len != sizeof(struct prebind_footer)) {
> + close(fd);
> + if (len == -1)
> + perror(object->load_name);
> + else
> + /* paranoia */
> + warnx("%s on %s: short read (corrupted file?)",
> + __func__, object->load_name);
> + return 1;
> + }
>
> if (fstat(fd, &ifstat) == -1) {
> perror(object->load_name);
> @@ -2213,7 +2227,6 @@ void
> copy_oldsymcache(int objidx, void *prebind_map)
> {
> struct prebind_footer *footer;
> - struct elf_object *object;
> struct elf_object *tobj;
> struct symcache_noflag *tcache;
> struct symcachetab *symcache;
> @@ -2222,8 +2235,6 @@ copy_oldsymcache(int objidx, void *prebi
> u_int32_t offset;
> u_int32_t *poffset;
> struct nameidx *nameidx;
> -
> - object = objarray[objidx].obj;
>
> poffset = (u_int32_t *)prebind_map;
> c = prebind_map;
