ar, ranlib: Call fchown before fchmod, explicitly check and ignore errors.

In ar and ranlib we don't mind if the fchown call fails (it normally
would, then the file simply gets own by the current user). We used to
call fchown before fchmod, but that might ignore (or reset) some mode
flags, so call fchown first, then try fchmod (like we already do in
elfcompress). Also explicitly test and then ignore any errors for
chown. We used to do some asm trick, but that confuses some static
analyzers (and it is somewhat unreadable). Also split out the giant
if statements to make them a little bit more understandable.

Signed-off-by: Mark Wielaard <mark@klomp.org>

diff --git a/src/ChangeLog b/src/ChangeLog
index e78bc3582afbcaa8c51137b0c638eb68a754eac1..0129b8bf39e05024318b5dd20ed08e4ed93bd18c 100644 (file)
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,11 @@
+2020-06-16  Mark Wielaard  <mark@klomp.org>
+
+       * ar.c (do_oper_extract): Split large if statement. Call fchown
+       before fchmod and explicitly ignore the return value.
+       (do_oper_delete): Likewise.
+       (do_oper_insert): Likewise.
+       * ranlib.c (handle_file): Likewise.
+
 2020-06-16  Mark Wielaard  <mark@klomp.org>
 
        * elflint.c (check_elf_header): Explicitly check and ignore

diff --git a/src/ar.c b/src/ar.c
index d70f1f467090d615320eeb8769b8710549a4b356..7d33d814d0f1ecd4a5a0aa7e0e06a70848b814c2 100644 (file)
--- a/src/ar.c
+++ b/src/ar.c
@@ -787,26 +787,30 @@ cannot rename temporary file to %.*s"),
              else
                rest_off = SARMAG;
 
-             if ((symtab.symsnamelen != 0
+             if (symtab.symsnamelen != 0
                   && ((write_retry (newfd, symtab.symsoff,
                                     symtab.symsofflen)
                        != (ssize_t) symtab.symsofflen)
                       || (write_retry (newfd, symtab.symsname,
                                        symtab.symsnamelen)
                           != (ssize_t) symtab.symsnamelen)))
-                 /* Even if the original file had content before the
-                    symbol table, we write it in the correct order.  */
-                 || (index_off != SARMAG
-                     && copy_content (elf, newfd, SARMAG, index_off - SARMAG))
-                 || copy_content (elf, newfd, rest_off, st.st_size - rest_off)
-                 /* Set the mode of the new file to the same values the
-                    original file has.  */
-                 || fchmod (newfd, st.st_mode & ALLPERMS) != 0
-                 /* Never complain about fchown failing.  */
-                 || (({asm ("" :: "r" (fchown (newfd, st.st_uid,
-                                               st.st_gid))); }),
-                     close (newfd) != 0)
-                 || (newfd = -1, rename (tmpfname, arfname) != 0))
+               goto nonew_unlink;
+             /* Even if the original file had content before the
+                symbol table, we write it in the correct order.  */
+             if ((index_off != SARMAG
+                  && copy_content (elf, newfd, SARMAG, index_off - SARMAG))
+                 || copy_content (elf, newfd, rest_off, st.st_size - rest_off))
+               goto nonew_unlink;
+
+             /* Never complain about fchown failing.  */
+             if (fchown (newfd, st.st_uid, st.st_gid) != 0) { ; }
+             /* Set the mode of the new file to the same values the
+                original file has.  */
+             if (fchmod (newfd, st.st_mode & ALLPERMS) != 0
+                 || close (newfd) != 0)
+               goto nonew_unlink;
+             newfd = -1;
+             if (rename (tmpfname, arfname) != 0)
                goto nonew_unlink;
            }
        }
@@ -1052,12 +1056,15 @@ do_oper_delete (const char *arfname, char **argv, int argc,
     }
 
   /* Set the mode of the new file to the same values the original file
-     has.  */
+     has.  Never complain about fchown failing.  But do it before
+     setting the mode (which might be reset/ignored if the owner is
+     wrong.  */
+  if (fchown (newfd, st.st_uid, st.st_gid) != 0) { ; }
   if (fchmod (newfd, st.st_mode & ALLPERMS) != 0
-      /* Never complain about fchown failing.  */
-      || (({asm ("" :: "r" (fchown (newfd, st.st_uid, st.st_gid))); }),
-         close (newfd) != 0)
-      || (newfd = -1, rename (tmpfname, arfname) != 0))
+      || close (newfd) != 0)
+    goto nonew_unlink;
+  newfd = -1;
+  if (rename (tmpfname, arfname) != 0)
     goto nonew_unlink;
 
  errout:
@@ -1534,13 +1541,19 @@ do_oper_insert (int oper, const char *arfname, char **argv, int argc,
 
   /* Set the mode of the new file to the same values the original file
      has.  */
-  if (fd != -1
-      && (fchmod (newfd, st.st_mode & ALLPERMS) != 0
-         /* Never complain about fchown failing.  */
-         || (({asm ("" :: "r" (fchown (newfd, st.st_uid, st.st_gid))); }),
-             close (newfd) != 0)
-         || (newfd = -1, rename (tmpfname, arfname) != 0)))
-      goto nonew_unlink;
+  if (fd != -1)
+    {
+      /* Never complain about fchown failing.  But do it before
+        setting the modes, or they might be reset/ignored if the
+        owner is wrong.  */
+      if (fchown (newfd, st.st_uid, st.st_gid) != 0) { ; }
+      if (fchmod (newfd, st.st_mode & ALLPERMS) != 0
+         || close (newfd) != 0)
+        goto nonew_unlink;
+      newfd = -1;
+      if (rename (tmpfname, arfname) != 0)
+       goto nonew_unlink;
+    }
 
  errout:
   for (int cnt = 0; cnt < argc; ++cnt)

diff --git a/src/ranlib.c b/src/ranlib.c
index b908348443b8fd0b6d4addf848d0752b94ec1c7d..483a1b65802452a4ac3813b8ae1b12a0d5186434 100644 (file)
--- a/src/ranlib.c
+++ b/src/ranlib.c
@@ -245,25 +245,31 @@ handle_file (const char *fname)
          else
            rest_off = SARMAG;
 
-         if ((symtab.symsnamelen != 0
-              && ((write_retry (newfd, symtab.symsoff,
-                                symtab.symsofflen)
-                   != (ssize_t) symtab.symsofflen)
-                  || (write_retry (newfd, symtab.symsname,
-                                   symtab.symsnamelen)
-                      != (ssize_t) symtab.symsnamelen)))
-             /* Even if the original file had content before the
-                symbol table, we write it in the correct order.  */
-             || (index_off > SARMAG
-                 && copy_content (arelf, newfd, SARMAG, index_off - SARMAG))
-             || copy_content (arelf, newfd, rest_off, st.st_size - rest_off)
-             /* Set the mode of the new file to the same values the
-                original file has.  */
-             || fchmod (newfd, st.st_mode & ALLPERMS) != 0
-             /* Never complain about fchown failing.  */
-             || (({asm ("" :: "r" (fchown (newfd, st.st_uid, st.st_gid))); }),
-                 close (newfd) != 0)
-             || (newfd = -1, rename (tmpfname, fname) != 0))
+         if (symtab.symsnamelen != 0
+             && ((write_retry (newfd, symtab.symsoff,
+                               symtab.symsofflen)
+                  != (ssize_t) symtab.symsofflen)
+                 || (write_retry (newfd, symtab.symsname,
+                                  symtab.symsnamelen)
+                     != (ssize_t) symtab.symsnamelen)))
+           goto nonew_unlink;
+
+         /* Even if the original file had content before the
+            symbol table, we write it in the correct order.  */
+         if ((index_off > SARMAG
+              && copy_content (arelf, newfd, SARMAG, index_off - SARMAG))
+             || copy_content (arelf, newfd, rest_off, st.st_size - rest_off))
+           goto nonew_unlink;
+
+         /* Never complain about fchown failing.  */
+         if (fchown (newfd, st.st_uid, st.st_gid) != 0) { ; }
+         /* Set the mode of the new file to the same values the
+            original file has.  */
+         if (fchmod (newfd, st.st_mode & ALLPERMS) != 0
+             || close (newfd) != 0)
+           goto nonew_unlink;
+         newfd = -1;
+         if (rename (tmpfname, fname) != 0)
            goto nonew_unlink;
        }
     }