Re: [Rpm-maint] [PATCH 3/4] Add sepdebugcrcfix to fixup old style gnu_debuglink CRC checksum.

2016-06-14 Thread Mark Wielaard 
Hi Thierry,

On Mon, 2016-06-13 at 14:33 +0200, Thierry Vignaud wrote:
> Could you provide a small example? From the bug report it isn't clear if
> > it really is a bug with that particular fix or that it is caused by bad
> > usage of attributes. If we have a small example we can create a test
> > case for it.
> 
> Here's one example attached.
> With current FC patch applied to rpm (aka uncommenting the patch at
> http://svnweb.mageia.org/packages/cauldron/rpm/current/SPECS/rpm.spec?revision=1018103=markup#l116)
> 
> With "%define debug_package %{nil}", "ll /bin/test2" shows:
> -rwsr-xr-x 1 root root 6128 Eve  13 13:56 /bin/test2*
> With "#define debug_package %{nil}", "ll /bin/test2" shows:
> -rwxr-xr-x 1 root root 7184 Eve  13 13:55 /bin/test2*
> 
> When the rpm patch is not applied, both cases works OK (aka ls -[ol]
> reports the suid bit)
 
Aha, ok. I see what is happening. Since you don't set the attributes
explicitly you rely on the suid bit being picked up. But since we change
the file the suid flag gets reset. The fix is similar to how other helpers
get and reset all mods. I added a testcase for this issue making sure that
it triggers all helpers and still has the suid binaries coming out as
expected. I also rearranged the patchset a little, to make sure the test
actually tests the right binary by bringing forward the "Don't use
hardcoded paths to tools/scripts in find-debuginfo.sh." fix.

 [PATCH 1/7] Add find-debuginfo.sh -m minisymtab support.
 [PATCH 2/7] Add dwz debuginfo compression support.
 [PATCH 3/7] Don't use hardcoded paths to tools/scripts in
 [PATCH 4/7] Add sepdebugcrcfix to fixup old style gnu_debuglink CRC
 [PATCH 5/7] Add build-id links to rpm for all ELF files.
 [PATCH 6/7] Make it possible to have unique build-ids across build
 [PATCH 7/7] Make adding GDB index sections configurable.

Thanks,

Mark
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [PATCH 3/4] Add sepdebugcrcfix to fixup old style gnu_debuglink CRC checksum.

2016-06-07 Thread Mark Wielaard 
On Tue, 2016-06-07 at 10:33 +0200, Thierry Vignaud wrote:
> On 6 June 2016 at 23:00, Mark Wielaard  wrote:
> > Some old tools might still use the .gnu_debuglink section to find
> > separate debuginfo files instead of build-id style lookups. When
> > dwz has compresses the .debug files the original CRC in the main
> > ELF file will no longer match. Make sure to run sepdebugcrcfix
> > after dwz to recalculate the CRC.
> >
> > The original fix was created by Jan Kratochvil based on code
> > from GNU binutils BFD. https://bugzilla.redhat.com/show_bug.cgi?id=971119
> > I added a testcase to make sure the CRCs were all correctly
> > updated after dwz has run to compress a debuginfo package.
> 
> WARNING: Note that the original Fedora patch has issues!
> Panu (the then upstream rpm.org maintainer) said it should _not_ be
> upstreamed when we bring the issue up:
> 
> Once we applied this patch to rpm-4.12 in Mageia, rpm silently dropped
> the suid/sgid bits from files if they were re not explicitely listed with 
> %attr
> This was breaking packages...
> See https://bugs.mageia.org/show_bug.cgi?id=14691

Could you provide a small example? From the bug report it isn't clear if
it really is a bug with that particular fix or that it is caused by bad
usage of attributes. If we have a small example we can create a test
case for it.

> Panu answered:
> 
> == BEGIN
> As for the "are you sure we (still) need this patch" question, the
> answer has always been "almost certainly not".
> 
> The short summary is that the issue it fixes only is present if you
> use dwz to compress the debuginfo (like Fedora does), and even then
> case it only affects a handful of toolchain developers in the entire
> world.
> 
> See https://bugzilla.redhat.com/show_bug.cgi?id=971119 for further
> explanation of the issue, comment #9 answers the who is affected part.
> 
> Personally my opinion is "just kick out the stupid patch already" :)
> but judge for yourselves.
> == END

I am missing some context here. But it reads like if you don't use dwz
support then it doesn't fix any real issue. So, then you can drop that
patch also. Which is true.

But you do seem to use compressed debuginfo packages. In which case it
does fix a real issue. The issue is that if you create .debug files
with .gnu_debuglink support then you need to make sure that the CRC is
calculated correctly.  I included a testcase to show what that issue is
(the testcase will fail without the patch applied).

Also note that one of the goals of improving and cleaning up debuginfo
packages is so more people can easily use them, in which case it will
impact more than just a few hackers that manage to get them properly
installed.

You can certainly argue that all tools should use build-id based
identification and lookup and so we should just completely drop .debug
files and .gnu_debuglink entirely. But that seems a rather big change. I
don't know of any distro who did this or anybody who audited all the
tools and processes to make sure .debug files and .gnu_debuglink
(including correct CRCs) are unnecessary in all situations.

Cheers,

Mark
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [PATCH 3/4] Add sepdebugcrcfix to fixup old style gnu_debuglink CRC checksum.

2016-06-07 Thread Thierry Vignaud 
On 6 June 2016 at 23:00, Mark Wielaard  wrote:
> Some old tools might still use the .gnu_debuglink section to find
> separate debuginfo files instead of build-id style lookups. When
> dwz has compresses the .debug files the original CRC in the main
> ELF file will no longer match. Make sure to run sepdebugcrcfix
> after dwz to recalculate the CRC.
>
> The original fix was created by Jan Kratochvil based on code
> from GNU binutils BFD. https://bugzilla.redhat.com/show_bug.cgi?id=971119
> I added a testcase to make sure the CRCs were all correctly
> updated after dwz has run to compress a debuginfo package.

WARNING: Note that the original Fedora patch has issues!
Panu (the then upstream rpm.org maintainer) said it should _not_ be
upstreamed when we bring the issue up:

Once we applied this patch to rpm-4.12 in Mageia, rpm silently dropped
the suid/sgid bits from files if they were re not explicitely listed with %attr
This was breaking packages...
See https://bugs.mageia.org/show_bug.cgi?id=14691

We tracked it down to:
rpm-4.11.1-sepdebugcrcfix.patch
("fix .gnu_debuglink CRC32 after dwz (rhbz#971119)")

See https://bugs.mageia.org/show_bug.cgi?id=14691#c4


Panu answered:

== BEGIN
As for the "are you sure we (still) need this patch" question, the
answer has always been "almost certainly not".

The short summary is that the issue it fixes only is present if you
use dwz to compress the debuginfo (like Fedora does), and even then
case it only affects a handful of toolchain developers in the entire
world.

See https://bugzilla.redhat.com/show_bug.cgi?id=971119 for further
explanation of the issue, comment #9 answers the who is affected part.

Personally my opinion is "just kick out the stupid patch already" :)
but judge for yourselves.
== END

Florian, you should still have the details in your mailbox ("rpm
silently loosing suid/sgid bits" private discussion in 2014
December/2015 February between me, Panu & you)

This shows I should have used the list instead of using private mails :-(

So I would say to _not_ merge this patch.
It should remain in Fedora's rpm, not be upstreamed.
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

[Rpm-maint] [PATCH 3/4] Add sepdebugcrcfix to fixup old style gnu_debuglink CRC checksum.

2016-06-06 Thread Mark Wielaard 
Some old tools might still use the .gnu_debuglink section to find
separate debuginfo files instead of build-id style lookups. When
dwz has compresses the .debug files the original CRC in the main
ELF file will no longer match. Make sure to run sepdebugcrcfix
after dwz to recalculate the CRC.

The original fix was created by Jan Kratochvil based on code
from GNU binutils BFD. https://bugzilla.redhat.com/show_bug.cgi?id=971119
I added a testcase to make sure the CRCs were all correctly
updated after dwz has run to compress a debuginfo package.

Signed-off-by: Mark Wielaard 
---
 Makefile.am   |   4 +
 scripts/find-debuginfo.sh |   9 ++
 tests/rpmbuild.at |  36 +
 tools/sepdebugcrcfix.c| 344 ++
 4 files changed, 393 insertions(+)
 create mode 100644 tools/sepdebugcrcfix.c

diff --git a/Makefile.am b/Makefile.am
index 157e79d..08d8c8e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -170,6 +170,10 @@ rpmlibexec_PROGRAMS += elfdeps
 elfdeps_SOURCES =  tools/elfdeps.c
 elfdeps_LDADD =rpmio/librpmio.la
 elfdeps_LDADD +=   @WITH_LIBELF_LIB@ @WITH_POPT_LIB@
+
+rpmlibexec_PROGRAMS += sepdebugcrcfix
+sepdebugcrcfix_SOURCES = tools/sepdebugcrcfix.c
+sepdebugcrcfix_LDADD = @WITH_LIBELF_LIB@
 endif
 endif
 
diff --git a/scripts/find-debuginfo.sh b/scripts/find-debuginfo.sh
index 8de7bad..c5d3ce6 100644
--- a/scripts/find-debuginfo.sh
+++ b/scripts/find-debuginfo.sh
@@ -114,10 +114,12 @@ done
 LISTFILE="$BUILDDIR/$out"
 SOURCEFILE="$BUILDDIR/debugsources.list"
 LINKSFILE="$BUILDDIR/debuglinks.list"
+ELFBINSFILE="$BUILDDIR/elfbins.list"
 
 > "$SOURCEFILE"
 > "$LISTFILE"
 > "$LINKSFILE"
+> "$ELFBINSFILE"
 
 debugdir="${RPM_BUILD_ROOT}/usr/lib/debug"
 
@@ -318,6 +320,8 @@ while read nlinks inum f; do
   # strip -g implies we have full symtab, don't add mini symtab in that case.
   $strip_g || ($include_minidebug && add_minidebug "${debugfn}" "$f")
 
+  echo "./${f#$RPM_BUILD_ROOT}" >> "$ELFBINSFILE"
+
   if [ -n "$id" ]; then
 make_id_link "$id" "$dn/$(basename $f)"
 make_id_link "$id" "/usr/lib/debug$dn/$bn" .debug
@@ -352,6 +356,11 @@ if $run_dwz && type dwz >/dev/null 2>&1 \
   [ -n "$id" ] \
&& make_id_link "$id" "/usr/lib/debug/.dwz/${dwz_multifile_name}" .debug
 fi
+
+# dwz invalidates .gnu_debuglink CRC32 in the main files.
+cat "$ELFBINSFILE" |
+(cd "$RPM_BUILD_ROOT"; \
+ xargs -d '\n' /usr/lib/rpm/sepdebugcrcfix usr/lib/debug)
   fi
 fi
 
diff --git a/tests/rpmbuild.at b/tests/rpmbuild.at
index 2fea1b6..5672279 100644
--- a/tests/rpmbuild.at
+++ b/tests/rpmbuild.at
@@ -459,3 +459,39 @@ test "$canonmultiref" = "$canonmultifile" || exit 1
 [],
 [ignore])
 AT_CLEANUP
+
+# --
+# Check that old style gnu_debuglink CRC checksums are correct even after
+# using dwz to compress the debuginfo files.
+AT_SETUP([rpmbuild debuginfo dwz gnu_debuglink crc])
+AT_KEYWORDS([build] [debuginfo])
+AT_CHECK([
+rm -rf ${TOPDIR}
+AS_MKDIR_P(${TOPDIR}/SOURCES)
+
+# Build a package that
+cp "${abs_srcdir}"/data/SOURCES/hello-1.0.tar.gz 
"${abs_srcdir}"/data/SOURCES/hello-1.0-modernize.patch ${TOPDIR}/SOURCES
+
+run rpmbuild --quiet \
+  
--macros=${abs_top_builddir}/macros:${abs_top_builddir}/tests/testing/usr/local/lib/rpm/platform/%{_target_cpu}-%{_target_os}/macros:${top_srcdir}/macros.debug
 \
+  --rcfile=${abs_top_builddir}/rpmrc \
+  -ba "${abs_srcdir}"/data/SPECS/hello2.spec
+
+# Unpack the main and debuginfo rpms so we can check binaries and .debug files.
+rpm2cpio ${abs_builddir}/testing/build/RPMS/*/hello2-debuginfo-1.0-1.*.rpm \
+  | cpio -diu
+rpm2cpio ${abs_builddir}/testing/build/RPMS/*/hello2-1.0-1.*.rpm \
+  | cpio -diu
+
+# Check that dwz has ran and a multi file has been produced
+test -f ./usr/lib/debug/.dwz/hello2-1.0-1.* || exit 1
+
+# Run sepdbugcrcfix on the binaries, both should have correct CRC already.
+${abs_top_builddir}/sepdebugcrcfix ./usr/lib/debug \
+  ./usr/local/bin/hello ./usr/local/bin/hello2 | grep CRC32 | cut -f2 -d:
+],
+[0],
+[ Updated 0 CRC32s, 2 CRC32s did match.
+],
+[ignore])
+AT_CLEANUP
diff --git a/tools/sepdebugcrcfix.c b/tools/sepdebugcrcfix.c
new file mode 100644
index 000..8e45abf
--- /dev/null
+++ b/tools/sepdebugcrcfix.c
@@ -0,0 +1,344 @@
+/* Copyright (C) 2013 Free Software Foundation, Inc.
+
+   This program 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.
+
+   This program 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