diff mbox

- improve sprintf buffer overflow detection (middle-end/49905)

Message ID yddh98g38m7.fsf@CeBiTec.Uni-Bielefeld.DE
State New
Headers show

Commit Message

Rainer Orth Oct. 13, 2016, 9:23 a.m. UTC 
Hi Martin,

>> as it happens, I'd already started bootstraps with your patch before
>> your mail arrived :-)
>
> Thanks for your help getting to the bottom of this!
>
>>
>> We're left with
>>
>> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test for excess errors)
>> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-4.c (test for excess errors)
>>
>> for 32 bit and
>>
>> FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-4.c (test for excess errors)
>>
>> for 64 bit on both i386-pc-solaris2.12 and sparc-sun-solaris2.12.
>>
>> In the 32-bit builtin-sprintf-warn-1.c case, there are many instances of
>>
>> Excess errors:
>> /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:224:3:
>> warning: format '%lc' expects argument of type 'wint_t', but argument 5
>> has type 'int' [-Wformat=]
>
> I've built the sparc-sun-solaris2.12 toolchain and reproduced these
> warnings.  They are vestiges of those I saw and some of which I fixed
> before.  The problem is that %lc expects a wint_t argument which on
> this target is an alias for long in but the argument of 0 has type
> int.  The warning is coming out of the -Wformat checker which doesn't
> seem to care that int and long have the same size.  I've committed
> r240758 that should fix the remaining warnings of this kind but long
> term I think GCC should change to avoid warning in this case (Clang
> doesn't).
>
>>
>> while the second is
>>
>> Excess errors:
>> /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c:15:23:
>> warning: writing a terminating nul past the end of the destination
>> [-Wformat-length=]/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c:30:21:
>> warning: writing format character '4' at offset 3 past the end of the
>> destination [-Wformat-length=]
>> /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c:46:21:
>> warning: writing format character '4' at offset 3 past the end of the
>> destination [-Wformat-length=]
>> /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c:61:25:
>> warning: writing a terminating nul past the end of the destination
>> [-Wformat-length=]
>> /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c:74:22:
>> warning: '%-s' directive writing 4 bytes into a region of size 1
>> [-Wformat-length=]
>>
>> I've no idea yet why in the first error message two different messages
>> are joined into one line.  Probably something with DejaGnu mangling the
>> output...
>
> I've reproduced this as well and it took me a while to see the
> problem.  It turns out that the target specifier I used in the
> test (*-*-*-*) happened to match my native target
> x86_64-pc-linux-gnu but not sparc-sun-solaris2.12.  Let me fix
> that in the next patch.  Hopefully with that all the remaining
> failures should clear up.
>
> Thanks again for your help and patience!

No worries: I've refreshed your patch on top of Thomas Preud'homme's for
PR testsuite/77710 and found that one more bit is needed to fix this
completely.  32-bit Solaris shows three more warnings:

/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1355:3: warning: format '%lc' expects argument of type 'wint_t', but argument 6 has type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1356:3: warning: format '%lc' expects argument of type 'wint_t', but argument 6 has type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1357:3: warning: format '%lc' expects argument of type 'wint_t', but argument 6 has type 'int' [-Wformat=]

Fixed as follows:
With this one and your refreshed patch, all failures are gone now for
i386-pc-solaris2.12, sparc-sun-solaris2.12, and x86_64-pc-linux-gnu.

	Rainer

Comments

Martin Sebor Oct. 13, 2016, 4:17 p.m. UTC | #1 
> No worries: I've refreshed your patch on top of Thomas Preud'homme's for
> PR testsuite/77710 and found that one more bit is needed to fix this
> completely.  32-bit Solaris shows three more warnings:
>
> /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1355:3: warning: format '%lc' expects argument of type 'wint_t', but argument 6 has type 'int' [-Wformat=]
> /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1356:3: warning: format '%lc' expects argument of type 'wint_t', but argument 6 has type 'int' [-Wformat=]
> /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1357:3: warning: format '%lc' expects argument of type 'wint_t', but argument 6 has type 'int' [-Wformat=]

Rats!  I overlooked those in followup patch I committed to fix
the others.  I had tested the change with a 32-bit cross compiler
but I still see them in the 32-bit Solaris cross compiler, though
not in the i366 one.  I assumed the i386 compiler was a good enough
proxy but now that I've checked more carefully I see that it warns
for %lc with a wchar_t argument such as L'a' but not for int such
as 0, while the 32-bit Solaris compiler for %lc with an int argument
and not for wchar_t.

In the i386 compiler wchar_t is long and wint_t is unsigned int while
in the Solaris one both wchar_t and wint_t are long int.  Even though
these types and arguments are the same width (and on Solaris even the
same sign), -Wformat still warns.

I've fixed fix this in the test in r241123.  Since I didn't manage
to convince Joseph that the warning is unhelpful in our discussion
last week I wasn't going to pursue it but I've now changed my mind.
The warning is obviously detrimental to portability so I've raised
bug 77970 for it.

Thanks
Martin

>
> Fixed as follows:
>
>
>
>
> With this one and your refreshed patch, all failures are gone now for
> i386-pc-solaris2.12, sparc-sun-solaris2.12, and x86_64-pc-linux-gnu.
>
> 	Rainer
>
diff mbox

Patch

# HG changeset patch
# Parent  1aaf616a61b8ea3ecff9313e059a1e85571cdde1
[testsuite] Fix 32-bit gcc.dg/tree-ssa/builtin-sprintf-warn-1.c on Solaris

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c
--- a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c
@@ -1352,9 +1352,9 @@  void test_snprintf_chk_c_const (void)
   T (3, "%c_%c", '1', '2');      /* { dg-warning "output truncated" } */
 
   /* Wide characters.  */
-  T (0, "%lc",  0);
-  T (1, "%lc",  0);
-  T (2, "%lc",  0);
+  T (0, "%lc",  (wint_t)0);
+  T (1, "%lc",  (wint_t)0);
+  T (2, "%lc",  (wint_t)0);
 
   /* The following could result in as few as a single byte and in as many
      as MB_CUR_MAX, but since the MB_CUR_MAX value is a runtime property