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).
>
> 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.
if (len == -1 || len < sizeof())
perhaps?
.... Ken
> + 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;
>
