Hi, On Wed, Jul 7, 2021 at 5:02 AM Mark Wielaard <m...@klomp.org> wrote:
> Hi Andrei, > > On Mon, 2021-06-28 at 18:26 -0700, Andrei Homescu wrote: > > The mkl_memory_patched.o object inside the libmkl_core.a library from > > the Intel Math Kernel Library version 2018.2.199 has this section > > with an alignment of 4096 and offset of 0xb68: > > [ 2] .data PROGBITS 0000000000000000 000b68 011000 00 WA 0 0 4096 > > > > Reading this file with libelf and trying to write it back to disk > triggers > > the following sequence of events: > > 1) code in elf_getdata.c clamps d_align for this section's data buffer > > to the section's offset > > 2) code in elf32_updatenull.c checks if the alignment is a power of two > > and incorrectly returns an error > > > > This commit fixes this corner case by increasing the alignment to the > > next power of two after the clamping, so the check passes. > > > > A test that reproduces this bug using strip is also included. > > Thanks for this perfect patch, including and excellent explanation of > the issue and a testcase. I have only a minor suggestion: > > > --- /dev/null > > +++ b/tests/run-strip-largealign.sh > > @@ -0,0 +1,34 @@ > > +#! /bin/sh > > +# Copyright (C) 2011 TODO. > > Should we make this Copyright (C) 2021 Andrei Homescu <a...@immunant.com> > or Copyright (C) 2021 Immunant, Inc. > Sorry about that, I intended to resolve that TODO myself and then missed it. I wrote this patch on behalf of someone else, so the copyright line should be: Copyright (C) 2021 Runsafe Security, Inc. > > +# This file is part of elfutils. > > +# > > +# This file is free software; you can redistribute it and/or modify > > +# it under the terms of the GNU General Public License as published > > by > > +# the Free Software Foundation; either version 3 of the License, or > > +# (at your option) any later version. > > +# > > +# elfutils is distributed in the hope that it will be useful, but > > +# WITHOUT ANY WARRANTY; without even the implied warranty of > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > +# GNU General Public License for more details. > > +# > > +# You should have received a copy of the GNU General Public License > > +# along with this program. If not, see < > > http://www.gnu.org/licenses/>. > > +# > > + > > +. $srcdir/test-subr.sh > > + > > +# = testfile-largealign.S = > > +# section .data > > +# align 4096 > > +# dd 0x12345678 > > +# > > +# nasm -f elf64 -o testfile-largealign.o testfile-largealign.S > > + > > +infile=testfile-largealign.o > > +outfile=$infile.stripped > > + > > +testfiles $infile > > +tempfiles $outfile > > + > > +testrun ${abs_top_builddir}/src/strip -o $outfile $infile > > The testcase already fails before the patch and succeeds after, but it > would be nice to double check the output with elflint just in case. > Shall we add: > > testrun ${abs_top_builddir}/src/elflint --gnu $outfile > Sounds good, no objection from me. Should I submit an updated version of the patch, or can you add this on your end? Andrei
