This is the mail archive of the elfutils-devel@sourceware.org mailing list for the elfutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [Patch] Don't relocate compressed sections


On Wed, Mar 28, 2012 at 02:54:49PM -0700, Roland McGrath wrote:
> > On Wed, Mar 28, 2012 at 01:36:26PM -0700, Roland McGrath wrote:
> > > As mentioned before, if we merge and finish up the relocate branch
> > > then the problem goes away.
> > 
> > But we currently don't have an estimate for when this will happen.
> 
> IIRC it was pretty well ready when I left it.  I just merged the trunk
> into the branch.  It still builds except for strip.c where you copied
> some of the libdwfl/relocate.c code.  That needs to get the
> corresponding updates to use ebl_reloc_simple_types instead of
> ebl_reloc_simple_type.  Do you want to make those fixes on the branch?

Thanks for that. I'll take a look.

> I think the main barrier was wanting some more consideration on the new
> API additions (look at 'git diff master..roland/relocate -- libdw.h')
> before we enshrined them in the permanent libdw ABI.  So one option is
> just to decide we think they are good enough as they are.

OK. I'll start a new review thread for that. I admit I didn't know it
was already that close.
 
> Another option is to do an intermediate version of the branch with all
> the internals changes, but without making any new interfaces public.
> Then libdwfl-based uses like systemtap's get the (presumed, measurable)
> performance benefits of reloc-on-demand and the compressed-section
> problem goes away for libdwfl.  But we don't commit to any new APIs or
> ABIs.

I can try it against some systemtap testcases to see how it goes.

> > Until we do finish that I like to not apply relocations to compressed
> > sections, since we know that just doesn't work (and the corruption might
> > not even be detected if we do). That is really all the patch does.
> 
> I have several concerns here.  Even if we did just that, then I thought
> the patch looked ugly enough that it can probably be done in a way
> somewhat cleaner than that.  But that's just the trivial concern.

Since I thought the patch was as clean as I could come up with please
do let me know what you thought was the ugly part. I might have to
adjust my "beautiful code" filter :) The patch really was minimal
IMHO:

diff --git a/libdwfl/relocate.c b/libdwfl/relocate.c
index 95206f4..effef44 100644
--- a/libdwfl/relocate.c
+++ b/libdwfl/relocate.c
@@ -319,6 +319,12 @@ relocate_section (Dwfl_Module *mod, Elf *relocated, const GElf_Ehdr *ehdr,
   if (tdata == NULL)
     return DWFL_E_LIBELF;
 
+  if (tname[0] == '.' && tname[1] == 'z' && tdata->d_size >= 4 + 8
+      && memcmp (tdata->d_buf, "ZLIB", 4) == 0)
+    /* Compressed section can only be relocated after decompression,
+       don't touch it or we will corrupt it. */
+    return DWFL_E_NOERROR;
+
   /* Apply one relocation.  Returns true for any invalid data.  */
   Dwfl_Error relocate (GElf_Addr offset, const GElf_Sxword *addend,
                       int rtype, int symndx)


> It concerns me more that we would be papering over our lack of thorough
> support for .zdebug_* sections while making it appear that we might
> handle them.  Giving such a false impression might be worse than an
> "obviously broken" state.

OK, I was more concerned with the possibility of silently corrupting
the sections libdwfl prepared.
 
> The support I added in commit 725aad5d was pretty minimal and I surely
> overlooked multiple subtleties at the time.  (We weren't near a release
> at the time, and I probably figured we'd have more close attention
> before the next release than we actually had in the year--exactly one
> year, it so happens--that we had from then until 0.153.)  The libdwfl
> interaction for ET_REL that's been noticed here is just one.  Another
> that pops to mind now is that ebl_debugscn_p doesn't match .zdebug_*
> sections.  What effects does that have?

Some strip variants would get confused, I can write some tests for that.
Funnily relocate () has a debug flag which checks ebl_debugscn_p and doesn't
relocate if the target isn't a ebl_debugscn_p (). But we never use that
flag.

Cheers,

Mark

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]