On 04/05/21(Tue) 21:41, Klemens Nanni wrote:
> On Thu, Apr 15, 2021 at 03:05:45PM +0200, Mark Kettenis wrote:
> > > > [...]
> > > > Hence, only access buckets in _dl_find_symbol_obj() if there are any;
> > > > this fixes the crash and in fact allows me to play the song even when
> > > > preloading the currupted library, i.e.
> > > >
> > > > $ LD_PRELOAD=./libvorbisenc.so.3.1 ogg123 song62.ogg
> > > >
> > > > now also works with patched ld.so installed -- I'd expected ld.so,
> > > > libvorbis or ogg123 to crash on some other place...
> > > >
> > > > I'm not sure what to make of this, I also don't know enough about ld.so
> > > > to judge this diff in context, it does however fix an obvious error.
> > > > FWIW, regress/libexec/ld.so runs fine with this diff.
> > >
> > > I'm not sure if silently ignoring the corruption is the best way to go.
> >
> > It certainly isn't. If corruption is detected, the prcess should
> > terminate immedtaley.
>
> I totally agree, mention of regress passing shouldn't imply that I want
> that diff in, but rather show that it didn't have unexpected drawbacks.
>
> > > Do you know why `nbuckets' and `buckets_elf' aren't initialized for this
> > > object? Do you know if _dl_finalize_object() has been call for it?
>
> Yes, _dl_finalize_object() is always called for it.
>
> I compared my corrupted shared library with an intact copy from ports
> and it showed that the corrupted one was simply zeroed out at some point
> (not truncated) until the end, e.g. readelf(1)'s `-h' or `-l' report
> "Error: no .dynamic section in the dynamic segment".
>
> So this isn't a case of some badly linked library or one that has a few
> bits flipped, it's simply a partial one... seems like bad luck?
>
> > > > Is this a code path that can happen with intact objects?
> > > > Given that the file is obviously corrupted but programs using it still
> > > > (partially) work, should a warning be printed in this case?
> > >
> > > Indicating that the library is corrupted might indeed be better than
> > > crashing. However it isn't clear to me where such check should happen.
>
> I've done just that now as there's nothing else to do. It is an obscure
> case that I cannot explain without corruption, so very unlikely to
> happen, but now it did...
>
> > > > Index: resolve.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/libexec/ld.so/resolve.c,v
> > > > retrieving revision 1.94
> > > > diff -u -p -r1.94 resolve.c
> > > > --- resolve.c 4 Oct 2019 17:42:16 -0000 1.94
> > > > +++ resolve.c 14 Apr 2021 15:56:14 -0000
> > > > @@ -608,7 +608,7 @@ _dl_find_symbol_obj(elf_object_t *obj, s
> > > > return r > 0;
> > > > }
> > > > } while ((*hashval++ & 1U) == 0);
> > > > - } else {
> > > > + } else if (obj->nbuckets > 0) {
> > > > Elf_Word si;
> > > >
> > > > for (si = obj->buckets_elf[sl->sl_elf_hash %
> > > > obj->nbuckets];
> > > >
> > >
> > >
>
> readelf(1) detects this corruption as missing (or empty/zeroed out as
> code reading showed); we could probably to that as well but it'd be
> less trivial and a generalisation of my issue.
>
> So the new diff simply bails out if there is no symbol hash table, which
> is equally relevant for both ELF and GNU hashes:
>
> $ LD_PRELOAD=./bad.so ./ogg123 ~/song62.ogg
> ld.so: ogg123.test: ./bad.so: no buckets
> killed
> $
>
> Feedback? Objections? OK?
I still don't understand what the corruption is and the check below
doesn't explain that either. So if I'm developing a library and I see
such message it doesn't give me more info than the previous core dump.
What is corrupted? The header? A section is missing? An offset is
incorrect? Is there a mismatch between DT_GNU_HASH and DT_HASH?
> Index: resolve.c
> ===================================================================
> RCS file: /cvs/src/libexec/ld.so/resolve.c,v
> retrieving revision 1.94
> diff -u -p -r1.94 resolve.c
> --- resolve.c 4 Oct 2019 17:42:16 -0000 1.94
> +++ resolve.c 29 Apr 2021 22:07:46 -0000
> @@ -573,6 +573,9 @@ _dl_find_symbol_obj(elf_object_t *obj, s
> {
> const Elf_Sym *symt = obj->dyn.symtab;
>
> + if (obj->nbuckets == 0)
> + _dl_die("%s: no buckets", obj->load_name);
> +
> if (obj->status & STAT_GNU_HASH) {
> uint32_t hash = sl->sl_gnu_hash;
> Elf_Addr bloom_word;
