From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (wildebeest.demon.nl [212.238.236.112]) by sourceware.org (Postfix) with ESMTPS id B56243858D34 for ; Tue, 16 Jun 2020 22:26:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B56243858D34 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mark@klomp.org Received: from tarox.wildebeest.org (tarox.wildebeest.org [172.31.17.39]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id 76F5D317FC13; Wed, 17 Jun 2020 00:25:59 +0200 (CEST) Received: by tarox.wildebeest.org (Postfix, from userid 1000) id 24186402412A; Wed, 17 Jun 2020 00:25:58 +0200 (CEST) From: Mark Wielaard To: elfutils-devel@sourceware.org Cc: Mark Wielaard Subject: [PATCH 06/10] ar, ranlib: Call fchown before fchmod, explicitly check and ignore errors. Date: Wed, 17 Jun 2020 00:25:35 +0200 Message-Id: <20200616222539.29109-6-mark@klomp.org> X-Mailer: git-send-email 2.18.4 In-Reply-To: <20200616222539.29109-1-mark@klomp.org> References: <20200616222539.29109-1-mark@klomp.org> X-Spam-Status: No, score=-12.5 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: elfutils-devel@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Elfutils-devel mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 16 Jun 2020 22:26:04 -0000 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 --- src/ChangeLog | 8 +++++++ src/ar.c | 65 ++++++++++++++++++++++++++++++--------------------- src/ranlib.c | 44 +++++++++++++++++++--------------- 3 files changed, 72 insertions(+), 45 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index e78bc358..0129b8bf 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,11 @@ +2020-06-16 Mark Wielaard + + * 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 * elflint.c (check_elf_header): Explicitly check and ignore diff --git a/src/ar.c b/src/ar.c index d70f1f46..7d33d814 100644 --- 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 b9083484..483a1b65 100644 --- 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; } } -- 2.18.4