fts: avoid reading beyond the heap allocation
authorPádraig Brady <P@draigBrady.com>
Wed, 24 Jun 2015 22:52:24 +0000 (23:52 +0100)
committerPádraig Brady <P@draigBrady.com>
Thu, 25 Jun 2015 01:35:59 +0000 (02:35 +0100)
GCC 5.1.1 with -O2 and -fsanitize=address reports
the following from `chmod a+rx ..` for example:

  ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61200000be48
  READ of size 4 at 0x61200000be48 thread T0
    #0 0x40f0a1 in fts_stat lib/fts.c:1821
    #1 0x4141ec in fts_open lib/fts.c:514
    #2 0x40ca6e in xfts_open lib/xfts.c:36
    #3 0x402c35 in process_files src/chmod.c:336
    #4 0x402c35 in main src/chmod.c:566
    #5 0x7f8fdc38678f in __libc_start_main (/lib64/libc.so.6+0x2078f)
    #6 0x404608 in _start (/home/padraig/git/coreutils/src/chmod+0x404608)

  0x61200000be4b is located 0 bytes to the right of
  267-byte region [0x61200000bd40,0x61200000be4b) allocated by thread T0 here:
    #0 0x7f8fdd4cfa0a in malloc (/lib64/libasan.so.2+0x98a0a)
    #1 0x40f22c in fts_alloc lib/fts.c:1916

The read of size 4 from a heap object of size 3 is indeed invalid,
though this may be due to incorrect padding assumptions by GCC, see:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66661

The read is coming from the a[2] access in ISDOT() which GCC seems
to assume can access according to the alignment of the main structure,
since the flexible array members are being accessed directly.
One could cast the parameter to ISDOT() to (char const*) to restrict
to byte at a time access, however it seems more correct and maintainable
to increase the buffer size to the appropriate alignment to avoid this issue.
For reference some notes on alignment and flexible array members are at:
https://sites.google.com/site/embeddedmonologue/home/c-programming/data-alignment-structure-padding-and-flexible-array-member

* lib/fts.c (fts_alloc): Increase allocation to alignof(FTSENT).
* modules/fts: Depend on stdalign.

ChangeLog
lib/fts.c
modules/fts

index ef313402c7083cb9a29045575451ab54ed530ee5..7ca6bf0e98f4afa3cdbf5b712a2e08f370623031 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2015-06-25  Pádraig Brady  <P@draigBrady.com>
+
+       fts: avoid reading beyond the heap allocation
+       GCC 5.1.1 with -O2 and -fsanitize=address reports
+       a read of size 4 from a heap object of size 3 is indeed invalid,
+       though this may be due to incorrect padding assumptions by GCC, see:
+       https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66661
+       * lib/fts.c (fts_alloc): Increase allocation to alignof(FTSENT).
+       * modules/fts: Depend on stdalign.
+
 2015-06-24  Pádraig Brady  <P@draigBrady.com>
 
        savedir: avoid undefined behavior in qsort call
index 608410147788cbb167b6bae7a62481c007f422c1..a2f65eb848df0b509d9722b7b907783466a4f0af 100644 (file)
--- a/lib/fts.c
+++ b/lib/fts.c
@@ -62,6 +62,7 @@ static char sccsid[] = "@(#)fts.c       8.6 (Berkeley) 8/14/94";
 #endif
 #include <fcntl.h>
 #include <errno.h>
+#include <stdalign.h>
 #include <stdbool.h>
 #include <stdlib.h>
 #include <string.h>
@@ -1906,6 +1907,12 @@ fts_alloc (FTS *sp, const char *name, register size_t namelen)
          * structure and the file name in one chunk.
          */
         len = offsetof(FTSENT, fts_name) + namelen + 1;
+        /* Round the allocation up so it has the same alignment as FTSENT,
+           so that trailing padding may be referenced by direct access
+           to the flexible array members, without triggering undefined behavior
+           by accessing bytes beyond the heap allocation.  This implicit access
+           was seen for example with ISDOT() and GCC 5.1.1 at -O2.  */
+        len = (len + alignof(FTSENT) - 1) & ~(alignof(FTSENT) - 1);
         if ((p = malloc(len)) == NULL)
                 return (NULL);
 
index 3316264d037a42273e9f324adf4f43b25f12c0bf..b5dcd326077a8e3d38f9d575511a939a55a8379a 100644 (file)
@@ -29,6 +29,7 @@ openat-h
 openat-safer
 opendir
 readdir
+stdalign
 stdbool
 unistd-safer