From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 107473 invoked by alias); 15 Sep 2017 11:15:02 -0000 Mailing-List: contact elfutils-devel-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Post: List-Help: List-Subscribe: Sender: elfutils-devel-owner@sourceware.org Received: (qmail 107444 invoked by uid 89); 15 Sep 2017 11:15:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Checked: by ClamAV 0.99.2 on sourceware.org X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY autolearn=ham version=3.3.2 spammy=107012 X-Spam-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY autolearn=ham version=3.3.2 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on sourceware.org X-Spam-Level: X-HELO: gnu.wildebeest.org Received: from wildebeest.demon.nl (HELO gnu.wildebeest.org) (212.238.236.112) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 15 Sep 2017 11:14:59 +0000 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 D26B3304D691; Fri, 15 Sep 2017 13:14:57 +0200 (CEST) Received: by tarox.wildebeest.org (Postfix, from userid 1000) id CD8A1413CC80; Fri, 15 Sep 2017 13:14:57 +0200 (CEST) From: Mark Wielaard To: elfutils-devel@sourceware.org Cc: Matthias Klose , Mark Wielaard Subject: [PATCH] ar: Check whether ar header values fit. Date: Fri, 15 Sep 2017 11:15:00 -0000 Message-Id: <1505474076-16062-1-git-send-email-mark@klomp.org> X-Mailer: git-send-email 1.8.3.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2017-q3/txt/msg00111.txt.bz2 When compiling with -O3 gcc finds an interesting error: src/ar.c: In function ‘do_oper_insert’: src/ar.c:1077:56: error: ‘%-*ld’ directive output may be truncated writing between 6 and 10 bytes into a region of size 7 [-Werror=format-truncation=] snprintf (tmpbuf, sizeof (tmpbuf), ofmt ? "%-*lo" : "%-*ld", bufsize, val); ^~~~~ The problem is that the ar header values have to fit in a limited (not zero terminated) string. We should check the snprintf return value to see if the values are representable. Also make ar valgrind and ubsan clean and add a minimal sanity test. Reported-by: Matthias Klose Signed-off-by: Mark Wielaard --- src/ChangeLog | 9 ++++++++ src/ar.c | 66 ++++++++++++++++++++++++++++++++++++++----------------- tests/ChangeLog | 6 +++++ tests/Makefile.am | 4 ++-- tests/run-ar.sh | 40 +++++++++++++++++++++++++++++++++ 5 files changed, 103 insertions(+), 22 deletions(-) create mode 100755 tests/run-ar.sh diff --git a/src/ChangeLog b/src/ChangeLog index daedfca..3c34026 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,12 @@ +2017-09-10 Mark Wielaard + + * ar.c (do_oper_delete): Remove DEBUG conditional check. + (no0print): Return bool. Check snprintf return value. + (do_oper_insert): Initialize elf. Remove DEBUG conditional check. + Check no0print calls succeed. Explicitly elf_end found elfs. + (do_oper_extract): Make sure we don't create an empty variable + length array. + 2017-05-04 Ulf Hermann * stack.c: Print pid_t using %lld. diff --git a/src/ar.c b/src/ar.c index ec32cee..818115b 100644 --- a/src/ar.c +++ b/src/ar.c @@ -1,5 +1,5 @@ /* Create, modify, and extract from archives. - Copyright (C) 2005-2012, 2016 Red Hat, Inc. + Copyright (C) 2005-2012, 2016, 2017 Red Hat, Inc. This file is part of elfutils. Written by Ulrich Drepper , 2005. @@ -442,7 +442,7 @@ static int do_oper_extract (int oper, const char *arfname, char **argv, int argc, long int instance) { - bool found[argc]; + bool found[argc > 0 ? argc : 1]; memset (found, '\0', sizeof (found)); size_t name_max = 0; @@ -1056,13 +1056,11 @@ do_oper_delete (const char *arfname, char **argv, int argc, goto nonew_unlink; errout: -#ifdef DEBUG elf_end (elf); arlib_fini (); close (fd); -#endif not_found (argc, argv, found); @@ -1070,12 +1068,18 @@ do_oper_delete (const char *arfname, char **argv, int argc, } -static void +/* Prints the given value in the given buffer without a trailing zero char. + Returns false if the given value doesn't fit in the given buffer. */ +static bool no0print (bool ofmt, char *buf, int bufsize, long int val) { char tmpbuf[bufsize + 1]; - snprintf (tmpbuf, sizeof (tmpbuf), ofmt ? "%-*lo" : "%-*ld", bufsize, val); + int ret = snprintf (tmpbuf, sizeof (tmpbuf), ofmt ? "%-*lo" : "%-*ld", + bufsize, val); + if (ret >= (int) sizeof (tmpbuf)) + return false; memcpy (buf, tmpbuf, bufsize); + return true; } @@ -1084,7 +1088,7 @@ do_oper_insert (int oper, const char *arfname, char **argv, int argc, const char *member) { int status = 0; - Elf *elf; + Elf *elf = NULL; struct stat st; int fd = open_archive (arfname, O_RDONLY, 0, &elf, &st, oper != oper_move); @@ -1303,13 +1307,11 @@ do_oper_insert (int oper, const char *arfname, char **argv, int argc, if (status != 0) { -#ifdef DEBUG elf_end (elf); arlib_fini (); close (fd); -#endif return status; } @@ -1463,14 +1465,36 @@ do_oper_insert (int oper, const char *arfname, char **argv, int argc, memcpy (arhdr.ar_name, tmpbuf, sizeof (arhdr.ar_name)); } - no0print (false, arhdr.ar_date, sizeof (arhdr.ar_date), - all->sec); - no0print (false, arhdr.ar_uid, sizeof (arhdr.ar_uid), all->uid); - no0print (false, arhdr.ar_gid, sizeof (arhdr.ar_gid), all->gid); - no0print (true, arhdr.ar_mode, sizeof (arhdr.ar_mode), - all->mode); - no0print (false, arhdr.ar_size, sizeof (arhdr.ar_size), - all->size); + if (! no0print (false, arhdr.ar_date, sizeof (arhdr.ar_date), + all->sec)) + { + error (0, errno, gettext ("cannot represent ar_date")); + goto nonew_unlink; + } + if (! no0print (false, arhdr.ar_uid, sizeof (arhdr.ar_uid), + all->uid)) + { + error (0, errno, gettext ("cannot represent ar_uid")); + goto nonew_unlink; + } + if (! no0print (false, arhdr.ar_gid, sizeof (arhdr.ar_gid), + all->gid)) + { + error (0, errno, gettext ("cannot represent ar_gid")); + goto nonew_unlink; + } + if (! no0print (true, arhdr.ar_mode, sizeof (arhdr.ar_mode), + all->mode)) + { + error (0, errno, gettext ("cannot represent ar_mode")); + goto nonew_unlink; + } + if (! no0print (false, arhdr.ar_size, sizeof (arhdr.ar_size), + all->size)) + { + error (0, errno, gettext ("cannot represent ar_size")); + goto nonew_unlink; + } memcpy (arhdr.ar_fmag, ARFMAG, sizeof (arhdr.ar_fmag)); if (unlikely (write_retry (newfd, &arhdr, sizeof (arhdr)) @@ -1514,13 +1538,15 @@ do_oper_insert (int oper, const char *arfname, char **argv, int argc, goto nonew_unlink; errout: -#ifdef DEBUG + for (int cnt = 0; cnt < argc; ++cnt) + elf_end (found[cnt]->elf); + elf_end (elf); arlib_fini (); - close (fd); -#endif + if (fd != -1) + close (fd); return status; } diff --git a/tests/ChangeLog b/tests/ChangeLog index 0d5bee7..7b6bf30 100644 --- a/tests/ChangeLog +++ b/tests/ChangeLog @@ -1,3 +1,9 @@ +2017-09-10 Mark Wielaard + + * run-ar.sh: New test. + * Makefile.am (TESTS): Add run-ar.sh. + (EXTRA_DIST): Likewise. + 2017-08-18 Ulf Hermann * Makefile.am: Drop -rdynamic from deleted_lib_so_LDFLAGS. diff --git a/tests/Makefile.am b/tests/Makefile.am index 2eac802..e583504 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -72,7 +72,7 @@ backtrace-child-biarch$(EXEEXT): backtrace-child.c $(AM_LDFLAGS) $(LDFLAGS) $(backtrace_child_LDFLAGS) \ -o $@ $< -TESTS = run-arextract.sh run-arsymtest.sh newfile test-nlist \ +TESTS = run-arextract.sh run-arsymtest.sh run-ar.sh newfile test-nlist \ update1 update2 update3 update4 \ run-show-die-info.sh run-get-files.sh run-get-lines.sh \ run-get-pubnames.sh run-get-aranges.sh run-allfcts.sh \ @@ -159,7 +159,7 @@ check_PROGRAMS += $(asm_TESTS) TESTS += $(asm_TESTS) run-disasm-bpf.sh endif -EXTRA_DIST = run-arextract.sh run-arsymtest.sh \ +EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \ run-show-die-info.sh run-get-files.sh run-get-lines.sh \ run-get-pubnames.sh run-get-aranges.sh \ run-show-abbrev.sh run-strip-test.sh \ diff --git a/tests/run-ar.sh b/tests/run-ar.sh new file mode 100755 index 0000000..fb9394d --- /dev/null +++ b/tests/run-ar.sh @@ -0,0 +1,40 @@ +#! /bin/bash +# Copyright (C) 2017 Red Hat, 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 . + +. $srcdir/test-subr.sh + +tempfiles objects.list test.ar + +echo Make a sorted list of the just build src .o files. +(cd ${abs_top_builddir}/src; ls *.o | sort) > objects.list +cat objects.list + +echo Create a new ar file with the .o files. +testrun ${abs_top_builddir}/src/ar -r test.ar \ + $(echo ${abs_top_builddir}/src/*.o | sort) + +echo List the ar file contents. +testrun_compare ${abs_top_builddir}/src/ar -t test.ar < objects.list + +echo Delete all objects again. +testrun ${abs_top_builddir}/src/ar -d test.ar $(cat objects.list) + +echo Check new ar file is now empty +testrun_compare ${abs_top_builddir}/src/ar -t test.ar << EOF +EOF + +exit 0 -- 1.8.3.1