This is the mail archive of the
elfutils-devel@sourceware.org
mailing list for the elfutils project.
Re: [Patch] Don't relocate compressed sections
- From: Mark Wielaard <mjw at redhat dot com>
- To: elfutils-devel at lists dot fedorahosted dot org
- Date: Thu, 29 Mar 2012 21:46:25 +0200
- Subject: 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