Bug 1607044

Simpler replicator:

[root@tarox tmp]# cp /bin/echo ./echo-test
[root@tarox tmp]# chmod +s ./echo-test 
[root@tarox tmp]# ls -l ./echo-test 
-rwsr-sr-x. 1 root root 33128 Jul 21 13:12 ./echo-test
[root@tarox tmp]# eu-elfcompress -v -p -t none ./echo-test 
processing: ./echo-test
[root@tarox tmp]# ls -l ./echo-test 
-rwxr-xr-x. 1 root root 33128 Jul 21 13:13 ./echo-test

This is probably caused by the order eu-elfcompress sets the mode/owner of the file. fchown says:

       When the owner or group of an executable file are changed by an unpriv‐
       ileged user the S_ISUID and S_ISGID mode bits are cleared.  POSIX  does
       not specify whether this also should happen when root does the chown();
       the Linux behavior depends on the kernel version.  In case  of  a  non-
       group-executable  file (i.e., one for which the S_IXGRP bit is not set)
       the S_ISGID bit indicates mandatory locking, and is not  cleared  by  a
       chown().

and fchmod says:

       If  the  calling  process  is  not privileged (Linux: does not have the
       CAP_FSETID capability), and the group of the file does  not  match  the
       effective  group  ID  of  the process or one of its supplementary group
       IDs, the S_ISGID bit will be turned off, but this  will  not  cause  an
       error to be returned.

So even with eu-elfcompress -v you won't see any errors, but the permission bit might be dropped anyway.

This patch solves the issue by swapping the fchmod and fchown calls:

diff --git a/src/elfcompress.c b/src/elfcompress.c
index bdb0e3b..1a0f984 100644
--- a/src/elfcompress.c
+++ b/src/elfcompress.c
@@ -1235,13 +1235,16 @@ process_file (const char *fname)
   elf_end (elfnew);
   elfnew = NULL;
 
-  /* Try to match mode and owner.group of the original file.  */
-  if (fchmod (fdnew, st.st_mode & ALLPERMS) != 0)
-    if (verbose >= 0)
-      error (0, errno, "Couldn't fchmod %s", fnew);
+  /* Try to match mode and owner.group of the original file.
+     Note to set suid bits we have to make sure the owner is setup
+     correctly first. Otherwise fchmod will drop them silently
+     or fchown may clear them.  */
   if (fchown (fdnew, st.st_uid, st.st_gid) != 0)
     if (verbose >= 0)
       error (0, errno, "Couldn't fchown %s", fnew);
+  if (fchmod (fdnew, st.st_mode & ALLPERMS) != 0)
+    if (verbose >= 0)
+      error (0, errno, "Couldn't fchmod %s", fnew);
 
   /* Finally replace the old file with the new file.  */
   if (foutput == NULL)

Why does it even fchmod()/fchown()?

why not to do something like stat() + chmod()?

(In reply to Igor Gnatenko from comment #2)
> Why does it even fchmod()/fchown()?

To make sure the new file has the same mode and owner as the original.

(In reply to Igor Gnatenko from comment #3)
> why not to do something like stat() + chmod()?

That is what it does, it stats the original file and then fchown/fchmod the new file.

This should be fixed by:

commit 15973e2b9bab41a0851627863f37a877851f5610
Author: Mark Wielaard <mjw>
Date:   Sat Jul 21 17:20:57 2018 +0200

    0.173-4 - Add elfutils-0.173-elfcompress.patch (#1607044)

But also needs:

commit 896e01dee76f9a08594e75ecda324fc99e03fce2
Author: Mark Wielaard <mjw>
Date:   Sat Jul 21 23:58:40 2018 +0200

    0.173-5
    
    - Add BuildRequires gcc-c++ for demangle support.
    - Add elfutils-0.173-annobingroup.patch.

It still doesn't build because of a (latent?) bug on i386 handling the new annobin ELF group support in ET_REL files in eu-unstrip:

https://koji.fedoraproject.org/koji/taskinfo?taskID=28503802

Other arches are fine, but this doesn't really look like it should be arch specific.

It needed another patch to eu-unstrip:

commit 47092416243d54e4cff3cd2558ece8b93695d54e
Author: Mark Wielaard <mark>
Date:   Tue Jul 24 23:34:19 2018 +0200

    unstrip: Also check sh_size in compare_unalloc_sections.
    
    compare_unalloc_sections only checked sh_flags and the section names.
    This would cause stripped/debug section mismatches when there were
    multiple sections with the same name and flags. Fix this by also checking
    the size of the section matches.
    
    Add a testcase that has two ".group" sections created on i386 with the
    gcc annobin plugin.
    
    Signed-off-by: Mark Wielaard <mark>

Added to elfutils-0.173-6.fc29.