ar: Check whether ar header values fit.

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 <doko@ubuntu.com>
Signed-off-by: Mark Wielaard <mark@klomp.org>

diff --git a/src/ChangeLog b/src/ChangeLog
index 8370b696cf5b11ee163cc4e8796b9a3f7f898542..0d3bfc1921739a38222d6902730fbcdf25c0379e 100644 (file)
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,12 @@
+2017-09-10  Mark Wielaard  <mark@klomp.org>
+
+       * 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-09-01  Mark Wielaard  <mark@klomp.org>
 
        * stack.c (main): Replace \v in doc string with \n\n.

diff --git a/src/ar.c b/src/ar.c
index ec32cee59c2b7b93fcdbe4a363d23941d8fdf95a..818115bd93823be31435d90288a8ca32d4b1712f 100644 (file)
--- 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 <drepper@redhat.com>, 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 0d5bee75ccc81785fe25f806cb03a7f070975f74..7b6bf304aeaca5db54998c2a674b63b8e47a1ca6 100644 (file)
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,9 @@
+2017-09-10  Mark Wielaard  <mark@klomp.org>
+
+       * run-ar.sh: New test.
+       * Makefile.am (TESTS): Add run-ar.sh.
+       (EXTRA_DIST): Likewise.
+
 2017-08-18  Ulf Hermann <ulf.hermann@qt.io>
 
        * Makefile.am: Drop -rdynamic from deleted_lib_so_LDFLAGS.

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 2eac8020767f6775c6a54001d4748dc62fc57573..e583504622ecff3595e3d72d95ff6274c9dd5ee4 100644 (file)
--- 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 (executable)
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 <http://www.gnu.org/licenses/>.
+
+. $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