Re: strip vs elf section groups

[Mark Wielaard <mjw at redhat dot com>]

On Wed, 2011-05-18 at 09:44 -0700, Roland McGrath wrote:
> unstrip on ET_REL files is really only intended to work for the limited
> case of .ko-style files, so this may not really be a practical problem.
> unstrip always has to be a bit fuzzy (hence the by-name matching), so I'm
> not sure it can really solve this problem.

It would need to match on the name of the signature symbol of SHT_GROUP
sections, which is much more work than is currently done in unstrip
section matching. Sadly we do hit this even with .ko-style files when
using -gdwarf-4 which outputs .debug_types as COMDAT group sections. It
is nice to have this working for roundtripping to catch any mistakes in
strip/unstrip. But I don't think there is a real case for it, so I'll
put fixing it somewhere down the list for now.

> > +	      /* If a group section is marked as being removed make
> > +		 sure all the sections it contains are being remove, too.  */
> Typo:								   ^d

Thanks, fixed.

> > +	      if (shdr_info[cnt].shdr.sh_type == SHT_GROUP)
> > +		{
> > +		  Elf32_Word *grpref;
> > +		  grpref = (Elf32_Word *) shdr_info[cnt].data->d_buf;
> 
> This is a getdata buffer, right?  i.e. it's been byte-swapped if needed.

Yes it is. I did do a quick test against a ppc elf file just to be sure.

> > +		  for (size_t in = 1;
> > +		       in < shdr_info[cnt].data->d_size / sizeof (Elf32_Word);
> > +		       ++in)
> > +		    if (shdr_info[grpref[in]].idx != 0)
> 
> At least for the robustify branch (and IMHO might as well do it in trunk)
> this should check for a bogon in GRPREF rather than using a wild index.

Yeah, the robustify branch actually caught some issues while I hacked on
this. But it wasn't as useful as I hoped. The checks all just chain to
"invalid file", which isn't much more help than a simple crash/assert.
But I'll add it to the robustify branch when I merge to it.

> > @@ -883,6 +899,7 @@ handle_elf (int fd, Elf *elf, const char *prefix, const char *fname,
> >  	  bool discard_section = (shdr_info[cnt].idx > 0
> >  				  && shdr_info[cnt].debug_data == NULL
> >  				  && shdr_info[cnt].shdr.sh_type != SHT_NOTE
> > +				  && shdr_info[cnt].shdr.sh_type != SHT_GROUP
> >  				  && cnt != ehdr->e_shstrndx);
> 
> I'm not clear on all the control logic without reading through it all again.
> But does this prevent it ever removing SHT_GROUP sections?  They ought to
> be removed iff all their constituents were removed.

Yes, that is still the case. This check is just so (a little down) we
copy the original content (with the original section numbers of the
group) when we store it in the debug file in case the section group gets
rewritten (with renumbered sections) in the stripped file.

Thanks for the review,

Mark