diff mbox

enhance buffer overflow warnings (and c/53562)

Message ID 334666bc-6308-aa5f-f63f-40697695152f@gmail.com
State New
Headers show

Commit Message

Martin Sebor Oct. 31, 2016, 4:44 p.m. UTC 
On 10/31/2016 06:39 AM, Tobias Burnus wrote:
> Martin Sebor wrote:
>> Attached is an updated patch that adds checks for excessive sizes
>> and bounds (those in excess of SIZE_MAX / 2), and also enables
>> the same checking for strcat and strncat).  This version also
>> fixes an issue with the interpretation of anti-ranges in the
>> first patch.  The improvements exposed two bugs in the regression
>> tests.
>
> If I apply this patch to my local trunk - and try to bootstrap GCC,
> bootstrapping fails (on x86-64_gnu-linux) as following. I have not
> tried to figure out whether the warning (-Werror) makes sense or not.
>
> ../../gcc/emit-rtl.c: In function ‘rtx_note* make_note_raw(insn_note)’:
> ../../gcc/emit-rtl.c:3933:59: error: void* memset(void*, int, size_t) writing 8 bytes into a region of size 0 overflows the destination [-Werror=stringop-overflow]
>    memset (&NOTE_DATA (note), 0, sizeof (NOTE_DATA (note)));
>
>
> where NOTE_DATA is defined in rtl.h as
>
> /* Opaque data.  */
> #define NOTE_DATA(INSN)         RTL_CHECKC1 (INSN, 3, NOTE)

Thanks for trying it out!  The patch bootstrapped and passed regression
tests for me yesterday, also on x86_64, and today on powepc64le, but
just now I reproduced the error with the top of today's trunk on
x86_64.  I think the error is justified because the call to memset
in the make_note_raw function references a fourth element
(rtx_note::rtx_insn::rtx_def::u.fld[3]) of what is just a single
element array.  After macro expansion, the memset call itself:

   memset (&((note)->u.fld[3]), 0, sizeof (((note)->u.fld[3])));

is within the bounds of the object pointed to by note but the index
3 is out of bounds for note->u.fld and so undefined.  An unpatched
GCC issues a similar error when the call to memset is replaced with
__builtin___memset_chk like so:

   __builtin___memset_chk (&((note)->u.fld[3]), 0,
                           sizeof (((note)->u.fld[3])),
			  __builtin_object_size (&((note)->u.fld[3]), 1));

/src/gcc/53562/gcc/emit-rtl.c: In function ‘rtx_note* 
make_note_raw(insn_note)’:
/src/gcc/53562/gcc/emit-rtl.c:3941:53: warning: call to void* 
__builtin___memset_chk(void*, int, long unsigned int, long unsigned int) 
will always overflow destination buffer

The code can be made valid and the warning avoided by deriving
the same address not from an invalid pointer/subscript but rather
from a pointer to the beginning of the union itself and adding
the offset of the fourth element, like so:

   char *p = (char*) &(note)->u.fld[0];
   p += sizeof (((note)->u.fld[0])) * 3;
   memset (p, 0, sizeof *p);

Unfortunately, because the invalid subscript is the result of
the expansion of the RTL_CHECK1() macro fixing this isn't as
straightforward as that.  What really should be fixed is the macro
itself.  Until then, it can be worked (hacked would be a better
term) around it by storing the result of &NOTE_DATA (note)
expression in a volatile pointer, like so:

Martin
diff mbox

Patch

diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
index 8afcfbe..6dd9439 100644
--- a/gcc/emit-rtl.c
+++ b/gcc/emit-rtl.c
@@ -3930,7 +3930,8 @@  make_note_raw (enum insn_note subtype)
    INSN_UID (note) = cur_insn_uid++;
    NOTE_KIND (note) = subtype;
    BLOCK_FOR_INSN (note) = NULL;
-  memset (&NOTE_DATA (note), 0, sizeof (NOTE_DATA (note)));
+  void* volatile p = &NOTE_DATA (note);
+  memset (p, 0, sizeof (NOTE_DATA (note)));
    return note;
  }