Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

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

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Rainer Orth]

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:

# 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

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

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Joseph Myers]

On Thu, 6 Oct 2016, Martin Sebor wrote:

> > The problem is that extracting via an integer of the opposite signedness
> > is *not* defined unless the argument has a value representable in both
> > types - both as a matter of ISO C rules on variadic functions, and as a
> > practical matter of what various ABIs say about how sub-word arguments are
> > extended when passed as function arguments.
> 
> No, sign mismatch is not the problem we're discussing.  GCC doesn't
> warn for %lc when wchar_t and wint_t differ in signedness except
> with -Wformat-signedness.  The cause of the warning is that wchar_t
> is a distinct type from wint_t such as long from int (irrespective
> of their sign), even if the two have the same size, and the checker
> doesn't distinguish the typedefs from the underlying types.

For most configurations of GCC, wint_t is unsigned int (the default from 
defaults.h).  Only a few specific (OS, CPU) pairs make a different choice.

> I did say, however, that the warning might have some merit even
> in this case if there also were a target to which the code could
> be ported where wchar_t and wint_t had different sizes (i.e., as
> a portability warning).  But for such a portability warning to
> be useful it would need to be issued by all GCC compilers, not
> just when compiling for the target with that property.  (But
> since neither of us knows(*) of such a target this is a moot
> point.)

As I pointed out, wint_t and wchar_t are ABI-incompatible when passed to 
functions on powerpc64-linux-gnu, because of the different signedness, 
even though they have the same size.

Thus, diagnosing the wrong choice of type is useful as a portability 
warning.  Unfortunately, it's hard to diagnose it on architectures where 
wchar_t and wint_t are the same type or too similar, but by diagnosing 
where they are sufficiently different at the C level (but possibly not at 
the ABI level), code can be fixed so it doesn't break in cases where the 
difference matters.

> The text of the warning itself indicates that even suggesting how
> to fix it isn't trivial.  GCC warns for just for the %lc directive
> (not for the sign mismatch) and suggests to fix it by replacing
> the %lc directive with %ld.  That's clearly wrong.  Printing

It's true the hint is wrong, but the warning is right.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 10/05/2016 05:54 PM, Joseph Myers wrote:

On Wed, 5 Oct 2016, Martin Sebor wrote:


On 10/05/2016 05:11 PM, Joseph Myers wrote:

On Wed, 5 Oct 2016, Martin Sebor wrote:


may have been subjected to.  Issuing a warning for a safe piece
of code only on the basis that there might be some other piece
of code in the program that does something wrong makes no sense.
Suggesting to the user to change the safe piece of code is
misleading and counterproductive.


It's warning because it's *bad code*.


Why?  What can go wrong and on what system?  Please name at least
one specific example.


It's bad because getting types wrong (more generally than just in this
case) is a careless coding practice.  It's a code smell.


There are lots of other "smells" that GCC doesn't warn about by
default even though it easily could, including -Wformat-signedness.


The problem is that extracting via an integer of the opposite signedness
is *not* defined unless the argument has a value representable in both
types - both as a matter of ISO C rules on variadic functions, and as a
practical matter of what various ABIs say about how sub-word arguments are
extended when passed as function arguments.


No, sign mismatch is not the problem we're discussing.  GCC doesn't
warn for %lc when wchar_t and wint_t differ in signedness except
with -Wformat-signedness.  The cause of the warning is that wchar_t
is a distinct type from wint_t such as long from int (irrespective
of their sign), even if the two have the same size, and the checker
doesn't distinguish the typedefs from the underlying types.

My position is that when int and long have the same size, although
warning for objects of those types is appropriate because their
sizes often do differ from one target to the next, the %lc warning
is unhelpful because, unlike the underlying types, wchar_t and
wint_t are specifically designed to be interoperable and the same
propensity for a difference in size doesn't apply to them.

I did say, however, that the warning might have some merit even
in this case if there also were a target to which the code could
be ported where wchar_t and wint_t had different sizes (i.e., as
a portability warning).  But for such a portability warning to
be useful it would need to be issued by all GCC compilers, not
just when compiling for the target with that property.  (But
since neither of us knows(*) of such a target this is a moot
point.)


But my basic point is: if something can lead to analysis in this level of
detail of whether the code is or is not safe in a particular context,
while there is a trivial fix to the code that would short-circuit all that
analysis, that by itself is enough evidence that the code deserves a
warning and should be cleaned up to make it more obviously safe.


The premise that "it's trivial to fix" is wrong and at the core
of my objection to the warning (by default; I would have no issue
with it being issued under a separate option or under -Wpedantic).

The text of the warning itself indicates that even suggesting how
to fix it isn't trivial.  GCC warns for just for the %lc directive
(not for the sign mismatch) and suggests to fix it by replacing
the %lc directive with %ld.  That's clearly wrong.  Printing
a hint with fix you're looking for (i.e., casting the argument
to wint_t) may be difficult because of the lack of location
information for local variables.

  $ cat a.c && gcc -S -Wall -m32 a.c
  typedef __WCHAR_TYPE__ wchar_t;

  void f (int x, unsigned y)
  {
extern wchar_t w;

__builtin_printf ("%lc", w);
__builtin_printf ("%lc", x);
__builtin_printf ("%lc", y);
  }
  a.c: In function ‘f’:
  a.c:7:24: warning: format ‘%lc’ expects argument of type ‘wint_t’, 
but argument 2 has type ‘wchar_t {aka long int}’ [-Wformat=]

 __builtin_printf ("%lc", w);
~~^
%ld

But I think we're both starting to repeat ourselves without making
any headway.  I spent more time on this discussion (and the research
I did for it) than I should have an I don't view this as serious
enough of a problem to be worth spending more time on, just a minor
wart in GCC.  There are plenty others with a much better ROI.

Martin

[*] Since you've persistently dodged the question about the target
where this might be a problem I surveyed the GCC config files to
see if I could find one.  All but two of the operating systems GCC
apparently knows about define wint_t to be an alias for signed int.
The two exceptions are Ming32 and VxWorks both of which define both
wint_t and wchar_t to be unsigned short, and so those two targets
can be ruled out.

Besides these, the are a bunch of back ends that define wchar_t
to be long where, if long were wider than int, calling
printf("%lc", L'x') would be a problem.  But based on a closer
review of some of these targets it looks like they define both
int and long as 32-bit types and so there is no issue there.
I didn't look at all of them but defining wint_t to be 

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Joseph Myers]

On Wed, 5 Oct 2016, Martin Sebor wrote:

> On 10/05/2016 05:11 PM, Joseph Myers wrote:
> > On Wed, 5 Oct 2016, Martin Sebor wrote:
> > 
> > > may have been subjected to.  Issuing a warning for a safe piece
> > > of code only on the basis that there might be some other piece
> > > of code in the program that does something wrong makes no sense.
> > > Suggesting to the user to change the safe piece of code is
> > > misleading and counterproductive.
> > 
> > It's warning because it's *bad code*.
> 
> Why?  What can go wrong and on what system?  Please name at least
> one specific example.

It's bad because getting types wrong (more generally than just in this 
case) is a careless coding practice.  It's a code smell.  By requiring 
much more careful reasoning about whether it is OK in a particular 
context, given knowledge of ABIs and compiler optimizations, such code 
smells are liable to distract attention from other problems in the code, 
as well as being symptomatic of possible other problems in the code.

It should be possible to accommodate -Wall within a wide range of code 
styles, but you may will still need to adapt your code to it in some ways 
if you are doing things that are commonly dubious, even if you have 
reasons why they are safe in the cases that appear in your code and in the 
contexts in which that code will be used.

> > I mean high parts if the ABI is passing those arguments in 64-bit
> > registers / stack slots and has requirements on how 32-bit arguments are
> > extended for passing as 64-bit.
> 
> I don't understand what you mean.  We're talking about passing
> an integer (wchar_t) via the ellipsis.  That's just as well
> defined as passing any other integer of that size, as is (for
> all practical purposes) extracting it via an integer of the
> opposite signedness.  printf then stuffs the extracted wint_t
> value (which is of the same size as the original wchar_t) into
> a wchar_t, appends a nul, and formats the result via %ls (at
> least that's how it's specified).

Consider the powerpc64 ABI used for GNU/Linux (both BE and LE ABIs have 
this property).  A 32-bit argument is passed in a register (or in memory 
after a certain point), sign-extended to 64-bit if of a signed type and 
zero-extended to 64-bit if of an unsigned type.  wchar_t is int and wint_t 
is unsigned int in this case.  So if you pass a negative wchar_t argument, 
and the callee uses va_arg with type wint_t to access it, the compiler 
compiling the callee is entitled to optimize it on the basis that the 
value is already zero-extended to 64 bits, resulting in undefined behavior 
because the caller in fact sign-extended it.  Even if GCC doesn't optimize 
on that basis today, it could in future - and with a printf call, it's 
very likely that code compiled now will be used with a future libc.so 
shared library compiled with a newer compiler version, so printf may be 
built with a newer compiler than the code calling it.

The problem is that extracting via an integer of the opposite signedness 
is *not* defined unless the argument has a value representable in both 
types - both as a matter of ISO C rules on variadic functions, and as a 
practical matter of what various ABIs say about how sub-word arguments are 
extended when passed as function arguments.

(Negative wchar_t isn't actually a valid wide character, but without 
knowing where the wchar_t came from it's entirely possible it could have a 
value of type wchar_t that doesn't represent a valid character.  If you 
properly pass a wint_t value, then you avoid undefined behavior for 
invalid characters, since they are defined to produce an EILSEQ error.)

But my basic point is: if something can lead to analysis in this level of 
detail of whether the code is or is not safe in a particular context, 
while there is a trivial fix to the code that would short-circuit all that 
analysis, that by itself is enough evidence that the code deserves a 
warning and should be cleaned up to make it more obviously safe.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 10/05/2016 05:11 PM, Joseph Myers wrote:

On Wed, 5 Oct 2016, Martin Sebor wrote:


may have been subjected to.  Issuing a warning for a safe piece
of code only on the basis that there might be some other piece
of code in the program that does something wrong makes no sense.
Suggesting to the user to change the safe piece of code is
misleading and counterproductive.


It's warning because it's *bad code*.


Why?  What can go wrong and on what system?  Please name at least
one specific example.


Whether it's safe or not is
peripheral.  Lots of warnings are for things that are dubious even if they
may happen to do what the user wants on the platforms they care about.
I'd consider even cases of different signedness (e.g. passing one of int
and unsigned int to a format expecting the other) to be dubious without a
reason why values not representable in the other type can't reach that
code - and I'd consider cases where the types are different not just in
signedness to be clearly worse than the cases that we only warn for with
-Wformat-signedness.


There are plenty of architectures where the ABI does require the high part
to be extended in a particular way;


There is no high part when the wchar_t and wint_t types have
the same size, which they do not only in the case of interest but


I mean high parts if the ABI is passing those arguments in 64-bit
registers / stack slots and has requirements on how 32-bit arguments are
extended for passing as 64-bit.


I don't understand what you mean.  We're talking about passing
an integer (wchar_t) via the ellipsis.  That's just as well
defined as passing any other integer of that size, as is (for
all practical purposes) extracting it via an integer of the
opposite signedness.  printf then stuffs the extracted wint_t
value (which is of the same size as the original wchar_t) into
a wchar_t, appends a nul, and formats the result via %ls (at
least that's how it's specified).

Please explain what can go wrong on i386 when someone calls
printf("%lc", L'a'), or on any other target GCC supports.
I will be happy to agree that the warning is justified if
you show me how this breaks and where.

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Joseph Myers]

On Wed, 5 Oct 2016, Martin Sebor wrote:

> may have been subjected to.  Issuing a warning for a safe piece
> of code only on the basis that there might be some other piece
> of code in the program that does something wrong makes no sense.
> Suggesting to the user to change the safe piece of code is
> misleading and counterproductive.

It's warning because it's *bad code*.  Whether it's safe or not is 
peripheral.  Lots of warnings are for things that are dubious even if they 
may happen to do what the user wants on the platforms they care about.  
I'd consider even cases of different signedness (e.g. passing one of int 
and unsigned int to a format expecting the other) to be dubious without a 
reason why values not representable in the other type can't reach that 
code - and I'd consider cases where the types are different not just in 
signedness to be clearly worse than the cases that we only warn for with 
-Wformat-signedness.

> > There are plenty of architectures where the ABI does require the high part
> > to be extended in a particular way;
> 
> There is no high part when the wchar_t and wint_t types have
> the same size, which they do not only in the case of interest but

I mean high parts if the ABI is passing those arguments in 64-bit 
registers / stack slots and has requirements on how 32-bit arguments are 
extended for passing as 64-bit.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

The problem is e.g. code calling getwchar and not checking for WEOF.
0xu does not fit in a 32-bit signed variable unless your code is
confusing signed and unsigned freely.  And even if you consider it OK to
check after conversion to wchar_t rather than before, the lack of a wint_t
variable for the result seems likely to accompany a missing check.


Calling printf("%lc", x) with a wchar_t argument hardly suggests
anything about where the argument originated or what checking it
may have been subjected to.  Issuing a warning for a safe piece
of code only on the basis that there might be some other piece
of code in the program that does something wrong makes no sense.
Suggesting to the user to change the safe piece of code is
misleading and counterproductive.


There are plenty of architectures where the ABI does require the high part
to be extended in a particular way;


There is no high part when the wchar_t and wint_t types have
the same size, which they do not only in the case of interest but
also in all the other cases we know of.  Again, unless you know
of a counterexample this point isn't relevant to the discussion.


I think the warning is the correct thing for anyone trying to write C as a
high-level language and be type-correct and properly check before doing
conversions that might change values unless those changes are actually
intended, and that warning rather than lesser warnings for "C as portable
assembler" is the appropriate default for format checking.


I certainly agree with this general philosophy.  Unfortunately,
the wchar_t and wint_t types on the target of interest (i386
or ILP32 on x86_64) are defined in an unhelpful way that the
-Wformat checker normally uses to diagnose real (or potential)
problems that can do do come up with their underlying types.
They just can't come up with wchar_t and wint_t.  Useful
warnings shouldn't pedantically enforce type rules in cases
where the rules serve no practical purpose or are loose enough
to allow for nonsensical implementations.  This is a basic
principle that's been followed by the vast majority of GCC
warnings and that users have come to appreciate and rely on.

The underlying problem here (and I claim a bug) is simply that
the -Wformat code doesn't distinguish the wint_t typedef (and
in C, the wchar_t typedef as well) from their underlying types
and treats the typedefs the same.  It should not, at least not
at the default warning level.  I wouldn't object to retaining
the warning with -Wpedantic but otherwise it's unhelpful.

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Joseph Myers]

On Wed, 5 Oct 2016, Martin Sebor wrote:

> > Or it could show up a logical error where the code should have had a
> > wint_t variable and checked for WEOF somewhere but just assumed things
> > would fit in wchar_t
> 
> WEOF that fits in a wint_t also fits in a wchar_t when the two types
> have the same precision, the case where GCC issues the warning (and
> where WEOF expands to 0xu).  So this isn't an example of
> a problem that could arise on the targets that brought this up, or
> any other targets that I'm aware of.

The problem is e.g. code calling getwchar and not checking for WEOF.  
0xu does not fit in a 32-bit signed variable unless your code is 
confusing signed and unsigned freely.  And even if you consider it OK to 
check after conversion to wchar_t rather than before, the lack of a wint_t 
variable for the result seems likely to accompany a missing check.

> > (and so could be passing a negative value where the
> > ABI means the callee expects wint_t values to be zero-extended, with
> > consequent undefined behavior there).
> 
> Sign extension is also not an issue on the target where the warning
> is issued (x86_64 ILP32) and where wchar_t is int and wint_t is
> unsigned long.

There are plenty of architectures where the ABI does require the high part 
to be extended in a particular way; that's why the Linux kernel has 
syscall wrappers, because improperly extended 32-bit syscall arguments 
caused security issues in the past.  In some cases it's even 
architecturally undefined what the processor does when an instruction 
input isn't properly extended (although at least in the MIPS case the 
required extension there is always sign-extension, whether the 32-bit 
value is considered signed or unsigned).

I'd actually hope for future forms of sanitization to be able to detect at 
runtime when you have undefined behavior from incorrectly typed arguments 
to variadic functions.

I think the warning is the correct thing for anyone trying to write C as a 
high-level language and be type-correct and properly check before doing 
conversions that might change values unless those changes are actually 
intended, and that warning rather than lesser warnings for "C as portable 
assembler" is the appropriate default for format checking.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 10/05/2016 10:27 AM, Joseph Myers wrote:

On Wed, 5 Oct 2016, Martin Sebor wrote:


The warning would only helpful if there was a target where wchar_t
didn't promote to a type with the same precision as wint_t, or in


Or it could show up a logical error where the code should have had a
wint_t variable and checked for WEOF somewhere but just assumed things
would fit in wchar_t


WEOF that fits in a wint_t also fits in a wchar_t when the two types
have the same precision, the case where GCC issues the warning (and
where WEOF expands to 0xu).  So this isn't an example of
a problem that could arise on the targets that brought this up, or
any other targets that I'm aware of.


(and so could be passing a negative value where the
ABI means the callee expects wint_t values to be zero-extended, with
consequent undefined behavior there).


Sign extension is also not an issue on the target where the warning
is issued (x86_64 ILP32) and where wchar_t is int and wint_t is
unsigned long.


The correct fix involves thinking
about where you know something is a wide character, and where something
might be a wide character or WEOF.  It might then involve a cast to
wint_t, or changing the type of a variable, or more changes than that.


There is nothing to fix when wchar_t and wint_t have the same
precision and  printf("%lc", L'x') does the same thing as
printf("%lc", (wint_t) L'x').  The cast is entirely superfluous.

Even so, the warning could have merit if there were a different
target to which the program might need to be ported where this
property didn't hold.  But as I said, I don't know of one and,
as it seems, neither do you.

I'm very much in favor of strict warnings that help point out
either real or potential bugs.  But I see no value in warning
about constructs that are perfectly safe.  To the contrary,
I think such warnings are harmful because they lead to
unnecessary churn and to bug injection.

It might be worthwhile to mention that Clang used to issue the
same warning as GCC until 2011 when they fixed it.  The bug is
here:

  https://llvm.org/bugs/show_bug.cgi?id=7981

(Although I think there is a flaw in the logic they used in
comment #2 it's only hypothetical because there is no target
that Clang supports where the submitted test case doesn't do
what it's expected to do either.)

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Joseph Myers]

On Wed, 5 Oct 2016, Martin Sebor wrote:

> The warning would only helpful if there was a target where wchar_t
> didn't promote to a type with the same precision as wint_t, or in

Or it could show up a logical error where the code should have had a 
wint_t variable and checked for WEOF somewhere but just assumed things 
would fit in wchar_t (and so could be passing a negative value where the 
ABI means the callee expects wint_t values to be zero-extended, with 
consequent undefined behavior there).  The correct fix involves thinking 
about where you know something is a wide character, and where something 
might be a wide character or WEOF.  It might then involve a cast to 
wint_t, or changing the type of a variable, or more changes than that.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 10/05/2016 09:40 AM, Joseph Myers wrote:

On Tue, 4 Oct 2016, Martin Sebor wrote:


Well, typically cases where one of long and int is passed and the other is
expected, but they have the same size, are bugs waiting to happen when the
code is built on a 64-bit system.  That is, they *should* warn.


Typically, yes.  In the case of wchar_t (or int) and wint_t I don't
think it's helpful.  Consider this case from my comment #5 on bug
72858.  I don't think there is any point in issuing a warning here.
On the majority of targets they either all are or promote to a type
of the same size, don't they?


I'm unconvinced by that "majority of targets" of argument (which once
would have been true for int and long, and you could say it is true for
long and size_t).  There's a clear correct type here, and it's wint_t, and
it's always easy to fix the code to use the correct type.


The warning would only helpful if there was a target where wchar_t
didn't promote to a type with the same precision as wint_t, or in
other words where printf("%lc", L'a') did something different than
printf("%lc", (wint_t) L'a'). I don't know of such a target.  Can
you give an example of one?

It isn't any of the ILP32 targets where wint_t is int and wchar_t
is long and where the warning was found to cause test suite failures,
and it isn't any other other targets where the warning didn't trigger
because the two types have the same precision.

Otherwise, when there is no such target there is nothing to fix, and
changing working code just for the sake of pedantry only runs the
unnecessary risk of introducing bugs.  IMO, this is especially true
in subtle cases like this where the people who make the changes often
don't fully appreciate the subtleties of the code.  Suggesting the
wrong directive (replacing %lc with %ld) compounds the problem
further(*).

Martin

[*] This is not meant as a dig at David but rather to underscore
that both the "problem" is subtle and the expected solution not
obvious.

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Joseph Myers]

On Wed, 5 Oct 2016, Jakub Jelinek wrote:

> On Wed, Oct 05, 2016 at 03:40:18PM +, Joseph Myers wrote:
> > On Tue, 4 Oct 2016, Martin Sebor wrote:
> > 
> > > > Well, typically cases where one of long and int is passed and the other 
> > > > is
> > > > expected, but they have the same size, are bugs waiting to happen when 
> > > > the
> > > > code is built on a 64-bit system.  That is, they *should* warn.
> > > 
> > > Typically, yes.  In the case of wchar_t (or int) and wint_t I don't
> > > think it's helpful.  Consider this case from my comment #5 on bug
> > > 72858.  I don't think there is any point in issuing a warning here.
> > > On the majority of targets they either all are or promote to a type
> > > of the same size, don't they?
> > 
> > I'm unconvinced by that "majority of targets" of argument (which once 
> > would have been true for int and long, and you could say it is true for 
> > long and size_t).  There's a clear correct type here, and it's wint_t, and 
> > it's always easy to fix the code to use the correct type.
> 
> But, can we reliably detect differences between wint_t and unsigned int if
> wint_t is a typedef to that, or size_t and unsigned long etc., I mean doesn't
> early folding already throw it away?

Detecting it in all cases is hard as for size_t etc., but that shouldn't 
stop us warning in the cases where the types are different enough that we 
can tell the program is wrong.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Jakub Jelinek]

On Wed, Oct 05, 2016 at 03:40:18PM +, Joseph Myers wrote:
> On Tue, 4 Oct 2016, Martin Sebor wrote:
> 
> > > Well, typically cases where one of long and int is passed and the other is
> > > expected, but they have the same size, are bugs waiting to happen when the
> > > code is built on a 64-bit system.  That is, they *should* warn.
> > 
> > Typically, yes.  In the case of wchar_t (or int) and wint_t I don't
> > think it's helpful.  Consider this case from my comment #5 on bug
> > 72858.  I don't think there is any point in issuing a warning here.
> > On the majority of targets they either all are or promote to a type
> > of the same size, don't they?
> 
> I'm unconvinced by that "majority of targets" of argument (which once 
> would have been true for int and long, and you could say it is true for 
> long and size_t).  There's a clear correct type here, and it's wint_t, and 
> it's always easy to fix the code to use the correct type.

But, can we reliably detect differences between wint_t and unsigned int if
wint_t is a typedef to that, or size_t and unsigned long etc., I mean doesn't
early folding already throw it away?

Jakub

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Joseph Myers]

On Tue, 4 Oct 2016, Martin Sebor wrote:

> > Well, typically cases where one of long and int is passed and the other is
> > expected, but they have the same size, are bugs waiting to happen when the
> > code is built on a 64-bit system.  That is, they *should* warn.
> 
> Typically, yes.  In the case of wchar_t (or int) and wint_t I don't
> think it's helpful.  Consider this case from my comment #5 on bug
> 72858.  I don't think there is any point in issuing a warning here.
> On the majority of targets they either all are or promote to a type
> of the same size, don't they?

I'm unconvinced by that "majority of targets" of argument (which once 
would have been true for int and long, and you could say it is true for 
long and size_t).  There's a clear correct type here, and it's wint_t, and 
it's always easy to fix the code to use the correct type.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 10/04/2016 06:21 PM, Joseph Myers wrote:

On Tue, 4 Oct 2016, Martin Sebor wrote:


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).


Well, typically cases where one of long and int is passed and the other is
expected, but they have the same size, are bugs waiting to happen when the
code is built on a 64-bit system.  That is, they *should* warn.


Typically, yes.  In the case of wchar_t (or int) and wint_t I don't
think it's helpful.  Consider this case from my comment #5 on bug
72858.  I don't think there is any point in issuing a warning here.
On the majority of targets they either all are or promote to a type
of the same size, don't they?

$ cat t.c && gcc -S -Wall -m32 t.c
typedef __WCHAR_TYPE__ wchar_t;

void f (const wchar_t *ws)
{
  __builtin_printf ("%lc", *ws);
}
t.c: In function ‘f’:
t.c:5:24: warning: format ‘%lc’ expects argument of type ‘wint_t’, but 
argument 2 has type ‘wchar_t {aka const long int}’ [-Wformat=]

   __builtin_printf ("%lc", *ws);
  ~~^   ~~~
  %ld


There have been arguments that we should go further and warn for e.g. %zu
with a type that happens to be the same as size_t but doesn't use the
size_t typedef (or sizeof etc.), %td for something that happens to be the
same as ptrdiff_t but doesn't use the typedef (or pointer difference
etc.), etc. - which would get many similar cases of bugs waiting to happen
on a different system, but is also tricker because you need to decide
whether a given type is logically size_t etc. or not - code could validly
use autoconf to identify the underlying type, or use __SIZE_TYPE__, or use
an expression mixing size_t with other types, or in the case of %j*
(intmax_t) use the INTMAX_C macro to construct constants.  That probably
*would* need an option to disable just those format warnings (whereas I
don't see the need for such an option for the case of mixing int and
long).


I would view it useful if GCC warned on %zu with an argument that's
not size_t, and similarly for other directives and typedefs, for the
reason you mention.  But I don't think the same rationale applies
to warning on %lc with wchar_t or int arguments.

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Joseph Myers]

On Tue, 4 Oct 2016, Martin Sebor wrote:

> 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).

Well, typically cases where one of long and int is passed and the other is 
expected, but they have the same size, are bugs waiting to happen when the 
code is built on a 64-bit system.  That is, they *should* warn.

There have been arguments that we should go further and warn for e.g. %zu 
with a type that happens to be the same as size_t but doesn't use the 
size_t typedef (or sizeof etc.), %td for something that happens to be the 
same as ptrdiff_t but doesn't use the typedef (or pointer difference 
etc.), etc. - which would get many similar cases of bugs waiting to happen 
on a different system, but is also tricker because you need to decide 
whether a given type is logically size_t etc. or not - code could validly 
use autoconf to identify the underlying type, or use __SIZE_TYPE__, or use 
an expression mixing size_t with other types, or in the case of %j* 
(intmax_t) use the INTMAX_C macro to construct constants.  That probably 
*would* need an option to disable just those format warnings (whereas I 
don't see the need for such an option for the case of mixing int and 
long).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

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!

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Rainer Orth]

Hi Martin,

 +FAIL: gcc.dg/tree-ssa/builtin-sprintf.c execution test

 FAIL: test_a_double:364: "%a" expected result for "0x0.0p+0"
 doesn't match function call return value: 20 != 6
 FAIL: test_a_double:365: "%a" expected result for "0x1.0p+0"
 doesn't match function call return value: 20 != 6
 FAIL: test_a_double:366: "%a" expected result for "0x1.0p+1"
 doesn't match function call return value: 20 != 6

 FAIL: test_a_long_double:375: "%La" expected result for
 "0x0.p+0" doesn't match function call return
 value: 35 != 6
 FAIL: test_a_long_double:376: "%La" expected result for
 "0x1.p+0" doesn't match function call return
 value: 35 != 6
 FAIL: test_a_long_double:377: "%La" expected result for
 "0x1.p+1" doesn't match function call return
 value: 35 != 6
>>>
>>> I don't know about these.  It looks like the Solaris printf doesn't
>>> handle the %a directive correctly and the tests (and the related
>>> checks/optimization) might need to be disabled, which in turn might
>>> involve extending the existing printf hook or adding a new one.
>>
>> I've found the following in Solaris 10 (and up) printf(3C):
>>
>>  a, AA  double  argument  representing  a  floating-point
>>  number  is  converted in the style "[-]0xh.p+d",
>>  where the single  hexadecimal  digit  preceding  the
>>  radix  point is 0 if the value converted is zero and
>>  1 otherwise and the  number  of  hexadecimal  digits
>>  after it is equal to the precision; if the precision
>>  is missing, the number of digits printed  after  the
>>  radix  point  is  13  for the conversion of a double
>>  value, 16 for the conversion of a long double  value
>>  on  x86,  and 28 for the conversion of a long double
>>  value on SPARC; if the precision is zero and the '#'
>>  flag  is  not  specified, no decimal-point character
>>  will appear. The letters "abcdef"  are  used  for  a
>>  conversion  and  the  letters "ABCDEF" for A conver-
>>  sion. The A conversion specifier produces  a  number
>>  with  'X'  and  'P'  instead  of  'x'  and  'p'. The
>>  exponent will always contain at least one digit, and
>>  only  as  many more digits as necessary to represent
>>  the decimal exponent of 2. If the value is zero, the
>>  exponent is zero.
>>
>>  The converted value is rounded to fit the  specified
>>  output  format  according to the prevailing floating
>>  point rounding direction mode. If the conversion  is
>>  not exact, an inexact exception is raised.
>>
>>  A double argument representing an infinity or NaN is
>>  converted in the SUSv3 style of an e or E conversion
>>  specifier.
>>
>> I tried to check the relevant sections of the latest C99 and C11 drafts
>> to check if this handling of missing precision is allowed by the
>> standard, but I couldn't even fully parse the language there.
>>
>>> I don't have access to Solaris to fully debug and test this there.
>>> Would you mind helping with it?
>>
>> Not at all: if it turns out that Solaris has bugs in this area, I can
>> easily file them, too.
>
> I think it's actually a defect in the C standard.  It doesn't
> specify how many decimal digits an implementation must produce
> on output for a plain %a directive (i.e., when precision isn't
> explicitly specified).  With Glibc, for instance, printf("%a",
> 1.0) prints 0x8p-3 while on Solaris it prints  0x8.00p-3.
> Both seem reasonable but neither is actually specified.  In
> theory, an implementation is allowed print any number of zeros
> after the decimal point, which the standard should (IMO) not
> permit.  There should be a cap (e.g., of at most 6 decimal
> digits when precision is not specified with %a, just like
> there us with %e).  I'll propose to change the standard and
> forward it to the C committee.  Until then, I've worked
> around it in the patch for pr77735 (under review).  If you
> have a moment and could try it out on Solaris and let me
> know how it goes I'd be grateful.

as it happens, I'd already started bootstraps with your patch before
your mail arrived :-)

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:

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

+FAIL: gcc.dg/tree-ssa/builtin-sprintf.c execution test

FAIL: test_a_double:364: "%a" expected result for "0x0.0p+0"
doesn't match function call return value: 20 != 6
FAIL: test_a_double:365: "%a" expected result for "0x1.0p+0"
doesn't match function call return value: 20 != 6
FAIL: test_a_double:366: "%a" expected result for "0x1.0p+1"
doesn't match function call return value: 20 != 6

FAIL: test_a_long_double:375: "%La" expected result for
"0x0.p+0" doesn't match function call return
value: 35 != 6
FAIL: test_a_long_double:376: "%La" expected result for
"0x1.p+0" doesn't match function call return
value: 35 != 6
FAIL: test_a_long_double:377: "%La" expected result for
"0x1.p+1" doesn't match function call return
value: 35 != 6


I don't know about these.  It looks like the Solaris printf doesn't
handle the %a directive correctly and the tests (and the related
checks/optimization) might need to be disabled, which in turn might
involve extending the existing printf hook or adding a new one.


I've found the following in Solaris 10 (and up) printf(3C):

 a, AA  double  argument  representing  a  floating-point
 number  is  converted in the style "[-]0xh.p+d",
 where the single  hexadecimal  digit  preceding  the
 radix  point is 0 if the value converted is zero and
 1 otherwise and the  number  of  hexadecimal  digits
 after it is equal to the precision; if the precision
 is missing, the number of digits printed  after  the
 radix  point  is  13  for the conversion of a double
 value, 16 for the conversion of a long double  value
 on  x86,  and 28 for the conversion of a long double
 value on SPARC; if the precision is zero and the '#'
 flag  is  not  specified, no decimal-point character
 will appear. The letters "abcdef"  are  used  for  a
 conversion  and  the  letters "ABCDEF" for A conver-
 sion. The A conversion specifier produces  a  number
 with  'X'  and  'P'  instead  of  'x'  and  'p'. The
 exponent will always contain at least one digit, and
 only  as  many more digits as necessary to represent
 the decimal exponent of 2. If the value is zero, the
 exponent is zero.

 The converted value is rounded to fit the  specified
 output  format  according to the prevailing floating
 point rounding direction mode. If the conversion  is
 not exact, an inexact exception is raised.

 A double argument representing an infinity or NaN is
 converted in the SUSv3 style of an e or E conversion
 specifier.

I tried to check the relevant sections of the latest C99 and C11 drafts
to check if this handling of missing precision is allowed by the
standard, but I couldn't even fully parse the language there.


I don't have access to Solaris to fully debug and test this there.
Would you mind helping with it?


Not at all: if it turns out that Solaris has bugs in this area, I can
easily file them, too.


I think it's actually a defect in the C standard.  It doesn't
specify how many decimal digits an implementation must produce
on output for a plain %a directive (i.e., when precision isn't
explicitly specified).  With Glibc, for instance, printf("%a",
1.0) prints 0x8p-3 while on Solaris it prints  0x8.00p-3.
Both seem reasonable but neither is actually specified.  In
theory, an implementation is allowed print any number of zeros
after the decimal point, which the standard should (IMO) not
permit.  There should be a cap (e.g., of at most 6 decimal
digits when precision is not specified with %a, just like
there us with %e).  I'll propose to change the standard and
forward it to the C committee.  Until then, I've worked
around it in the patch for pr77735 (under review).  If you
have a moment and could try it out on Solaris and let me
know how it goes I'd be grateful.

Thanks
Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 09/24/2016 10:39 AM, Andreas Schwab wrote:

I'm still seeing these failures on m68k:

FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 358)
FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1222)
FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1272)
FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1383)
FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test for excess errors)
Excess errors:
/daten/aranym/gcc/gcc-20160924/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1408:3:
 warning: specified size 4294967295 exceeds the size 2 of the destination 
object [-Wformat-length=]


I'm pretty sure the first two are due to the same problem as bug 77735.
I plan to look into fixing it today or later this week.  The last two
were caused by bug v77762 and should be fixed by now.

Martin

Re: [BUILDROBOT] tic6x-uclinux: undefined reference to `gnu_libc_printf_pointer_format(tree_node*, char const**)' (was: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905))

[Segher Boessenkool]

On Tue, Sep 27, 2016 at 12:08:46AM +0200, Jan-Benedict Glaw wrote:
> Hi Martin,
> 
> On Thu, 2016-09-08 13:03:12 -0600, Martin Sebor  wrote:
> > Attached is another update to the patch to address the last round
> > of comments and suggestions, most notably to:
> [...]
> 
> with the currently committed version, the tic6x-uclinux target fails,
> see ie.
> http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=630308 :

[ snip ]

The same happens for bfin-uclinux:

g++ -no-pie   -g -O2 -DIN_GCC  -DCROSS_DIRECTORY_STRUCTURE   -fno-exceptions 
-fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings 
-Wcast-qual -Wmissing-format-attribute -Woverloaded-virtual -pedantic 
-Wno-long-long -Wno-variadic-macros -Wno-overlength-strings   -DHAVE_CONFIG_H 
-static-libstdc++ -static-libgcc  -o cc1 c/c-lang.o c-family/stub-objc.o 
attribs.o c/c-errors.o c/c-decl.o c/c-typeck.o c/c-convert.o c/c-aux-info.o 
c/c-objc-common.o c/c-parser.o c/c-array-notation.o c/c-fold.o 
c-family/c-common.o c-family/c-cppbuiltin.o c-family/c-dump.o 
c-family/c-format.o c-family/c-gimplify.o c-family/c-indentation.o 
c-family/c-lex.o c-family/c-omp.o c-family/c-opts.o c-family/c-pch.o 
c-family/c-ppoutput.o c-family/c-pragma.o c-family/c-pretty-print.o 
c-family/c-semantics.o c-family/c-ada-spec.o c-family/c-cilkplus.o 
c-family/array-notation-common.o c-family/cilk.o c-family/c-ubsan.o default-c.o 
\
  cc1-checksum.o libbackend.a main.o libcommon-target.a libcommon.a 
../libcpp/libcpp.a ../libdecnumber/libdecnumber.a libcommon.a 
../libcpp/libcpp.a   ../libbacktrace/.libs/libbacktrace.a 
../libiberty/libiberty.a ../libdecnumber/libdecnumber.a   -lmpc -lmpfr -lgmp 
-rdynamic -ldl  -L./../zlib -lz
bfin.o:(.data.rel+0x778): undefined reference to 
`gnu_libc_printf_pointer_format(tree_node*, char const**)'

(that symbol would be defined in linux.o, but that file is not built for
uclinux).


Segher

[BUILDROBOT] tic6x-uclinux: undefined reference to `gnu_libc_printf_pointer_format(tree_node*, char const**)' (was: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905))

[Jan-Benedict Glaw]

Hi Martin,

On Thu, 2016-09-08 13:03:12 -0600, Martin Sebor  wrote:
> Attached is another update to the patch to address the last round
> of comments and suggestions, most notably to:
[...]

with the currently committed version, the tic6x-uclinux target fails,
see ie.
http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=630308 :

g++-g -O2 -DIN_GCC  -DCROSS_DIRECTORY_STRUCTURE   -fno-exceptions -fno-rtti 
-fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings 
-Wcast-qual -Wmissing-format-attribute -Woverloaded-virtual -pedantic 
-Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -fno-common  
-DHAVE_CONFIG_H -static-libstdc++ -static-libgcc  -o cc1 c/c-lang.o 
c-family/stub-objc.o attribs.o c/c-errors.o c/c-decl.o c/c-typeck.o 
c/c-convert.o c/c-aux-info.o c/c-objc-common.o c/c-parser.o 
c/c-array-notation.o c/c-fold.o c-family/c-common.o c-family/c-cppbuiltin.o 
c-family/c-dump.o c-family/c-format.o c-family/c-gimplify.o 
c-family/c-indentation.o c-family/c-lex.o c-family/c-omp.o c-family/c-opts.o 
c-family/c-pch.o c-family/c-ppoutput.o c-family/c-pragma.o 
c-family/c-pretty-print.o c-family/c-semantics.o c-family/c-ada-spec.o 
c-family/c-cilkplus.o c-family/array-notation-common.o c-family/cilk.o 
c-family/c-ubsan.o default-c.o \
  cc1-checksum.o libbackend.a main.o libcommon-target.a libcommon.a 
../libcpp/libcpp.a ../libdecnumber/libdecnumber.a libcommon.a 
../libcpp/libcpp.a   ../libbacktrace/.libs/libbacktrace.a 
../libiberty/libiberty.a ../libdecnumber/libdecnumber.a   -lmpc -lmpfr -lgmp 
-rdynamic -ldl  -L./../zlib -lz
c6x.o:(.data+0x778): undefined reference to 
`gnu_libc_printf_pointer_format(tree_node*, char const**)'
collect2: error: ld returned 1 exit status
/home/jbglaw/repos/gcc/gcc/c/Make-lang.in:84: recipe for target 'cc1' failed
make[1]: *** [cc1] Error 1
make[1]: Leaving directory '/home/jbglaw/build/tic6x-uclinux/build-gcc/gcc'
Makefile:4252: recipe for target 'all-gcc' failed
make: *** [all-gcc] Error 2


Add another one for uClibc?

MfG, JBG

-- 
  Jan-Benedict Glaw  jbg...@lug-owl.de  +49-172-7608481
  Signature of:  Zensur im Internet? Nein danke!
  the second  :


signature.asc
Description: Digital signature

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Andreas Schwab]

I'm still seeing these failures on m68k:

FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 358)
FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1222)
FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1272)
FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1383)
FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test for excess errors)
Excess errors:
/daten/aranym/gcc/gcc-20160924/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1408:3:
 warning: specified size 4294967295 exceeds the size 2 of the destination 
object [-Wformat-length=]

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Rainer Orth]

Hi Martin,

> On 09/22/2016 06:14 AM, Rainer Orth wrote:
>> Hi Martin,
>>
>>> your patch broke bootstrap with MPFR 2.4.2, which is still the
>>> recommended (or perhaps minimal) version according to install.texi:
>> [...]
>>> The following patch (together with your other one to fix ILP32 targets)
>>> allows a sparc-sun-solaris2.12 bootstrap to continue.  I'm going to
>>> commit it as obvious.
>>
>> done now.  Once the bootstrap had finished, I see quite a number of
>> testsuite failures (i386-pc-solaris2.12 still running), both 32 and
>> 64-bit:
>>
>> +FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test for warnings, line
>> 1220)
>> +FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test for warnings, line
>> 1270)
>> +FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test for warnings, line
>> 1381)
>> +FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 
>> 356)
>> +FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 99)
>> +FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test for excess errors)
>
> I have a patch for (hopefully) most of these failures that I will
> commit along with the one for pr77676 as soon as it's approved.

as of r240389, a few of those failures occur also on x86_64-pc-linux-gnu
with -m32:

FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1222)
FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1272)
FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1383)
FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test for excess errors)

>> +FAIL: gcc.dg/tree-ssa/builtin-sprintf.c execution test
>>
>> FAIL: test_a_double:364: "%a" expected result for "0x0.0p+0"
>> doesn't match function call return value: 20 != 6
>> FAIL: test_a_double:365: "%a" expected result for "0x1.0p+0"
>> doesn't match function call return value: 20 != 6
>> FAIL: test_a_double:366: "%a" expected result for "0x1.0p+1"
>> doesn't match function call return value: 20 != 6
>>
>> FAIL: test_a_long_double:375: "%La" expected result for
>> "0x0.p+0" doesn't match function call return
>> value: 35 != 6
>> FAIL: test_a_long_double:376: "%La" expected result for
>> "0x1.p+0" doesn't match function call return
>> value: 35 != 6
>> FAIL: test_a_long_double:377: "%La" expected result for
>> "0x1.p+1" doesn't match function call return
>> value: 35 != 6
>
> I don't know about these.  It looks like the Solaris printf doesn't
> handle the %a directive correctly and the tests (and the related
> checks/optimization) might need to be disabled, which in turn might
> involve extending the existing printf hook or adding a new one.

I've found the following in Solaris 10 (and up) printf(3C):

 a, AA  double  argument  representing  a  floating-point
 number  is  converted in the style "[-]0xh.p+d",
 where the single  hexadecimal  digit  preceding  the
 radix  point is 0 if the value converted is zero and
 1 otherwise and the  number  of  hexadecimal  digits
 after it is equal to the precision; if the precision
 is missing, the number of digits printed  after  the
 radix  point  is  13  for the conversion of a double
 value, 16 for the conversion of a long double  value
 on  x86,  and 28 for the conversion of a long double
 value on SPARC; if the precision is zero and the '#'
 flag  is  not  specified, no decimal-point character
 will appear. The letters "abcdef"  are  used  for  a
 conversion  and  the  letters "ABCDEF" for A conver-
 sion. The A conversion specifier produces  a  number
 with  'X'  and  'P'  instead  of  'x'  and  'p'. The
 exponent will always contain at least one digit, and
 only  as  many more digits as necessary to represent
 the decimal exponent of 2. If the value is zero, the
 exponent is zero.

 The converted value is rounded to fit the  specified
 output  format  according to the prevailing floating
 point rounding direction mode. If the conversion  is
 not exact, an inexact exception is raised.

 A double argument representing an infinity or NaN is
 converted in the SUSv3 style of an e or E conversion
 specifier.

I tried to check the relevant sections of the latest C99 and C11 drafts
to check if this handling of missing precision is allowed by the
standard, but I couldn't even fully parse the language there.

> I don't have access to Solaris to fully debug and test this there.
> Would you mind helping with it?

Not at all: if it turns out that Solaris has bugs in this area, I can
easily file them, too.

Thanks.
Rainer

-- 

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 09/22/2016 06:14 AM, Rainer Orth wrote:

Hi Martin,


your patch broke bootstrap with MPFR 2.4.2, which is still the
recommended (or perhaps minimal) version according to install.texi:

[...]

The following patch (together with your other one to fix ILP32 targets)
allows a sparc-sun-solaris2.12 bootstrap to continue.  I'm going to
commit it as obvious.


done now.  Once the bootstrap had finished, I see quite a number of
testsuite failures (i386-pc-solaris2.12 still running), both 32 and
64-bit:

+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1220)
+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1270)
+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1381)
+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 356)
+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 99)
+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test for excess errors)


I have a patch for (hopefully) most of these failures that I will
commit along with the one for pr77676 as soon as it's approved.


Excess errors:
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:209:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 5 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:210:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 5 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:211:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 5 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:211: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:212:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 5 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:213:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 5 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:213: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:1238:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 4 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1239:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 4 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1240:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 4 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1287: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:1288: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:1289: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:99:3:
 warning: '%p' directive writing 1 byte into a region of size 0 
[-Wformat-length=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1406:3:
 warning: specified size 4294967295 exceeds the size 2 of the destination 
object [-Wformat-length=]

+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-2.c  (test for warnings, line 50)
+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-2.c sprintf transformed into strcpy
(test for warnings, line 83)
+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-2.c sprintf transformed into strcpy
(test for warnings, line 84)

+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-4.c (test for excess errors)

Excess errors:
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c:10:21:
 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:22:22:
 warning: '%-s' directive writing 4 bytes into a region of size 1 
[-Wformat-length=]

+FAIL: gcc.dg/tree-ssa/builtin-sprintf.c execution test

FAIL: test_a_double:364: "%a" expected result for "0x0.0p+0" 
doesn't match function call return value: 20 != 6
FAIL: test_a_double:365: "%a" expected result for "0x1.0p+0" 
doesn't match function 

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

your patch broke bootstrap with MPFR 2.4.2, which is still the
recommended (or perhaps minimal) version according to install.texi:

/vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c: In function 'int 
{anonymous}::format_floating_max(tree, char)':
/vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c:1128:27: error: 
'MPFR_RNDN' was not declared in this scope
   mpfr_from_real (x, , MPFR_RNDN);
   ^
/vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c: In function 
'{anonymous}::fmtresult {anonymous}::format_floating(const 
{anonymous}::conversion_spec&, tree)':
/vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c:1328:37: error: 
'MPFR_RNDN' was not declared in this scope
   mpfr_from_real (mpfrval, rvp, MPFR_RNDN);
 ^

MPFR_RNDN was only introduced in mpfr 3.0.0, and everywhere else in gcc
GMP_RNDN is used instead.  mpfr 3.0.0  has

/* kept for compatibility with MPFR 2.4.x and before */
#define GMP_RNDN MPFR_RNDN

The following patch (together with your other one to fix ILP32 targets)
allows a sparc-sun-solaris2.12 bootstrap to continue.  I'm going to
commit it as obvious.


Thanks.  I didn't realize the older MPFR was still recommended.
FWIW, I was using MPFR 2.4 until recently but after running into
some (unexplained) runtime issues I reran the download_prerequisites
script to update them and it installed 3.1.4.

It seems that either the recommended MPFR version should be bumped
up (or the script downgraded to 2.4 to avoid this risk, though I
suspect there are reasons why it was updated to the latest).

Based on the log the script was updated from MPFR 2.4 to 3.0 in
r235763.  The change also updated install.texi but not the MPFR
version mentioned there.  It might be worth checking with the
author, Bernd Edlinger, to see how to resolve this.  Let me ping
him in a separate thread.

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Rainer Orth]

Hi Martin,

> your patch broke bootstrap with MPFR 2.4.2, which is still the
> recommended (or perhaps minimal) version according to install.texi:
[...]
> The following patch (together with your other one to fix ILP32 targets)
> allows a sparc-sun-solaris2.12 bootstrap to continue.  I'm going to
> commit it as obvious.

done now.  Once the bootstrap had finished, I see quite a number of
testsuite failures (i386-pc-solaris2.12 still running), both 32 and
64-bit:

+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1220)
+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1270)
+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1381)
+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 356)
+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 99)
+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test for excess errors)

Excess errors:
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:209:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 5 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:210:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 5 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:211:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 5 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:211: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:212:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 5 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:213:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 5 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:213: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:1238:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 4 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1239:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 4 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1240:3:
 warning: format '%lc' expects argument of type 'wint_t', but argument 4 has 
type 'int' [-Wformat=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1287: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:1288: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:1289: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:99:3:
 warning: '%p' directive writing 1 byte into a region of size 0 
[-Wformat-length=]
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c:1406:3:
 warning: specified size 4294967295 exceeds the size 2 of the destination 
object [-Wformat-length=]

+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-2.c  (test for warnings, line 50)
+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-2.c sprintf transformed into strcpy 
(test for warnings, line 83)
+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-2.c sprintf transformed into strcpy 
(test for warnings, line 84)

+FAIL: gcc.dg/tree-ssa/builtin-sprintf-warn-4.c (test for excess errors)

Excess errors:
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c:10:21:
 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:22:22:
 warning: '%-s' directive writing 4 bytes into a region of size 1 
[-Wformat-length=]

+FAIL: gcc.dg/tree-ssa/builtin-sprintf.c execution test

FAIL: test_a_double:364: "%a" expected result for "0x0.0p+0" 
doesn't match function call return value: 20 != 6
FAIL: test_a_double:365: "%a" expected result for "0x1.0p+0" 
doesn't match function call return value: 20 != 6
FAIL: test_a_double:366: "%a" expected result for "0x1.0p+1" 
doesn't match function call return value: 20 != 6

FAIL: 

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Rainer Orth]

Hi Martin,

>> Another nit, if I may: FWIW I'm not in love with the wording of the
>> messages.  Sorry to bikeshed, but how about:
>>   warning: buffer overflow will occur when writing terminating NUL
>> and:
>>   note: formatted output of 2 bytes into a destination of size 1
>> or somesuch.
>
> I won't claim the text of the messages is perfect but I did spend
> a lot of time tweaking them.  That's not to say they can't be
> improved but changing them means quite a bit of work adjusting
> the tests.  At this point, I'd like to focus on getting the patch
> committed.  After that, if there's still time, I'm happy to take
> feedback and tweak the diagnostics based on it.
>
> Thanks again for your help and the suggestions above!

your patch broke bootstrap with MPFR 2.4.2, which is still the
recommended (or perhaps minimal) version according to install.texi:

/vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c: In function 'int 
{anonymous}::format_floating_max(tree, char)':
/vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c:1128:27: error: 
'MPFR_RNDN' was not declared in this scope
   mpfr_from_real (x, , MPFR_RNDN);
   ^
/vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c: In function 
'{anonymous}::fmtresult {anonymous}::format_floating(const 
{anonymous}::conversion_spec&, tree)':
/vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c:1328:37: error: 
'MPFR_RNDN' was not declared in this scope
   mpfr_from_real (mpfrval, rvp, MPFR_RNDN);
 ^

MPFR_RNDN was only introduced in mpfr 3.0.0, and everywhere else in gcc
GMP_RNDN is used instead.  mpfr 3.0.0  has 

/* kept for compatibility with MPFR 2.4.x and before */
#define GMP_RNDN MPFR_RNDN

The following patch (together with your other one to fix ILP32 targets)
allows a sparc-sun-solaris2.12 bootstrap to continue.  I'm going to
commit it as obvious.

Rainer


2016-09-22  Rainer Orth  

gcc:
* gimple-ssa-sprintf.c (format_floating_max): Use GMP_RNDN instead
of MPFR_RNDN.
(format_floating): Likewise.

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -1125,7 +1127,7 @@ format_floating_max (tree type, char spe
  round-to-nearest mode.  */
   mpfr_t x;
   mpfr_init2 (x, rfmt->p);
-  mpfr_from_real (x, , MPFR_RNDN);
+  mpfr_from_real (x, , GMP_RNDN);
 
   const char fmt[] = { '%', 'R', spec, '\0' };
   int n = mpfr_snprintf (NULL, 0, fmt, x);
@@ -1325,7 +1327,7 @@ format_floating (const conversion_spec &
 	 round-to-nearest mode.  */
   mpfr_t mpfrval;
   mpfr_init2 (mpfrval, rfmt->p);
-  mpfr_from_real (mpfrval, rvp, MPFR_RNDN);
+  mpfr_from_real (mpfrval, rvp, GMP_RNDN);
 
   char fmtstr [40];
   char *pfmt = fmtstr;

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Gerald Pfeifer]

On Wed, 21 Sep 2016, Martin Sebor wrote:
>> Is it possible that is related to your warning patches?
> Yes, this is likely the same bug as mentioned in comment #6 on
> pr77676.  The bug in the comment ILP32-specific and only tangentially
> related to the PR itself.  I'm testing the patch that's attached to
> the PR that should fix both of these problems.  I don't have access
> to i?86-unknown-freebsd so if you could help validate it there I'd
> be grateful.

Yes, with this patch I was able to bootstrap i?86-unknown-freebsd11
again.

Thanks for looking into this so quickly, Martin!

Gerald

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Jakub Jelinek]

On Wed, Sep 21, 2016 at 01:55:33PM -0600, Martin Sebor wrote:
> On 09/21/2016 01:40 PM, Gerald Pfeifer wrote:
> >I noticed the following bootstrap failure on i?86-unknown-freebsd
> >that started in the last 24 hours:
> >
> >/scratch/tmp/gerald/gcc-HEAD/gcc/vec.c: In member function ‘void 
> >vec_usage::dump(mem_location*, mem_usage&) const’:
> >/scratch/tmp/gerald/gcc-HEAD/gcc/vec.c:79:3: error: ‘%s’ directive writing 
> >between 0 and 4294967295 bytes into a region of size 4096 
> >[-Werror=format-length=]
> >   dump (mem_location *loc, mem_usage ) const
> >   ^~~~
> >/scratch/tmp/gerald/gcc-HEAD/gcc/vec.c:83:36: note: format output between 6 
> >and4294967311 bytes into a destination of size 4096
> >   loc->m_line, loc->m_function);
> >^
> >cc1plus: all warnings being treated as errors
> >gmake[3]: *** [Makefile:2557: build/vec.o] Error 1
> >gmake[3]: Leaving directory '/scratch/tmp/gerald/OBJ-0921-1705/gcc'
> >gmake[2]: *** [Makefile:4612: all-stage2-gcc] Error 2
> >gmake[2]: Leaving directory '/scratch/tmp/gerald/OBJ-0921-1705'
> >gmake[1]: *** [Makefile:24365: stage2-bubble] Error 2
> >
> >Is it possible that is related to your warning patches?
> 
> Yes, this is likely the same bug as mentioned in comment #6 on
> pr77676.  The bug in the comment ILP32-specific and only tangentially
> related to the PR itself.  I'm testing the patch that's attached to
> the PR that should fix both of these problems.  I don't have access
> to i?86-unknown-freebsd so if you could help validate it there I'd
> be grateful.  (The patch just successfully bootstrapped on
> i386-pc-linux-gnu.)

Looking at target_int_max you are using in the new patch:
static unsigned HOST_WIDE_INT
target_int_max ()
{
  static const unsigned HOST_WIDE_INT int_max
= HOST_WIDE_INT_M1U >> (sizeof int_max * CHAR_BIT
- TYPE_PRECISION (integer_type_node) + 1);
  return int_max;
}

1) sizeof int_max * CHAR_BIT should IMNSHO be HOST_BITS_PER_WIDE_INT
2) why is the var static, subtraction and shift is very cheap, while C++
   local statics are expensive?  It needs a guard variable,
   __cxa_guard_acquire, __cxa_guard_release calls, etc.

Jakub

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 09/21/2016 01:40 PM, Gerald Pfeifer wrote:

I noticed the following bootstrap failure on i?86-unknown-freebsd
that started in the last 24 hours:

/scratch/tmp/gerald/gcc-HEAD/gcc/vec.c: In member function ‘void 
vec_usage::dump(mem_location*, mem_usage&) const’:
/scratch/tmp/gerald/gcc-HEAD/gcc/vec.c:79:3: error: ‘%s’ directive writing 
between 0 and 4294967295 bytes into a region of size 4096 
[-Werror=format-length=]
   dump (mem_location *loc, mem_usage ) const
   ^~~~
/scratch/tmp/gerald/gcc-HEAD/gcc/vec.c:83:36: note: format output between 6 
and4294967311 bytes into a destination of size 4096
   loc->m_line, loc->m_function);
^
cc1plus: all warnings being treated as errors
gmake[3]: *** [Makefile:2557: build/vec.o] Error 1
gmake[3]: Leaving directory '/scratch/tmp/gerald/OBJ-0921-1705/gcc'
gmake[2]: *** [Makefile:4612: all-stage2-gcc] Error 2
gmake[2]: Leaving directory '/scratch/tmp/gerald/OBJ-0921-1705'
gmake[1]: *** [Makefile:24365: stage2-bubble] Error 2

Is it possible that is related to your warning patches?


Yes, this is likely the same bug as mentioned in comment #6 on
pr77676.  The bug in the comment ILP32-specific and only tangentially
related to the PR itself.  I'm testing the patch that's attached to
the PR that should fix both of these problems.  I don't have access
to i?86-unknown-freebsd so if you could help validate it there I'd
be grateful.  (The patch just successfully bootstrapped on
i386-pc-linux-gnu.)

Thanks and sorry for the breakage.
Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Gerald Pfeifer]

I noticed the following bootstrap failure on i?86-unknown-freebsd
that started in the last 24 hours:

/scratch/tmp/gerald/gcc-HEAD/gcc/vec.c: In member function ‘void 
vec_usage::dump(mem_location*, mem_usage&) const’:
/scratch/tmp/gerald/gcc-HEAD/gcc/vec.c:79:3: error: ‘%s’ directive writing 
between 0 and 4294967295 bytes into a region of size 4096 
[-Werror=format-length=]
   dump (mem_location *loc, mem_usage ) const
   ^~~~
/scratch/tmp/gerald/gcc-HEAD/gcc/vec.c:83:36: note: format output between 6 
and4294967311 bytes into a destination of size 4096
   loc->m_line, loc->m_function);
^
cc1plus: all warnings being treated as errors
gmake[3]: *** [Makefile:2557: build/vec.o] Error 1
gmake[3]: Leaving directory '/scratch/tmp/gerald/OBJ-0921-1705/gcc'
gmake[2]: *** [Makefile:4612: all-stage2-gcc] Error 2
gmake[2]: Leaving directory '/scratch/tmp/gerald/OBJ-0921-1705'
gmake[1]: *** [Makefile:24365: stage2-bubble] Error 2

Is it possible that is related to your warning patches?

Gerald

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Pedro Alves]

On 09/21/2016 03:59 PM, Szabolcs Nagy wrote:
> On 21/09/16 15:37, Alexander Monakov wrote:
>> On Wed, 21 Sep 2016, Martin Sebor wrote:
>>> On 09/21/2016 01:11 AM, Markus Trippelsdorf wrote:
 The patch uses "nul" instead of "null" throughout.
>>>
>>> Yes, that's intentional.  NUL and null are alternate spellings for
>>> the same character. I went with nul to distinguish it from the null
>>> pointer and used all lowercase as per the GCC convention.
>>
>> Can you elaborate which guideline suggests spelling that in lowercase?
> 
> the c standard calls it "null character".

*nod*  

The character is called "NUL" just much as I'm called "palves".  :-)

I.e., three-letter uppercase "NUL" is a control character
name abbreviation, just like, ACK, LF, DEL, ESC, etc.

The original ASCII standard used "NULL" for abbreviation, and then
in later revisions, it was shortened to "NUL".  All control
characters have 2 or 3 letter abbreviations in the latest ASCII
standard, and are written in uppercase, which to me looks like
the obvious reason for "NUL" with single "L".

https://en.wikipedia.org/wiki/Control_character
https://en.wikipedia.org/wiki/ASCII

https://web.archive.org/web/20160605145632/http://worldpowersystems.com/archives/codes/X3.4-1963/page5.JPG
https://web.archive.org/web/20160613203624/https://tools.ietf.org/html/rfc20

>From the latter:

~~~
[...]
5.2 Control Characters

  NUL (Null): The all-zeros character which may serve to accomplish
   time fill and media fill.
  SOH (Start of Heading): A communication control character used at
   the beginning of a sequence of characters which constitute a
[...]
~~~

> 
>> It seems quite strange to me, especially given that the documentation
>> added with the patch uses "NUL character" (which I believe to be a more
>> common form), but then warnings use "nul" (without the "character" iiuc).
>>

To me too.  Lowercase "nul" has all the looks of a typo to me.
I'd only use lowercase like that if abbreviations of other characters
were lowercase too in similar contexts, like "... the esc character ...",
etc.

Thanks,
Pedro Alves

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Jeff Law]

On 09/21/2016 01:11 AM, Markus Trippelsdorf wrote:

On 2016.09.11 at 20:03 -0600, Martin Sebor wrote:

On 09/08/2016 04:10 PM, Joseph Myers wrote:

On Thu, 8 Sep 2016, Martin Sebor wrote:



PR middle-end/49905 - Better sanity checking on sprintf src & dest to
produce warning for dodgy code


The patch uses "nul" instead of "null" throughout.
It was referring to a character rather than a null pointer.  That's why 
I didn't call it out.


jeff

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 09/21/2016 08:37 AM, Alexander Monakov wrote:

On Wed, 21 Sep 2016, Martin Sebor wrote:

On 09/21/2016 01:11 AM, Markus Trippelsdorf wrote:

The patch uses "nul" instead of "null" throughout.


Yes, that's intentional.  NUL and null are alternate spellings for
the same character. I went with nul to distinguish it from the null
pointer and used all lowercase as per the GCC convention.


Can you elaborate which guideline suggests spelling that in lowercase?
It seems quite strange to me, especially given that the documentation
added with the patch uses "NUL character" (which I believe to be a more
common form), but then warnings use "nul" (without the "character" iiuc).


I don't know if there is a specific guideline that applies here,
but it's my impression that conventionally GCC messages use
lowercase letters.  The only guideline I know about is in the GNU
Coding Standard that calls for messages not to begin with capital
letters (or end in a period;  though now that I've checked more
carefully there are a fair number of messages that ignore that
suggestion).

That said, if the lowercase nul is offensive it can certainly be
changed to some other spelling (as can any of the other messages
emitted by the pass).

Martin

PS Yes, the C and C++ standards refer to NUL as the null character.
NUL is an abbreviation, used extensively (for example) by POSIX.

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Szabolcs Nagy]

On 21/09/16 15:37, Alexander Monakov wrote:
> On Wed, 21 Sep 2016, Martin Sebor wrote:
>> On 09/21/2016 01:11 AM, Markus Trippelsdorf wrote:
>>> The patch uses "nul" instead of "null" throughout.
>>
>> Yes, that's intentional.  NUL and null are alternate spellings for
>> the same character. I went with nul to distinguish it from the null
>> pointer and used all lowercase as per the GCC convention.
> 
> Can you elaborate which guideline suggests spelling that in lowercase?

the c standard calls it "null character".

> It seems quite strange to me, especially given that the documentation
> added with the patch uses "NUL character" (which I believe to be a more
> common form), but then warnings use "nul" (without the "character" iiuc).
> 
> Thanks.
> Alexander
> 

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Alexander Monakov]

On Wed, 21 Sep 2016, Martin Sebor wrote:
> On 09/21/2016 01:11 AM, Markus Trippelsdorf wrote:
> > The patch uses "nul" instead of "null" throughout.
> 
> Yes, that's intentional.  NUL and null are alternate spellings for
> the same character. I went with nul to distinguish it from the null
> pointer and used all lowercase as per the GCC convention.

Can you elaborate which guideline suggests spelling that in lowercase?
It seems quite strange to me, especially given that the documentation
added with the patch uses "NUL character" (which I believe to be a more
common form), but then warnings use "nul" (without the "character" iiuc).

Thanks.
Alexander

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 09/21/2016 01:11 AM, Markus Trippelsdorf wrote:

On 2016.09.11 at 20:03 -0600, Martin Sebor wrote:

On 09/08/2016 04:10 PM, Joseph Myers wrote:

On Thu, 8 Sep 2016, Martin Sebor wrote:



PR middle-end/49905 - Better sanity checking on sprintf src & dest to
produce warning for dodgy code


The patch uses "nul" instead of "null" throughout.


Yes, that's intentional.  NUL and null are alternate spellings for
the same character. I went with nul to distinguish it from the null
pointer and used all lowercase as per the GCC convention.

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 09/21/2016 04:36 AM, Christophe Lyon wrote:

Hi Martin,

On 21 September 2016 at 09:11, Markus Trippelsdorf
 wrote:

On 2016.09.11 at 20:03 -0600, Martin Sebor wrote:

On 09/08/2016 04:10 PM, Joseph Myers wrote:

On Thu, 8 Sep 2016, Martin Sebor wrote:



PR middle-end/49905 - Better sanity checking on sprintf src & dest to
  produce warning for dodgy code


The patch uses "nul" instead of "null" throughout.

--
Markus


I've noticed that some of the new tests fail on arm linux:
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1220)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1270)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1381)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 815)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 816)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 817)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 820)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 821)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 824)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 825)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 828)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 829)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 832)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 833)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test for excess errors)
  gcc.dg/tree-ssa/builtin-sprintf-warn-3.c  (test for warnings, line 46)
  gcc.dg/tree-ssa/builtin-sprintf-warn-3.c  (test for warnings, line 47)
  gcc.dg/tree-ssa/builtin-sprintf-warn-3.c (test for excess errors)
  gcc.dg/tree-ssa/builtin-sprintf.c (test for excess errors)

on aarch64 and arm bare-metal targets (using newlib), I've noticed
these failures:
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 99)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test for excess errors)
  gcc.dg/tree-ssa/builtin-sprintf-warn-2.c sprintf transformed into
strcpy (test for warnings, line 83)
  gcc.dg/tree-ssa/builtin-sprintf-warn-2.c sprintf transformed into
strcpy (test for warnings, line 84)
  gcc.dg/tree-ssa/builtin-sprintf-warn-4.c (test for excess errors)
  gcc.dg/tree-ssa/builtin-sprintf.c execution test

You may download the results from
http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/240298/report-build-info.html


Thanks, I'll look into these today.

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Christophe Lyon]

Hi Martin,

On 21 September 2016 at 09:11, Markus Trippelsdorf
 wrote:
> On 2016.09.11 at 20:03 -0600, Martin Sebor wrote:
>> On 09/08/2016 04:10 PM, Joseph Myers wrote:
>> > On Thu, 8 Sep 2016, Martin Sebor wrote:
>
>> PR middle-end/49905 - Better sanity checking on sprintf src & dest to
>>   produce warning for dodgy code
>
> The patch uses "nul" instead of "null" throughout.
>
> --
> Markus

I've noticed that some of the new tests fail on arm linux:
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1220)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1270)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 1381)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 815)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 816)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 817)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 820)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 821)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 824)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 825)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 828)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 829)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 832)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 833)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test for excess errors)
  gcc.dg/tree-ssa/builtin-sprintf-warn-3.c  (test for warnings, line 46)
  gcc.dg/tree-ssa/builtin-sprintf-warn-3.c  (test for warnings, line 47)
  gcc.dg/tree-ssa/builtin-sprintf-warn-3.c (test for excess errors)
  gcc.dg/tree-ssa/builtin-sprintf.c (test for excess errors)

on aarch64 and arm bare-metal targets (using newlib), I've noticed
these failures:
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c  (test for warnings, line 99)
  gcc.dg/tree-ssa/builtin-sprintf-warn-1.c (test for excess errors)
  gcc.dg/tree-ssa/builtin-sprintf-warn-2.c sprintf transformed into
strcpy (test for warnings, line 83)
  gcc.dg/tree-ssa/builtin-sprintf-warn-2.c sprintf transformed into
strcpy (test for warnings, line 84)
  gcc.dg/tree-ssa/builtin-sprintf-warn-4.c (test for excess errors)
  gcc.dg/tree-ssa/builtin-sprintf.c execution test

You may download the results from
http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/240298/report-build-info.html

Thanks,

Christophe

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Markus Trippelsdorf]

On 2016.09.11 at 20:03 -0600, Martin Sebor wrote:
> On 09/08/2016 04:10 PM, Joseph Myers wrote:
> > On Thu, 8 Sep 2016, Martin Sebor wrote:

> PR middle-end/49905 - Better sanity checking on sprintf src & dest to
>   produce warning for dodgy code

The patch uses "nul" instead of "null" throughout.

-- 
Markus

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 09/20/2016 01:38 PM, David Malcolm wrote:

On Tue, 2016-09-20 at 13:35 -0600, Martin Sebor wrote:

[...]


+#if 0

...

+#else

...

+#endif

Please remove the #if 0'd code.


Sorry about that.  This was left over from my effort to persuade
the substring_loc class to underline just the last quote of the
format string.  I thought David had made some changes to make it
possible but I may have misremembered.  Let me raise a separate
bug for it.


I don't think the substring_loc code supports such a feature yet;
please open a bug for it and CC me on it.


Done in bug 77672 - wrong rich location in warning: writing
a terminating nul past the end.

Thanks
Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[David Malcolm]

On Tue, 2016-09-20 at 13:35 -0600, Martin Sebor wrote:

[...]

> > > +#if 0
> ...
> > > +#else
> ...
> > > +#endif
> > Please remove the #if 0'd code.
> 
> Sorry about that.  This was left over from my effort to persuade
> the substring_loc class to underline just the last quote of the
> format string.  I thought David had made some changes to make it
> possible but I may have misremembered.  Let me raise a separate
> bug for it.

I don't think the substring_loc code supports such a feature yet;
please open a bug for it and CC me on it.

[...]

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

+  /* The minimum and maximum length.  The MAXLEN pointer stays unchanged
+ but MINLEN may be cleared during the execution of the function.  */
+  tree *minlen = length;
+  tree* const maxlen = length + 1;

Check formatting here.  I'm not sure if the formatting standards
explicitly state how to handle the "*" when there's a qualifier between
the type and the object.  But please check.


If my regular expressions were right, most code puts the star right
before the const (379 occurrences in .c and .h files under the gcc/
directory not counting the test suite).  For example:

  gcc/cp/decl.c:  const char *const name;

There's quite a bit of code that also puts a space after it (219
occurrences).  For example:

  gcc/cgraph.h:extern const char * const tls_model_names[];

There are 28 occurrences of the style I used, mostly in function
declarations.  For instance:

gcc/cp/mangle.c:decl_is_template_id (const tree decl, tree* const 
template_info)


I changed my code to follow with the dominant style.




+inform (info.fmtloc,
+"using the range [%qE, %qE] for directive argument",
+fmtres.argmin, fmtres.argmax);

Don't you need G_ for these messages?

Can you please check the calls to fmtwarn which pass in the string as an
argument and add G_ as needed?  I think the cases where the string is
computed into a variable are all handled correctly, it's jut those where
the string appears as a call argument that need double checking.  I
think there's ~3 of them.


I found an error() call in c-family/c-format.c with the string split
across two lines like here (see below) and the error message appears
correctly concatenated in the gcc.pot file so I take that to mean
that the G_ isn't necessary here.

  error ("found a %<%s%> reference but the format argument should"
 " be a string", format_name (gcc_objc_string_format_type));

Other than that, though, since it's an easy mistake to make, it
would be helpful if problems like the one you were worried about
could be detected by some tool run during a build.  I wonder how
hard it would be to write an awk scirpt...


+#if 0

...

+#else

...

+#endif

Please remove the #if 0'd code.


Sorry about that.  This was left over from my effort to persuade
the substring_loc class to underline just the last quote of the
format string.  I thought David had made some changes to make it
possible but I may have misremembered.  Let me raise a separate
bug for it.


I think with the nits above fixed, this is OK for the trunk.

Thanks for your patience.


Great, thank you!

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

David, in the way of feedback, I spent hours debugging the simple
multiline test I added.  It seems that DejaGnu is exquisitely
sensitive to whitespaces in the multiline output.  I appreciate
that spacing is essential to lining up the caret and the tildes
with the text of the program but tests fail even when all the
expected output is lined up right in the multiline directive but
off by a space (or tab) with respect to the actual output.  That
DejaGnu output doesn't make this very clear at all makes debugging
these types of issues a pain.  I wonder if there's a better to do
this.


Gah; I'm sorry this was such a pain to do.

I tend to just copy the stuff I want to quote directly from
Emacs's compilation buffer.


Yes, I did start with that.  The problem is that I have a script
that I run to help massage my patches to the format required by
the GCC Coding Conventions.  The script mainly replaces blocks
of spaces with tabs. So while my initial patch worked, the final
one didn't because of the tabs, and it took me hours to figure
out why.  Because I couldn't see the difference I thought it was
a bug in DejaGnu or some other tool that was different between
the two machines I was using for testing the two patches.  Until
it occurred to me to diff the test itself... (FWIW, I think this
style convention is bad and should be abolished in favor of using
only plain spaces.)



However a nit, which I think is why you had problems...


...


You have two pairs of dg-begin/end-multiline-output, but I think it's
better to have four; use a single begin/end pair for each call to
diagnostic_show_locus.  Hopefully doing that ought to make things a bit
less sensitive to whitespace, and easier to debug: each begin/end can
be handled by itself, whereas by combining them it's harder for
multiline.exp to detect them.  If combined, they can only be detected
if all of the dg-warning/dg-messages work (and are thus pruned by
prune.exp), if any aren't pruned, the multiline outputs will also fail.
 Maybe this exacerbated the problem?


Maybe.  I remember experimenting with the multiline directives when
I first added the test and was struggling to get it to work.  I think
the problems I was having were unrelated to tabs but probably had
something to do with the exact number of leading spaces.

FWIW, to make the multiline directives less prone to this type of
a problem it seems that instead of counting each leading space and
tab character and treating them as ordinary they could verify that
the first non-whitespace character on each line of the actual output
lines up with the first non-whitespace character of the expected
output with some constant offset.  That way spaces vs tabs shouldn't
matter as long as they were used consistently (or they could be made
not to matter at all if a tab was treated as a single space).  What
do you think about that?



So something like this, grouping the 4 diagnostics together with their
diagnostic_show_locus output by opening and closing comments (line
numbers will need adjusting):


Thanks.  Let me apply it and see how it works.



Another nit, if I may: FWIW I'm not in love with the wording of the messages.  
Sorry to bikeshed, but how about:
  warning: buffer overflow will occur when writing terminating NUL
and:
  note: formatted output of 2 bytes into a destination of size 1
or somesuch.


I won't claim the text of the messages is perfect but I did spend
a lot of time tweaking them.  That's not to say they can't be
improved but changing them means quite a bit of work adjusting
the tests.  At this point, I'd like to focus on getting the patch
committed.  After that, if there's still time, I'm happy to take
feedback and tweak the diagnostics based on it.

Thanks again for your help and the suggestions above!

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[David Malcolm]

On Sun, 2016-09-11 at 20:03 -0600, Martin Sebor wrote:
> On 09/08/2016 04:10 PM, Joseph Myers wrote:
> > On Thu, 8 Sep 2016, Martin Sebor wrote:
> > 
> > > diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
> > > index da133a4..4607495 100644
> > > --- a/gcc/doc/tm.texi.in
> > > +++ b/gcc/doc/tm.texi.in
> > > @@ -4081,6 +4081,13 @@ In either case, it remains possible to
> > > select code-generation for the alternate
> > >   scheme, by means of compiler command line switches.
> > >   @end defmac
> > > 
> > > +@deftypefn {Target Hook} {const char *}
> > > TARGET_LIBC_PRINTF_POINTER_FORMAT (tree, const char **@var{flags}
> > > )
> > > +A hook to determine the target @code{printf} implementation
> > > format string
> > > +that the most closely corresponds to the @code{%p} format
> > > directive.
> > > +The object pointed to by the @var{flags} is set to a string
> > > consisting
> > > +of recognized format flags such as the @code{'#'} character.
> > > +@end deftypefn
> > 
> > No, the substance of hook documentation should go in target.def
> > with just
> > an @hook line in tm.texi.in leading to the documentation going in
> > tm.texi
> > automatically.
> > 
> > You appear to be defining a target macro masquerading as a hook. 
> >  Please
> > don't (new target macros should be avoided where possible); use a
> > proper
> > hook.  (Maybe the settings depending on OS rather than architecture
> > means
> > it needs to be one of those whose default is a manual setting in
> > target-def.h rather than automatically generated, but that should
> > be the
> > limit of deviation from the normal workings of hooks.)
> > 
> > > +  const char *pfmt = TARGET_LIBC_PRINTF_POINTER_FORMAT (arg,
> > > );
> > 
> > With a proper hook them you'd call
> > targetm.libc_printf_pointer_format.
> > 
> > > + inform (callloc,
> > > + (nbytes + exact == 1
> > > +  ? "format output %wu byte into a destination of
> > > size %wu"
> > > +  : "format output %wu bytes into a destination
> > > of size %wu"),
> > > + nbytes + exact, info.objsize);
> > 
> > You need to use G_() around both format strings in such a case;
> > xgettext
> > doesn't know how to extract them both.
> 
> Attached is revision 8 of the patch (hopefully) adjusted as
> requested above, and with a simple test with of the multiline
> diagnostic directives suggested by David.  This revision also
> enables the -fprintf-return-value option by default.  The libgomp
> test failures I was seeing in my earlier testing must have been
> caused by an older version of GMP or MPFR that I had inadvertently
> use (normally I use in-tree versions downloaded and expanded there
> by the download_prerequisites script but that time I forgot that
> step).
> 
> David, in the way of feedback, I spent hours debugging the simple
> multiline test I added.  It seems that DejaGnu is exquisitely
> sensitive to whitespaces in the multiline output.  I appreciate
> that spacing is essential to lining up the caret and the tildes
> with the text of the program but tests fail even when all the
> expected output is lined up right in the multiline directive but
> off by a space (or tab) with respect to the actual output.  That
> DejaGnu output doesn't make this very clear at all makes debugging
> these types of issues a pain.  I wonder if there's a better to do
> this.

Gah; I'm sorry this was such a pain to do.

I tend to just copy the stuff I want to quote directly from
Emacs's compilation buffer.

However a nit, which I think is why you had problems...


diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c 
b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c
new file mode 100644
index 000..a601ba4
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-4.c
@@ -0,0 +1,29 @@
+/* { dg-do compile } */
+/* { dg-options "-Wformat -Wformat-length=1 -fdiagnostics-show-caret 
-ftrack-macro-expansion=0" } */
+
+extern int sprintf (char*, const char*, ...);
+
+char dst [8];
+
+void test (void)
+{
+  sprintf (dst + 7, "%-s", "1");
+  /* { dg-warning "writing a terminating nul past the end of the destination" 
"" { target *-*-*-* } 10 }
+ { dg-message "format output 2 bytes into a destination of size 1" "" { 
target *-*-*-* } 10 }
+{ dg-begin-multiline-output "" }
+   sprintf (dst + 7, "%-s", "1");
+ ^
+   sprintf (dst + 7, "%-s", "1");
+   ^
+{ dg-end-multiline-output "" } */
+
+  sprintf (dst + 7, "%-s", "abcd");
+  /* { dg-warning ".%-s. directive writing 4 bytes into a region of size 1" "" 
{ target *-*-*-* } 20 }
+ { dg-message "format output 5 bytes into a destination of size 1" "" { 
target *-*-*-* } 20 }
+{ dg-begin-multiline-output "" }
+   sprintf (dst + 7, "%-s", "abcd");
+  ^~~   ~~
+   sprintf (dst + 7, "%-s", "abcd");
+   ^~~~
+{ dg-end-multiline-output "" } */
+}

You have two pairs of dg-begin/end-multiline-output, 

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Jeff Law]

On 09/11/2016 08:03 PM, Martin Sebor wrote:


Attached is revision 8 of the patch (hopefully) adjusted as
requested above, and with a simple test with of the multiline
diagnostic directives suggested by David.  This revision also
enables the -fprintf-return-value option by default.  The libgomp
test failures I was seeing in my earlier testing must have been
caused by an older version of GMP or MPFR that I had inadvertently
use (normally I use in-tree versions downloaded and expanded there
by the download_prerequisites script but that time I forgot that
step).
Ah, my question answered.  I started looking at the multiline stuff and 
didn't read the last sentence WRT libgomp.  Sorry for the noise in my 
prior message.





David, in the way of feedback, I spent hours debugging the simple
multiline test I added.  It seems that DejaGnu is exquisitely
sensitive to whitespaces in the multiline output.  I appreciate
that spacing is essential to lining up the caret and the tildes
with the text of the program but tests fail even when all the
expected output is lined up right in the multiline directive but
off by a space (or tab) with respect to the actual output.  That
DejaGnu output doesn't make this very clear at all makes debugging
these types of issues a pain.  I wonder if there's a better to do
this.
dejagnu as a whole is a pain and trying to get the right whitespace, 
regexps, escaping is an exercise in pure torture.  How you and David 
have managed to do any multiline tests is amazing.





Thanks
Martin

gcc-49905.diff


PR middle-end/49905 - Better sanity checking on sprintf src & dest to
produce warning for dodgy code

gcc/ChangeLog:
PR middle-end/49905
* Makefile.in (OBJS): Add gimple-ssa-sprintf.o.
* config/linux.h (TARGET_PRINTF_POINTER_FORMAT): Redefine.
* config/linux.c (gnu_libc_printf_pointer_format): New function.
* config/sol2.h (TARGET_PRINTF_POINTER_FORMAT): Same.
* config/sol2.c (solaris_printf_pointer_format): New function.
* doc/invoke.texi (-Wformat-length, -fprintf-return-value): New
options.
* doc/tm.texi.in (TARGET_PRINTF_POINTER_FORMAT): Document.
* doc/tm.texi: Regenerate.
* gimple-fold.h (get_range_strlen): New function.
(get_maxval_strlen): Declare existing function.
* gimple-fold.c (get_range_strlen): Add arguments and compute both
maximum and minimum.
 (get_range_strlen): Define overload.
(get_maxval_strlen): Adjust.
* gimple-ssa-sprintf.c: New file and pass.
* passes.def (pass_sprintf_length): Add new pass.
* targhooks.h (default_printf_pointer_format): Declare new function.
(gnu_libc_printf_pointer_format): Same.
(solaris_libc_printf_pointer_format): Same.
* targhooks.c (default_printf_pointer_format): Define new function.
* tree-pass.h (make_pass_sprintf_length): Declare new function.
* print-tree.c: Increase buffer size.

gcc/c-family/ChangeLog:
PR middle-end/49905
* c.opt: Add -Wformat-length and -fprintf-return-value.

gcc/testsuite/ChangeLog:
PR middle-end/49905
* gcc.dg/builtin-stringop-chk-1.c: Adjust.
* gcc.dg/tree-ssa/builtin-sprintf-warn-1.c: New test.
* gcc.dg/tree-ssa/builtin-sprintf-warn-2.c: New test.
* gcc.dg/tree-ssa/builtin-sprintf-warn-3.c: New test.
* gcc.dg/tree-ssa/builtin-sprintf-warn-4.c: New test.
* gcc.dg/tree-ssa/builtin-sprintf.c: New test.
* gcc.dg/tree-ssa/builtin-sprintf-2.c: New test.





 static bool
-get_maxval_strlen (tree arg, tree *length, bitmap *visited, int type)
+get_range_strlen (tree arg, tree length[2], bitmap *visited, int type,
+ bool fuzzy)
 {
   tree var, val;
   gimple *def_stmt;

+  /* The minimum and maximum length.  The MAXLEN pointer stays unchanged
+ but MINLEN may be cleared during the execution of the function.  */
+  tree *minlen = length;
+  tree* const maxlen = length + 1;
Check formatting here.  I'm not sure if the formatting standards 
explicitly state how to handle the "*" when there's a qualifier between 
the type and the object.  But please check.



+
+ if (warned && fmtres.argmin)
+   {
+ if (fmtres.argmin == fmtres.argmax)
+   inform (info.fmtloc, "directive argument %qE", fmtres.argmin);
+ else if (fmtres.bounded)
+   inform (info.fmtloc, "directive argument in the range [%E, %E]",
+   fmtres.argmin, fmtres.argmax);
+ else
+   inform (info.fmtloc,
+   "using the range [%qE, %qE] for directive argument",
+   fmtres.argmin, fmtres.argmax);

Don't you need G_ for these messages?

Can you please check the calls to fmtwarn which pass in the string as an 
argument and add G_ as needed?  I think the cases where the string is 
computed into a variable are all handled correctly, 

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Jeff Law]

On 09/08/2016 01:03 PM, Martin Sebor wrote:

Attached is another update to the patch to address the last round
of comments and suggestions, most notably to:

 *  implement the checking of the implementation limit of 4,095 on
the output of a single directive to allow for the Glibc failure
due to ENOMEM (the patch issues a warning and disables the
optimization when this happens)
 *  implement checking for exceeding INT_MAX bytes (warn and disable
optimization)
 *  call set_range_info when the return value optimization is not
possible
 *  remove code to work around tree-optimization/71831 (now on
trunk)

The -fprintf-return value optimization is still disabled.  GCC
successfully bootstraps with it and most tests pass but there's
a failure in the Fortran libgomp tests that I am yet to figure
out.
I'm just now getting back to this (sorry for the long delay).  I see 
there's another version after this one.  Did the version from 9/11/2016 
address the Fortran libgomp test issue?


jeff

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Joseph Myers]

On Mon, 12 Sep 2016, Martin Sebor wrote:

> On 09/12/2016 04:08 PM, Joseph Myers wrote:
> > On Sun, 11 Sep 2016, Martin Sebor wrote:
> > 
> > > Attached is revision 8 of the patch (hopefully) adjusted as
> > > requested above, and with a simple test with of the multiline
> > 
> > The ChangeLog appears not to have been updated (at least, it doesn't
> > mention the changes to target.def).
> 
> I did update the ChangeLog but missed this file.  Are there any
> substantive issues with the patch that you noticed or may I take
> this as your approval (modulo the ChangeLog)?

I have not reviewed the substance of the patch.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 09/12/2016 04:08 PM, Joseph Myers wrote:

On Sun, 11 Sep 2016, Martin Sebor wrote:


Attached is revision 8 of the patch (hopefully) adjusted as
requested above, and with a simple test with of the multiline


The ChangeLog appears not to have been updated (at least, it doesn't
mention the changes to target.def).


I did update the ChangeLog but missed this file.  Are there any
substantive issues with the patch that you noticed or may I take
this as your approval (modulo the ChangeLog)?

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Joseph Myers]

On Sun, 11 Sep 2016, Martin Sebor wrote:

> Attached is revision 8 of the patch (hopefully) adjusted as
> requested above, and with a simple test with of the multiline

The ChangeLog appears not to have been updated (at least, it doesn't 
mention the changes to target.def).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

Patch #1.  All the fixes to static buffer sizes that were inspired by
your warning.  These are all approved and can go in immediately.



Attached is this patch.


Hi, this patch changed the file gcc/go/gofrontend/expressions.cc.  As
described in gcc/go/gofrontend/README, the files in the directory
gcc/go/gofrontend should not be changed in the GCC repository.
Instead, they are mirrored into GCC from a separate repository,
https://go.googlesource.com/gofrontend .  When you need to make
changes in the gofrontend directory, you can either follow the
contribution process described as
https://golang.org/doc/gccgo_contribute.html, or simply ask me to make
the change.  Thanks.


Sorry if this caused an inconvenience. I'll keep it in mind in
the future.

Thanks
Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Ian Lance Taylor]

On Fri, Aug 19, 2016 at 8:29 AM, Martin Sebor  wrote:
>> My biggest concern with this iteration is the tight integration between
>> the optimization and warning.  We generally avoid that kind of tight
>> integration such that enabling the warning does not affect the
>> optimization and vice-versa.
>>
>> So ISTM you have to do the analysis if the optimization or warning has
>> been requested.  Then you conditionalize whether or not the warnings are
>> emitted by their flag and the optimization based on its flag.
>
>
> As we discussed in IRC yesterday, the warning and the optimization
> are independent of one another, and each controlled by its own option
> (-Wformat-length and -fprintf-return-value).  In light of that we've
> agreed that submitting both as part of the same patch is sufficient.
>
>>
>> I understand you're going to have some further work to do because of
>> conflicts with David's patches.  With that in mind, I'd suggest a bit of
>> carving things up so things can start moving forward.
>>
>>
>> Patch #1.  All the fixes to static buffer sizes that were inspired by
>> your warning.  These are all approved and can go in immediately.
>
>
> Attached is this patch.

Hi, this patch changed the file gcc/go/gofrontend/expressions.cc.  As
described in gcc/go/gofrontend/README, the files in the directory
gcc/go/gofrontend should not be changed in the GCC repository.
Instead, they are mirrored into GCC from a separate repository,
https://go.googlesource.com/gofrontend .  When you need to make
changes in the gofrontend directory, you can either follow the
contribution process described as
https://golang.org/doc/gccgo_contribute.html, or simply ask me to make
the change.  Thanks.

Ian

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Joseph Myers]

On Thu, 8 Sep 2016, Martin Sebor wrote:

> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
> index da133a4..4607495 100644
> --- a/gcc/doc/tm.texi.in
> +++ b/gcc/doc/tm.texi.in
> @@ -4081,6 +4081,13 @@ In either case, it remains possible to select 
> code-generation for the alternate
>  scheme, by means of compiler command line switches.
>  @end defmac
>  
> +@deftypefn {Target Hook} {const char *} TARGET_LIBC_PRINTF_POINTER_FORMAT 
> (tree, const char **@var{flags})
> +A hook to determine the target @code{printf} implementation format string
> +that the most closely corresponds to the @code{%p} format directive.
> +The object pointed to by the @var{flags} is set to a string consisting
> +of recognized format flags such as the @code{'#'} character.
> +@end deftypefn

No, the substance of hook documentation should go in target.def with just 
an @hook line in tm.texi.in leading to the documentation going in tm.texi 
automatically.

You appear to be defining a target macro masquerading as a hook.  Please 
don't (new target macros should be avoided where possible); use a proper 
hook.  (Maybe the settings depending on OS rather than architecture means 
it needs to be one of those whose default is a manual setting in 
target-def.h rather than automatically generated, but that should be the 
limit of deviation from the normal workings of hooks.)

> +  const char *pfmt = TARGET_LIBC_PRINTF_POINTER_FORMAT (arg, );

With a proper hook them you'd call targetm.libc_printf_pointer_format.

> + inform (callloc,
> + (nbytes + exact == 1
> +  ? "format output %wu byte into a destination of size %wu"
> +  : "format output %wu bytes into a destination of size %wu"),
> + nbytes + exact, info.objsize);

You need to use G_() around both format strings in such a case; xgettext 
doesn't know how to extract them both.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 09/08/2016 01:45 PM, David Malcolm wrote:

On Thu, 2016-09-08 at 13:03 -0600, Martin Sebor wrote:

Attached is another update to the patch to address the last round
of comments and suggestions, most notably to:

   *  implement the checking of the implementation limit of 4,095 on
  the output of a single directive to allow for the Glibc failure
  due to ENOMEM (the patch issues a warning and disables the
  optimization when this happens)
   *  implement checking for exceeding INT_MAX bytes (warn and disable
  optimization)
   *  call set_range_info when the return value optimization is not
  possible
   *  remove code to work around tree-optimization/71831 (now on
  trunk)

The -fprintf-return value optimization is still disabled.  GCC
successfully bootstraps with it and most tests pass but there's
a failure in the Fortran libgomp tests that I am yet to figure
out.

I'm hoping to get the patch reviewed and hopefully approved while
I debug the libgomp failure.

Martin


I see that you also integrated the substring_loc and format_warning API
into this revision - thanks.


Yes, I forgot to mention it among the highlights.  Thanks for making
the API available to the middle-end!



The patch has a lot of macro-based testcases, presumably for exercising
all of the format codes and boundary conditions, but it seems to be
lacking what I'd call a "usability test case" - a test case that shows
a representative example of idiomatic but buggy code, along with the
full output of the warning, with carets and underlining, so that we can
easily see what the user experience is.  (sorry if there is one and I
didn't see it).


There is a simple test (in the test_sprintf_note() function in
builtin-sprintf-warn-1.c) that exercises the notes but there's always
room for more and more robust test cases :)  I'll see about adding
a few.

FWIW, the challenge here is not in adding them but rather in knowing
when to stop and what level of detail to go to.  With too many test
cases (exercising this level of detail) it can get very tedious and
time consuming to then change the diagnostics or add more detail.
This is not an excuse for not having enough tests.  But striking
the right balance between the level of detail in them is something
I had to grapple with on this project.


 From a marketing point-of-view, I think any new diagnostics like this
deserve a screenshot on the website's gcc-7/changes.html page, showing
a simple example of the above that makes a casual reader think "gcc 7
looks neat; I've definitely made that mistake; I wonder if that's going
to find bugs in my code; I'd better try it at some point".

So please can you add a test case that demonstrates such a screenshot
-worthy example, using:

   /* { dg-options "-fdiagnostics-show-caret" } */

and you can use:

   /* { dg-begin-multiline-output "" }
copy the source, underlines and carets here, omitting trailing dg
directives.
  { dg-end-multiline-output "" } */

(we'd strip away all the dg- directives when making the screenshots for
the website).

The act of creating such an example sometimes suggests tweaks e.g. to
the exact wording of the warning.

Sorry if this seems like I'm picking on you Martin; I just wanted to
share some thoughts that I'm trying to crystallize into general
guidelines on writing diagnostics.


Not at all!  Thanks for the review and for the suggestion to make
use of the multiline DejaGnu directives.  I'll add more tests in
the next revision of the patch (as I have been in each iteration).

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[David Malcolm]

On Thu, 2016-09-08 at 13:03 -0600, Martin Sebor wrote:
> Attached is another update to the patch to address the last round
> of comments and suggestions, most notably to:
> 
>   *  implement the checking of the implementation limit of 4,095 on
>  the output of a single directive to allow for the Glibc failure
>  due to ENOMEM (the patch issues a warning and disables the
>  optimization when this happens)
>   *  implement checking for exceeding INT_MAX bytes (warn and disable
>  optimization)
>   *  call set_range_info when the return value optimization is not
>  possible
>   *  remove code to work around tree-optimization/71831 (now on
>  trunk)
> 
> The -fprintf-return value optimization is still disabled.  GCC
> successfully bootstraps with it and most tests pass but there's
> a failure in the Fortran libgomp tests that I am yet to figure
> out.
> 
> I'm hoping to get the patch reviewed and hopefully approved while
> I debug the libgomp failure.
> 
> Martin

I see that you also integrated the substring_loc and format_warning API
into this revision - thanks.

The patch has a lot of macro-based testcases, presumably for exercising
all of the format codes and boundary conditions, but it seems to be
lacking what I'd call a "usability test case" - a test case that shows
a representative example of idiomatic but buggy code, along with the
full output of the warning, with carets and underlining, so that we can
easily see what the user experience is.  (sorry if there is one and I
didn't see it).

>From a marketing point-of-view, I think any new diagnostics like this
deserve a screenshot on the website's gcc-7/changes.html page, showing
a simple example of the above that makes a casual reader think "gcc 7
looks neat; I've definitely made that mistake; I wonder if that's going
to find bugs in my code; I'd better try it at some point".

So please can you add a test case that demonstrates such a screenshot
-worthy example, using:

  /* { dg-options "-fdiagnostics-show-caret" } */

and you can use:

  /* { dg-begin-multiline-output "" }
copy the source, underlines and carets here, omitting trailing dg
directives.
 { dg-end-multiline-output "" } */

(we'd strip away all the dg- directives when making the screenshots for
the website).

The act of creating such an example sometimes suggests tweaks e.g. to
the exact wording of the warning.

Sorry if this seems like I'm picking on you Martin; I just wanted to
share some thoughts that I'm trying to crystallize into general
guidelines on writing diagnostics.

Hope this is constructive
Dave

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 09/08/2016 01:21 PM, Jeff Law wrote:

On 08/24/2016 10:40 AM, Martin Sebor wrote:

On 08/23/2016 05:00 PM, Joseph Myers wrote:

Some observations:

* Does -fprintf-return-value allow for the possibility of snprintf
failing
because of a memory allocation failure and so returning -1 when GCC
computed bounds on what it could return if successful?


No.  I recall having seen Glibc fail with ENOMEM years ago when
formatting a floating point number to a very large precision but
I haven't seen any implementation fail.  I haven't yet looked to
see if the Glibc failure can still happen.  My reading of C and
POSIX is that snprintf is only allowed to fail due to an encoding
error, not because it runs out of memory, so such a failure would
seem like a bug.

At the same time, the cause of the failure doesn't really matter.
If the function could fail, unless GCC could determine the failure
at compile time and avoid the optimization, the transformation
wouldn't be guaranteed to be safe.  It might be something to
consider and possibly accommodate in the implementation.  It's
one of the reasons why I want to expose the optimization to
more code before enabling it.

I also haven't yet thought about how to deal with it but if it
is a possibility we want to allow for then maybe a target hook
for libc implementers to set to indicate whether sprintf can fail
and when would work.  Libc implementations that can fail under
any conditions (whether allowed by the standard or not) would
need to disable (or not enable, depending on the default) the
optimization.  I'm certainly open to other ideas.

So what are the implications for the optimization part of this patch?


By coincidence I've just posted an updated patch that handles this
situation by avoiding the optimization (and issuing a warning pointing
out that a directive produced more that 4,095 bytes worth of output).
Ditto for INT_MAX.

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Jeff Law]

On 08/24/2016 10:40 AM, Martin Sebor wrote:

On 08/23/2016 05:00 PM, Joseph Myers wrote:

Some observations:

* Does -fprintf-return-value allow for the possibility of snprintf
failing
because of a memory allocation failure and so returning -1 when GCC
computed bounds on what it could return if successful?


No.  I recall having seen Glibc fail with ENOMEM years ago when
formatting a floating point number to a very large precision but
I haven't seen any implementation fail.  I haven't yet looked to
see if the Glibc failure can still happen.  My reading of C and
POSIX is that snprintf is only allowed to fail due to an encoding
error, not because it runs out of memory, so such a failure would
seem like a bug.

At the same time, the cause of the failure doesn't really matter.
If the function could fail, unless GCC could determine the failure
at compile time and avoid the optimization, the transformation
wouldn't be guaranteed to be safe.  It might be something to
consider and possibly accommodate in the implementation.  It's
one of the reasons why I want to expose the optimization to
more code before enabling it.

I also haven't yet thought about how to deal with it but if it
is a possibility we want to allow for then maybe a target hook
for libc implementers to set to indicate whether sprintf can fail
and when would work.  Libc implementations that can fail under
any conditions (whether allowed by the standard or not) would
need to disable (or not enable, depending on the default) the
optimization.  I'm certainly open to other ideas.

So what are the implications for the optimization part of this patch?

Jeff

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Jeff Law]

On 08/24/2016 05:14 PM, Manuel López-Ibáñez wrote:



I agree.  The challenge is that not all the bits this depends on
(the g_string_concat_db and parse_in globals defined in the front
end) are available in the middle end.  I've been talking to David
Malcolm about how best to factor things out of c-format.c and make
it available in both parts of the compiler under a convenient API.


Perhaps diagnostics_context could have pointers to those, forward
defined in the .h file and include the relevant libcpp headers in
diagnostics.c (or input.c). FEs that make use of those features could
initialize them (via some API) to some existing object. Otherwise,
they will work like in your patch (but within diagnostic.c). Similar
to how we initialize the FE-specific pretty-printers.

We already depend on libcpp for line-map.c, so internally depending on
other libcpp features is not so bad. The important thing is to hide
this from the clients, so that the clients do not need to be aware of
what diagnostics.c requires. That is, the middle-end and Ada should
not include headers that include libcpp headers, but diagnostics.c can
include whatever it needs.

Otherwise, the future will be again a mess and we get further away
from ever separating the FEs from the ME.
Warnings which rely on things like const/copy propagation, range 
analysis, inherently belong in the the middle end.  They're next to 
useless in the front-end.


This implies that a goodly amount of what's in c-format needs to move 
and likely bits of libcpp/line-map as well.





BTW, it would be nice to explain in comments why each header needs to
be included, besides obvious ones such as tree.h and gimple.h (it
would be great if we had guidelines on how to order included headers,
why not group together all gimple*, tree*, backend-stuff, diagnostics
stuff?). On the other hand, it is unfair to nitpick your patch
regarding this when other commits do the same.

I see this as busy work and work that easily gets out of date.

I'd rather just use Andrew's header file refactoring work to be able to 
answer these kinds of questions, canonicalize include ordering and to 
eliminate unnecessary/redundant includes.  In fact, ISTM it ought to be 
run before we hit stage1 close :-)


Jeff

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Joseph Myers]

On Wed, 24 Aug 2016, Martin Sebor wrote:

> Since it's presumably a Glibc bug (bug 2509?) is this something
> you believe the optimization needs to worry about?  If so, can
> you confirm that only Glibc versions 2.4 and prior are affected
> and 2.5 is not?

The code may not need to care about it, but the assumptions need to be 
made clear.  I haven't tried to confirm which glibc versions had an issue 
here.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Manuel López-Ibáñez]

>> --Wno-format-contains-nul -Wno-format-extra-args -Wformat-nonliteral @gol
>> +-Wno-format-contains-nul -Wno-format-extra-args -Wformat-length=1 @gol
>>
>> Most options that take levels are documented as:
>>
>> -Wshift-overflow -Wshift-overflow=@var{n}
>> -Wstrict-overflow -Wstrict-overflow=@var{n}
>
>
> I haven't found any uses of the @var{} tag for numeric arguments
> to options (e.g., -Wformat=2), only for symbolic arguments (e.g.,
> -Warray-bounds=@var{n}), so I'm leaving this as is.

My suggestion was to document -Wformat-length=@var{n}. There is no
difference between -Wformat-length's levels and -Wstrict-overfllow,
-Wshift-overflow, -Wstrict-aliasing levels.
-Wformat=2 is actually the odd one. Note that its description does use:
@item -Wformat
@itemx -Wformat=@var{n}

Even if you want to be explicit and follow -Wformat, then it should be

+-Wno-format-contains-nul -Wno-format-extra-args -Wformat-length
-Wformat-length=2 @gol


> I agree.  The challenge is that not all the bits this depends on
> (the g_string_concat_db and parse_in globals defined in the front
> end) are available in the middle end.  I've been talking to David
> Malcolm about how best to factor things out of c-format.c and make
> it available in both parts of the compiler under a convenient API.

Perhaps diagnostics_context could have pointers to those, forward
defined in the .h file and include the relevant libcpp headers in
diagnostics.c (or input.c). FEs that make use of those features could
initialize them (via some API) to some existing object. Otherwise,
they will work like in your patch (but within diagnostic.c). Similar
to how we initialize the FE-specific pretty-printers.

We already depend on libcpp for line-map.c, so internally depending on
other libcpp features is not so bad. The important thing is to hide
this from the clients, so that the clients do not need to be aware of
what diagnostics.c requires. That is, the middle-end and Ada should
not include headers that include libcpp headers, but diagnostics.c can
include whatever it needs.

Otherwise, the future will be again a mess and we get further away
from ever separating the FEs from the ME.

BTW, it would be nice to explain in comments why each header needs to
be included, besides obvious ones such as tree.h and gimple.h (it
would be great if we had guidelines on how to order included headers,
why not group together all gimple*, tree*, backend-stuff, diagnostics
stuff?). On the other hand, it is unfair to nitpick your patch
regarding this when other commits do the same.

+#include "backend.h"
+#include "tree.h"
+#include "gimple.h"
+#include "tree-pass.h"
+#include "ssa.h"
+#include "gimple-fold.h"
+#include "gimple-pretty-print.h"
+#include "diagnostic-core.h"

Already included in diagnostic.h

+#include "fold-const.h"
+#include "gimple-iterator.h"
+#include "tree-ssa.h"
+#include "tree-object-size.h"
+#include "params.h"
+#include "tree-cfg.h"
+#include "calls.h"
+#include "cfgloop.h"
+#include "intl.h"
+
+#include "builtins.h"
+#include "stor-layout.h"
+
+#include "realmpfr.h"
+#include "target.h"
+#include "targhooks.h"
+
+#include "cpplib.h"

Not in the ME!

+#include "input.h"

already included in coretypes.h

+#include "toplev.h"

Very few files actually need to include toplev.h. Most of them just
include it because of copy-pasting headers lists. And even for bogus
reasons (final.c:#include "toplev.h" /* exact_log2, floor_log2 */)

+#include "substring-locations.h"
+#include "diagnostic.h"

Cheers,

Manuel.

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 08/24/2016 04:03 PM, Joseph Myers wrote:

On Wed, 24 Aug 2016, Martin Sebor wrote:


No.  I recall having seen Glibc fail with ENOMEM years ago when
formatting a floating point number to a very large precision but
I haven't seen any implementation fail.  I haven't yet looked to
see if the Glibc failure can still happen.  My reading of C and
POSIX is that snprintf is only allowed to fail due to an encoding
error, not because it runs out of memory, so such a failure would
seem like a bug.


It's a general ISO C principle that there may be implementation limits,
including in areas where no specific minimum limit is given in the
standard, and even where there is a mimimum, that doesn't mean that every
possible program that does not exceed that minimum does not exceed the
limit in that area.  In this case, a specific minimum limit is given,
7.21.6.1#15 "The number of characters that can be produced by any single
conversion shall be at least 4095.".  That is, libc can't reject all cases
of larger results, but it's possible that if memory is short then some
cases with shorter results could still fail.


It sounds like using the 4095 limit should be safe even with Glibc.
I'll use it then, thanks.


It sounds like the concern is that for the following call (when
UCHAR_MAX is 255):

   sprintf (d, "%hhu", 1000)

some implementation (an old version of Glibc?) may have actually
produced four digits and returned 4 on the basis of C saying that
the %hhu argument must be an unsigned char (promoted to int) and
thus the behavior of the call being undefined.


Yes.


Since it's presumably a Glibc bug (bug 2509?) is this something
you believe the optimization needs to worry about?  If so, can
you confirm that only Glibc versions 2.4 and prior are affected
and 2.5 is not?

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Joseph Myers]

On Wed, 24 Aug 2016, Martin Sebor wrote:

> No.  I recall having seen Glibc fail with ENOMEM years ago when
> formatting a floating point number to a very large precision but
> I haven't seen any implementation fail.  I haven't yet looked to
> see if the Glibc failure can still happen.  My reading of C and
> POSIX is that snprintf is only allowed to fail due to an encoding
> error, not because it runs out of memory, so such a failure would
> seem like a bug.

It's a general ISO C principle that there may be implementation limits, 
including in areas where no specific minimum limit is given in the 
standard, and even where there is a mimimum, that doesn't mean that every 
possible program that does not exceed that minimum does not exceed the 
limit in that area.  In this case, a specific minimum limit is given, 
7.21.6.1#15 "The number of characters that can be produced by any single 
conversion shall be at least 4095.".  That is, libc can't reject all cases 
of larger results, but it's possible that if memory is short then some 
cases with shorter results could still fail.

It's also a general POSIX principle that functions may have additional 
error conditions beyond those given in the standard, when it's possible 
for them to return errors at all.

(Since people may wish to use snprintf / dprintf in signal handlers, 
avoiding malloc where possible in those functions is still a good idea.)

> > * It looks like you convert to (signed/unsigned) char for %hh formats,
> > etc.  Now, there is the possibility that the value passed was actually of
> > type int, and out of range for those types.  And there is the possibility
> > that the implementation might not itself convert those values to char /
> > short (glibc didn't until 2006) - passing a value outside the range of the
> > relevant type seems likely undefined behavior, so implementations may not
> > actually need to convert, and there's an open question about whether the
> > value actually needs to have been promoted from char/short in the caller
> > (see my ).  I don't
> > know if you wish to allow at all for this issue.
> 
> It sounds like the concern is that for the following call (when
> UCHAR_MAX is 255):
> 
>   sprintf (d, "%hhu", 1000)
> 
> some implementation (an old version of Glibc?) may have actually
> produced four digits and returned 4 on the basis of C saying that
> the %hhu argument must be an unsigned char (promoted to int) and
> thus the behavior of the call being undefined.

Yes.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Florian Weimer]

On 08/24/2016 06:40 PM, Martin Sebor wrote:

On 08/23/2016 05:00 PM, Joseph Myers wrote:

Some observations:

* Does -fprintf-return-value allow for the possibility of snprintf
failing
because of a memory allocation failure and so returning -1 when GCC
computed bounds on what it could return if successful?


No.  I recall having seen Glibc fail with ENOMEM years ago when
formatting a floating point number to a very large precision but
I haven't seen any implementation fail.  I haven't yet looked to
see if the Glibc failure can still happen.  My reading of C and
POSIX is that snprintf is only allowed to fail due to an encoding
error, not because it runs out of memory, so such a failure would
seem like a bug.


It can still happen, and not just for floating point.

#include 

int
main()
{
  printf("%099d\n", 5);
}

valgrind reports:

   total heap usage: 1 allocs, 1 frees, 1,000,031 bytes allocated

We are certainly not proud of this state of affairs, but it is unlikely 
to change anytime soon.


Florian

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 08/23/2016 05:00 PM, Joseph Myers wrote:

Some observations:

* Does -fprintf-return-value allow for the possibility of snprintf failing
because of a memory allocation failure and so returning -1 when GCC
computed bounds on what it could return if successful?


No.  I recall having seen Glibc fail with ENOMEM years ago when
formatting a floating point number to a very large precision but
I haven't seen any implementation fail.  I haven't yet looked to
see if the Glibc failure can still happen.  My reading of C and
POSIX is that snprintf is only allowed to fail due to an encoding
error, not because it runs out of memory, so such a failure would
seem like a bug.

At the same time, the cause of the failure doesn't really matter.
If the function could fail, unless GCC could determine the failure
at compile time and avoid the optimization, the transformation
wouldn't be guaranteed to be safe.  It might be something to
consider and possibly accommodate in the implementation.  It's
one of the reasons why I want to expose the optimization to
more code before enabling it.

I also haven't yet thought about how to deal with it but if it
is a possibility we want to allow for then maybe a target hook
for libc implementers to set to indicate whether sprintf can fail
and when would work.  Libc implementations that can fail under
any conditions (whether allowed by the standard or not) would
need to disable (or not enable, depending on the default) the
optimization.  I'm certainly open to other ideas.


* It looks like you convert to (signed/unsigned) char for %hh formats,
etc.  Now, there is the possibility that the value passed was actually of
type int, and out of range for those types.  And there is the possibility
that the implementation might not itself convert those values to char /
short (glibc didn't until 2006) - passing a value outside the range of the
relevant type seems likely undefined behavior, so implementations may not
actually need to convert, and there's an open question about whether the
value actually needs to have been promoted from char/short in the caller
(see my ).  I don't
know if you wish to allow at all for this issue.


It sounds like the concern is that for the following call (when
UCHAR_MAX is 255):

  sprintf (d, "%hhu", 1000)

some implementation (an old version of Glibc?) may have actually
produced four digits and returned 4 on the basis of C saying that
the %hhu argument must be an unsigned char (promoted to int) and
thus the behavior of the call being undefined.

I wouldn't think this to be the intended interpretation (C11 says
the argument "value shall be converted to signed char or unsigned
char before printing") but if it really was meant to be undefined
then having GCC treat the undefined call differently than libc
shouldn't be a problem.  That said, I've tried not to allow the
optimization for calls with undefined behavior so if there was
a serious concern about this causing trouble I could see about
detecting this case and disabling it.

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Manuel López-Ibáñez]

On 23 August 2016 at 22:56, Martin Sebor  wrote:
> Attached is the latest patch.

Some suggestions:

--Wno-format-contains-nul -Wno-format-extra-args -Wformat-nonliteral @gol
+-Wno-format-contains-nul -Wno-format-extra-args -Wformat-length=1 @gol

Most options that take levels are documented as:

-Wshift-overflow -Wshift-overflow=@var{n}
-Wstrict-overflow -Wstrict-overflow=@var{n}


+@item -Wformat-length
+@itemx -Wformat-length=@var{level}
+@opindex Wformat-length
+@opindex Wno-format-length
+@opindex ffreestanding
+@opindex fno-builtin
+@opindex Wformat-length=

This whole hunk is duplicated

Usually, at the end of the option description, it is mentioned which
options enabled this one (-Wformat=1 in this case, which is in turn
enabled by -Wall).

+The @option{-Wformat-length} option causes GCC to attempt to detect calls
+to formatted functions such as @code{sprintf} that might overflow the
+destination buffer, or bounded functions like @code{snprintf} that result
+in output truncation.

This text is a bit too verbose and quite different from other warning
options. A humble proposal:

Warn about calls to formatted functions, such as @code{sprintf}, that
may overflow the destination buffer, or bounded functions, such as
@code{snprintf} , that may? result in output truncation.

+GCC counts the number of bytes that each format
+string and directive within it writes into the provided buffer and, when
+it detects that more bytes that fit in the destination buffer may be output,
+it emits a warning.

This is saying basically the same as the first sentence. Remove it?

+Directives whose arguments have values that can be
+determined at compile-time account for the exact number of bytes they write.
+Directives with arguments whose values cannot be determined are processed
+based on heuristics that depend on the @var{level} argument to the option,
+and on optimization.

When the exact number of bytes written by a format string directive
cannot be determined at compile-time, it is estimated based on
heuristics that depend on optimization and the @var{level} argument.
(I swapped the two because level is the next thing explained).

+The default setting of @var{level} is 1.  Level
+@var{1} employs a conservative approach that warns only about calls that
+most likely overflow the buffer or result in output truncation.  At this
+level, numeric arguments to format directives whose values are unknown
+are assumed to have the value of one, and strings of unknown length are
+assumed to have a length of zero.  Numeric arguments that are known to
+be bounded to a subrange of their type, or string arguments whose output
+is bounded by their directive's precision, are assumed to take on the value
+within the range that results in the most bytes on output.  Level @var{2}
+warns also bout calls that may overflow the destination buffer or result
+in truncation given an argument of sufficient length or magnitude.  At
+this level, unknown numeric arguments are assumed to have the minimum
+representable value for signed types with a precision greater than 1,
+and the maximum representable value otherwise.  Unknown string arguments
+are assumed to be 1 character long.

A better format (and it enables searching for specific levels) would be:
@table @gcctabopt
@item -Wformat-length=1
This is the warning level enabled by @option{-Wformat-length} and is enabled
by @option{-Wformat=1}, which is in turn enabled by @option{-Wall}. It
employs a conservative heuristic that warns only about calls that
most likely overflow the buffer or result in output truncation.  At this
level, numeric arguments to format directives whose values are unknown
are assumed to have the value of one, and strings of unknown length are
assumed to have a length of zero.  Numeric arguments that are known to
be bounded to a subrange of their type, or string arguments whose output
is bounded by their directive's precision, are assumed to take on the value
within the range that results in the most bytes on output.

@item -Wformat-length=2
This level also warns about calls that may overflow the destination
buffer or result
in truncation given an argument of sufficient length or magnitude.
Unknown numeric arguments are assumed to have the minimum
representable value for signed types with a precision greater than one,
and the maximum representable value otherwise.  Unknown string arguments
are assumed to be one character long.
@end table

+Enabling optimization will in most
+cases improve the accuracy of the warning, although in some cases it may
+also result in false positives.

Since the example is not about optimization. Move this paragraph after
the examples.

+For example, at level @var{1}, the call to @code{sprintf} below is diagnosed

For example, @option{-Wformat-length=1} warns about the call to
@code{sprintf} below

+At level @var{2}, the call
+is again diagnosed, but this time

@option{-Wformat-length=2} also warns about the call to

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Joseph Myers]

Some observations:

* Does -fprintf-return-value allow for the possibility of snprintf failing 
because of a memory allocation failure and so returning -1 when GCC 
computed bounds on what it could return if successful?

* It looks like you convert to (signed/unsigned) char for %hh formats, 
etc.  Now, there is the possibility that the value passed was actually of 
type int, and out of range for those types.  And there is the possibility 
that the implementation might not itself convert those values to char / 
short (glibc didn't until 2006) - passing a value outside the range of the 
relevant type seems likely undefined behavior, so implementations may not 
actually need to convert, and there's an open question about whether the 
value actually needs to have been promoted from char/short in the caller 
(see my ).  I don't 
know if you wish to allow at all for this issue.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Florian Weimer]

On 08/20/2016 05:18 AM, Trevor Saunders wrote:

Patch #5 and beyond: Further optimization work.


As one of the next steps I'd like to make this feature available
to user-defined sprintf-like functions decorated with attribute
format.  To do that, I'm thinking of adding either a fourth
(optional) argument to attribute format printf indicating which
of the function arguments is the destination buffer (to compute
its size), or perhaps a new attribute under its own name.  I'm
actually leaning toward latter since I think it could be used
in other contexts as well.  I welcome comments and suggestions
on this idea.

Whichever we think will be easier to use and thus encourage folks to
annotate their code properly :-)


So, sort of related I've been thinking about writing C++ string building
classes some lately.  Where you'd have a fixed length buffer and a
pointer to the next buffer (so a degenerate rope with only one side of
the tree).  One use case for such a thing is building up strings of
assembly to output in gcc.  however one somewhat awkward bit is that we
often format assembly with printf, which isn't really great for this use
case, because you need to deal with the case the current buffer only has
space for part of the string you are formatting.  So it would be nice to
have something like an interuptable printf that tells you where in the
string it stopped formatting, and allows you to continue from there with
a new buffer.


glibc provides fopencookie, which can be used to print directly to a 
custom stream.  I don't know if it would provide any speed gains because 
of the internal complexities of libio.


It could be emulated using snprintf and a temporary buffer for non-glibc 
systems.


Florian

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Trevor Saunders]

> > > Patch #5 and beyond: Further optimization work.
> > 
> > As one of the next steps I'd like to make this feature available
> > to user-defined sprintf-like functions decorated with attribute
> > format.  To do that, I'm thinking of adding either a fourth
> > (optional) argument to attribute format printf indicating which
> > of the function arguments is the destination buffer (to compute
> > its size), or perhaps a new attribute under its own name.  I'm
> > actually leaning toward latter since I think it could be used
> > in other contexts as well.  I welcome comments and suggestions
> > on this idea.
> Whichever we think will be easier to use and thus encourage folks to
> annotate their code properly :-)

So, sort of related I've been thinking about writing C++ string building
classes some lately.  Where you'd have a fixed length buffer and a
pointer to the next buffer (so a degenerate rope with only one side of
the tree).  One use case for such a thing is building up strings of
assembly to output in gcc.  however one somewhat awkward bit is that we
often format assembly with printf, which isn't really great for this use
case, because you need to deal with the case the current buffer only has
space for part of the string you are formatting.  So it would be nice to
have something like an interuptable printf that tells you where in the
string it stopped formatting, and allows you to continue from there with
a new buffer.
 
 It also seems worth noting that in C++11 you can actually write a
 printf that is typesafe without the format attributes for example Tom
 has one here https://github.com/tromey/typesafe-printf.  I'm not sure
 putting such a thing in libc for C++ programs is a great idea because
 of compile time costs, but its tempting.

 Trev

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Jeff Law]

On 08/19/2016 09:29 AM, Martin Sebor wrote:

My biggest concern with this iteration is the tight integration between
the optimization and warning.  We generally avoid that kind of tight
integration such that enabling the warning does not affect the
optimization and vice-versa.

So ISTM you have to do the analysis if the optimization or warning has
been requested.  Then you conditionalize whether or not the warnings are
emitted by their flag and the optimization based on its flag.


As we discussed in IRC yesterday, the warning and the optimization
are independent of one another, and each controlled by its own option
(-Wformat-length and -fprintf-return-value).  In light of that we've
agreed that submitting both as part of the same patch is sufficient.
Right.  I must have mis-read something.  I'll look for the tidbit that 
made me think they were more intertwined than they really are.  It may 
be the case that we just want to tweak a comment.






I understand you're going to have some further work to do because of
conflicts with David's patches.  With that in mind, I'd suggest a bit of
carving things up so things can start moving forward.


Patch #1.  All the fixes to static buffer sizes that were inspired by
your warning.  These are all approved and can go in immediately.


Attached is this patch.

Approved.  Please install.






Patch #2. Improvement to __builtin_object_size to handle
POINTER_PLUS_EXPR on arrays.  This is something that stands on it own
and ought to be reviewable quickly and doesn't really belong in the
bowels of the warning/optimization patch you're developing.


Sure.  I'll submit this patch next.

Excellent.



Patch #5 and beyond: Further optimization work.


As one of the next steps I'd like to make this feature available
to user-defined sprintf-like functions decorated with attribute
format.  To do that, I'm thinking of adding either a fourth
(optional) argument to attribute format printf indicating which
of the function arguments is the destination buffer (to compute
its size), or perhaps a new attribute under its own name.  I'm
actually leaning toward latter since I think it could be used
in other contexts as well.  I welcome comments and suggestions
on this idea.
Whichever we think will be easier to use and thus encourage folks to 
annotate their code properly :-)


jeff

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

My biggest concern with this iteration is the tight integration between
the optimization and warning.  We generally avoid that kind of tight
integration such that enabling the warning does not affect the
optimization and vice-versa.

So ISTM you have to do the analysis if the optimization or warning has
been requested.  Then you conditionalize whether or not the warnings are
emitted by their flag and the optimization based on its flag.


As we discussed in IRC yesterday, the warning and the optimization
are independent of one another, and each controlled by its own option
(-Wformat-length and -fprintf-return-value).  In light of that we've
agreed that submitting both as part of the same patch is sufficient.



I understand you're going to have some further work to do because of
conflicts with David's patches.  With that in mind, I'd suggest a bit of
carving things up so things can start moving forward.


Patch #1.  All the fixes to static buffer sizes that were inspired by
your warning.  These are all approved and can go in immediately.


Attached is this patch.



Patch #2. Improvement to __builtin_object_size to handle
POINTER_PLUS_EXPR on arrays.  This is something that stands on it own
and ought to be reviewable quickly and doesn't really belong in the
bowels of the warning/optimization patch you're developing.


Sure.  I'll submit this patch next.



Patch #3. Core infrastructure and possibly the warning.  The reason I
say possibly the warning is they may be intertwined enough that
separating them makes more work than it saves.  I think the warning bits
are largely ready to go and may just need twiddling due to conflicts
with David's work.

Patch #4. The optimizations you've got now which I'll want to take
another look at.  Other than the overly tight integration with the
warning, I don't see anything inherently wrong, but I would like to take
another look at those once #1-#3 are done and dusted.


As we agreed, these will be submitted as one patch (probably
next week).



Patch #5 and beyond: Further optimization work.


As one of the next steps I'd like to make this feature available
to user-defined sprintf-like functions decorated with attribute
format.  To do that, I'm thinking of adding either a fourth
(optional) argument to attribute format printf indicating which
of the function arguments is the destination buffer (to compute
its size), or perhaps a new attribute under its own name.  I'm
actually leaning toward latter since I think it could be used
in other contexts as well.  I welcome comments and suggestions
on this idea.

Thanks also for the rest of the detailed comments (snipped). I'll
also take care of those requests before I submit the next patch.

Martin
gcc/c-family/ChangeLog:
2016-08-18  Martin Sebor  

	* c-ada-spec.c (dump_ada_function_declaration): Increase buffer
	size to guarantee it fits the output of the formatted function
	regardless of its arguments.

gcc/cp/ChangeLog:
2016-08-18  Martin Sebor  

	* mangle.c: Increase buffer size to guarantee it fits the output
	of the formatted function regardless of its arguments.

gcc/go/ChangeLog:
2016-08-18  Martin Sebor  

	* gofrontend/expressions.cc: Increase buffer size to guarantee
	it fits the output of the formatted function regardless of its
	arguments.

gcc/java/ChangeLog:
2016-08-18  Martin Sebor  

	* decl.c (give_name_to_locals): Increase buffer size to guarantee
	it fits the output of the formatted function regardless of its
	arguments.
	* mangle_name.c (append_unicode_mangled_name): Same.

gcc/ChangeLog:
2016-08-18  Martin Sebor  

	* genmatch.c (parser::parse_expr): Increase buffer size to guarantee
	it fits the output of the formatted function regardless of its
	arguments.
	* gcc/genmodes.c (parser::parse_expr): Same.
	* gimplify.c (gimplify_asm_expr): Same.
	* passes.c (pass_manager::register_one_dump_file): Same.
	* print-tree.c (print_node): Same.

diff --git a/gcc/c-family/c-ada-spec.c b/gcc/c-family/c-ada-spec.c
index a4e0c38..6a8e04b 100644
--- a/gcc/c-family/c-ada-spec.c
+++ b/gcc/c-family/c-ada-spec.c
@@ -1603,7 +1603,7 @@ dump_ada_function_declaration (pretty_printer *buffer, tree func,
 {
   tree arg;
   const tree node = TREE_TYPE (func);
-  char buf[16];
+  char buf[17];
   int num = 0, num_args = 0, have_args = true, have_ellipsis = false;
 
   /* Compute number of arguments.  */
diff --git a/gcc/cp/mangle.c b/gcc/cp/mangle.c
index d8b5c45..5859d62 100644
--- a/gcc/cp/mangle.c
+++ b/gcc/cp/mangle.c
@@ -1740,7 +1740,9 @@ static void
 write_real_cst (const tree value)
 {
   long target_real[4];  /* largest supported float */
-  char buffer[9];   /* eight hex digits in a 32-bit number */
+  /* Buffer for eight hex digits in a 32-bit number but big enough
+ even for 64-bit long to avoid warnings.  */
+  char buffer[17];
   int i, limit, dir;
 
   tree type = TREE_TYPE (value);
diff --git a/gcc/genmatch.c 

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Jeff Law]

On 08/11/2016 08:13 PM, Martin Sebor wrote:

Attached is an updated patch with changes addressing most of
the suggestions and comments I received.

The main changes/improvements are the following:

 *  New option, -fprintf-return-value, enables the sprintf return
value optimization.  It's disabled by default in this version
but I'm hoping to enable in the final version of the patch.
I added it in this version mainly to be able to verify the
correctness of the pass by comparing the sprintf return value
computed by the pass to the expected result returned from
the same libc call.  (Before I had to rely solely on results
hardcoded in the tests).

 *  With the -fdump-tree-printf-return-value the pass generates
simple dump output to indicate what has been optimized and
what hasn't.

 *  Floating point formatting relies on mpfr_snprintf to determine
the size of the output for known values.  This lets the pass
avoid duplicating the tricky floating point math done by sprintf
and getting it wrong.

 *  New target hooks remove hardcoding target-specific assumptions
about libc implementation-specific details (%p format and printf
floating point rounding mode).

What's missing is integration with David's latest location range
changes.  I plan to work on it next but this seemed like a good
stopping point to get feedback.

There also are still opportunities for improvements to the string
length computation (now via gimple-fold.c's get_maxval_strlen,
thanks to Jakub) to improve checking of %s directives.  But since
that's a general-purpose function that's used outside this pass
it will be a separate patch.
My biggest concern with this iteration is the tight integration between 
the optimization and warning.  We generally avoid that kind of tight 
integration such that enabling the warning does not affect the 
optimization and vice-versa.


So ISTM you have to do the analysis if the optimization or warning has 
been requested.  Then you conditionalize whether or not the warnings are 
emitted by their flag and the optimization based on its flag.


I understand you're going to have some further work to do because of 
conflicts with David's patches.  With that in mind, I'd suggest a bit of 
carving things up so things can start moving forward.



Patch #1.  All the fixes to static buffer sizes that were inspired by 
your warning.  These are all approved and can go in immediately.


Patch #2. Improvement to __builtin_object_size to handle 
POINTER_PLUS_EXPR on arrays.  This is something that stands on it own 
and ought to be reviewable quickly and doesn't really belong in the 
bowels of the warning/optimization patch you're developing.


Patch #3. Core infrastructure and possibly the warning.  The reason I 
say possibly the warning is they may be intertwined enough that 
separating them makes more work than it saves.  I think the warning bits 
are largely ready to go and may just need twiddling due to conflicts 
with David's work.


Patch #4. The optimizations you've got now which I'll want to take 
another look at.  Other than the overly tight integration with the 
warning, I don't see anything inherently wrong, but I would like to take 
another look at those once #1-#3 are done and dusted.


Patch #5 and beyond: Further optimization work.







+
+static void
+compute_format_length (const pass_sprintf_length::call_info ,
+  format_result*res,
+  const char   *cvtbeg,
+  size_t   cvtlen,
+  size_t   offset,
+  const conversion_spec,
+  tree arg)
Needs function comment.  Please don't bother lining up arguments like 
that.  Just one space between type and its name.






+/* Given a suitable result RES of a call to a formatted output function
+   described by INFO, substitute the result for the return value of
+   the call.  The result is suitable if the number of bytes it represents
+   is known and exact.  */
+
+static void
+try_substitute_return_value (gimple_stmt_iterator  gsi,
+const pass_sprintf_length::call_info  ,
+const format_result  )
Single space between the type and the variable name.  I only happened to 
see this one because I was going to make a comment about this function, 
so please check elsewhere in your new code.



+{
+  tree lhs = gimple_get_lhs (info.callstmt);
+  if (lhs && res.bounded && res.number_chars < HOST_WIDE_INT_MAX)
+{
+  /* Replace the left-hand side of the call with the constant
+result of the formatting function minus 1 for the terminating
+NUL which the functions' return value does not include.  */
+  gimple_call_set_lhs (info.callstmt, NULL_TREE);
+  tree cst = 

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 08/12/2016 09:48 AM, Joseph Myers wrote:

On Thu, 11 Aug 2016, Martin Sebor wrote:


  *  New target hooks remove hardcoding target-specific assumptions
 about libc implementation-specific details (%p format and printf
 floating point rounding mode).


But the rounding mode may vary at runtime; optimally the conversions
should use the rounding mode set with fesetround.  Determining it with a
target hook doesn't make sense.  What's more appropriate would be
determining both rounded-up and rounded-down strings to get bounds on the
possible length.


Yes, that's a better solution, thanks.  Is there anything else,
particularly in the floating point area, that you think should
be changed or improved?

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Joseph Myers]

On Thu, 11 Aug 2016, Martin Sebor wrote:

>  *  New target hooks remove hardcoding target-specific assumptions
> about libc implementation-specific details (%p format and printf
> floating point rounding mode).

But the rounding mode may vary at runtime; optimally the conversions 
should use the rounding mode set with fesetround.  Determining it with a 
target hook doesn't make sense.  What's more appropriate would be 
determining both rounded-up and rounded-down strings to get bounds on the 
possible length.

> +Warn about function calls with format strings that wite past the end

"write"

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 07/22/2016 04:12 PM, Jeff Law wrote:

Working through the new pass...  Overall it looks pretty good.  There's
a certain level of trust I'll extend WRT getting the low level details
right -- a thorough testsuite obviously helps there.


In the latest patch where I add the return value optimization I also
add a new test that compares the return value computed by the pass
to the value returned by the library call.  That greatly increases
my confidence in the correctness of the pass (though the test still
could stand to be enhanced in a number of ways).


+const pass_data pass_data_sprintf_length = {
+  GIMPLE_PASS,// pass type
+  "sprintf_length",   // pass name
+  OPTGROUP_NONE,  // optinfo_flags
+  TV_NONE,// tv_id
+  PROP_cfg,   // properties_required
+  0, // properties_provided
+  0, // properties_destroyed
+  0, // properties_start
+  0, // properties_finish

So the worry here is that you need PROP_ssa in the late pass.  I'm not
sure if we have the infrastructure to allow you to distinguish the two.
I guess we could literally make it two passes with two distinct
pass_data structures.


I'm not sure I understand the concern.  It sounds like you are
suggesting to add another pass_data struct with PROP_ssa among
the required properties and specify it as an argument to
the gimple_opt pass constructor.  What will it do that what
I have doesn't (make sure the ssa pass runs before this pass?)
and how can I test it?



In handle_gimple_call, you don't handle anti ranges -- I haven't looked
closely at canonicalization rules in VRP, so I don't know if you're
likely to see a range like ![MININT, -1] which would be a synonym for
[0..MAXINT].  It might be worth doing some instrumentation to see if
you're getting useful anti-ranges with any kind of consistency.  ISTM
the most interesting ones are going to allow you to drop or insist on a
"-" sign.


I don't recall seeing any inverted ranges when I was developing
the range tests.  I had high hopes for the range information but
it turned out to be so poor that I decided not to spend time
completing this part.  Once Andrew's new VRP pass is available
I'll come back to it.  If it's as generic as it sounds this pass
will not only be a consumer but also be able to contribute to
improving it.


+  /* As a special case, bounded function allow the explicitly
+specified destination size argument to be zero as a request
+to determine the number of bytes on output without actually
+writing any output.  Disable length checking for those.  */

This doesn't parse well.



+  /* Try to use __builtin_object_size although it rarely returns
+ a useful result even for straighforward cases.  */

Hopefully you've stashed away some tests for this so that we can work on
them independently of the sprintf checking itself?  ISTM that the
recursive handling of POINTER_PLUS_EXPR really ought to move into the
generic builtin_object_size handling itself as an independent change.


Bug 71831 tracks this limitation/enhancement.  I would expect to
be relatively easy to enhance the compute_builtin_object_size
function to return more useful results for some basic expressions
even without optimization, without running the whole objsize pass.
It seems that a good number of other features in GCC (not just
warnings) would benefit from it.


+  /* Bail early if format length checking is disabled, either
+ via a command line option, or as a result of the size of
+ the destination object not being available, or due to
+ format directives or arguments encountered during processing
+ that prevent length tracking with any reliability.  */
+
+  if (HOST_WIDE_INT_MAX <= info.objsize)
+  return;

I think the return is indented too deep.


+  if (*pf == 0)
+   {
+ /* Incomplete directive.  */
+ return;
+   }

For this and the other early returns from compute_format_length, do we
know if -Wformat will catch these errors?  Might that be a low priority
follow-up project if it doesn't?


Being invoked by the front end, -Wformat is limited to checking
constant strings, so it misses problems that a pass that runs
later could detect.  With a bit of effort (to avoid duplicate
warnings) it would be possible to enhance this pass to catch some
these as well.  I chose not to spend time on it in this version
since non-constant format strings are rare but it I agree that
it's something to look into after this patch is in.


In format_floating, we end up using actual host floating point
arithmetic.  That's generally been frowned upon.  We're not doing
anything in terms of code generation here, so ultimately that might be
what allows us to still be safe from a cross compilation standpoint.

It's also not clear if that routine handles the other target floating
point formats.  For things like VAX FP, I'm comfortable issuing a
warning that this stuff isn't 

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 07/29/2016 04:50 PM, Joseph Myers wrote:

On Mon, 18 Jul 2016, Martin Sebor wrote:


+  /* Number of exponent digits or -1 when unknown.  */
+  int expdigs = -1;
+  /* 1 when ARG < 0, 0 when ARG >= 0, -1 when unknown.  */
+  int negative = -1;
+  /* Log10 of EXPDIGS.  */
+  int logexpdigs = 2;
+
+  const double log10_2 = .30102999566398119521;
+
+  if (arg && TREE_CODE (arg) == REAL_CST)
+{
+  expdigs = real_exponent (TREE_REAL_CST_PTR (arg)) * log10_2;
+  negative = real_isneg (TREE_REAL_CST_PTR (arg));
+  logexpdigs = ilog (expdigs, 10);


Thanks for looking at it!  The floating conversion wasn't 100%
complete and the bits that were there I had the least confidence
in so I especially appreciate a second pair of eyes on it.



This looks wrong for the case of a constant with a negative exponent.


You're right -- great catch!  I've fixed it.



Also, what if the constant has a decimal floating-point type?


Decimal floating point isn't handled in this initial implementation.
The checker gives up after the first directive it doesn't recognize
and stops checking the rest of the format string.  Eventually, I'd
like to enhance it to handle decimal floats and other directives
and types.




+}
+  else if (REAL_MODE_FORMAT (TYPE_MODE (type))->b == 2)
+{
+  /* Compute T_MAX_EXP for base 2.  */
+  expdigs = REAL_MODE_FORMAT (TYPE_MODE (type))->emax * log10_2;


Shouldn't you also compute logexpdigs here?  The comment on logexpdigs
implies it's always meant to have a given relation to expdigs.  Or maybe
those variables need to be split into min/max cases, since you're
computing both min/max values below.


I have reworked this so logexpdigs is computed here as well.




+case 'E':
+case 'e':
+  /* The minimum output is "[-+]1.234567e+00" for an IEEE double
+regardless of the value of the actual argument. */
+  res.min = ((0 < negative || spec.get_flag ('+') || spec.get_flag (' '))
++ 1 /* unit */ + (prec < 0 ? 7 : prec ? prec + 1 : 0)
++ 2 /* e+ */ + (logexpdigs < 2 ? 2 : logexpdigs));


As I understand your logic, for the case of a constant expdigs and so
logexpdigs may sometimes be 1 bigger than the correct value for that
constant, say for 9.999e+99, so this may not actually be the minimum.


Yes, %e is an approximation though I hadn't considered this case.
I reworked this part of the implementation to compute an accurate
result.


+case 'G':
+case 'g':
+  /* Treat this the same as '%F' for now even though that's
+inaccurate.  */
+  res.min = 2 + (prec < 0 ? 6 : prec);
+  res.max = ((spec.get_flag ('L') ? 4934 : 310)
++ (prec < 0 ? 6 : prec ? prec + 1 : 0));


Again, avoid embedding properties of given formats.


I've removed the IEEE 754 assumptions from both the code and the
comments.

I will post the latest version of the patch shortly.

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Jeff Law]

On 08/04/2016 03:13 PM, Martin Sebor wrote:

On 07/21/2016 03:48 PM, Jeff Law wrote:




Sorry about the delay in responding...


I saw a few places in GCC itself where you increased buffer sizes.  Were
any of those level 1 failures?


Yes.  With optimization, even level 1 uses range information when
it's available.  The idea is that when the range of values of the
variable has been constrained (possibly to avoid a sprintf buffer
overflow) but wasn't constrained enough, it's a possible sign of
a bug.

In some cases such as when the resulting range is the result of
a cast from signed to unsigned (as in cp/mangle.c) it triggers
a diagnostic that wouldn't be issued in its absence.  Here's
a small example:

  char d [9];

  void f (long *x)
  {
// warning here
__builtin_sprintf (d, "%08lx", (unsigned long)*x);
  }

  void g (long *x)
  {
// no warning here
__builtin_sprintf (d, "%08lx", *x);
  }

It's probably possible to tweak the pass to try to detect this
case and avoid the warning, but I'm not sure it's worth it.  What
do you think?
I don't think it's worth it to detect and avoid the warning right now. 
That may change as the warning sees more widespread use after going onto 
the trunk.





Right.  I'm not sure what the right solution is.  The biggest
difference between the levels is in the values they assume
unknown integers can take on.  Level one assumes 1, level two
assumes a value that results in the longest output (such as
INT_MIN).  This can cause false positives but in my experience
there aren't too many of those.  I would be comfortable making
level 2 the default if we had good range information.  Without
it, though, even some obviously safe cases tend to be diagnosed,
like this one:

  char d [4];

  void g (int i)
  {
if (0 <= i && i < 1000)
  __builtin_sprintf (d, "%i", i);
  }

  warning: ‘%i’ directive writing between 1 and 11 bytes into a region
of size 4 [-Wformat-length=]
   __builtin_sprintf (d, "%i", i);
  ^~
  note: using the range [‘1’, ‘-2147483648’] for directive argument
  note: format output between 2 and 12 bytes into a destination of size 4

Maybe leave level 1 the default for now and bump it up to 2 when
the new and improved VRP pass is in?
This is probably as much of a user education problem as anything.  I 
definitely want us to be level 1 clean for gcc-7.  I think we'll want to 
evaluate the pain of getting to level 2 clean once Andrew's work is far 
enough along to evaluate how many false positives it allows us to eliminate.


jeff

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 07/21/2016 03:48 PM, Jeff Law wrote:




Sorry about the delay in responding...


I saw a few places in GCC itself where you increased buffer sizes.  Were
any of those level 1 failures?


Yes.  With optimization, even level 1 uses range information when
it's available.  The idea is that when the range of values of the
variable has been constrained (possibly to avoid a sprintf buffer
overflow) but wasn't constrained enough, it's a possible sign of
a bug.

In some cases such as when the resulting range is the result of
a cast from signed to unsigned (as in cp/mangle.c) it triggers
a diagnostic that wouldn't be issued in its absence.  Here's
a small example:

  char d [9];

  void f (long *x)
  {
// warning here
__builtin_sprintf (d, "%08lx", (unsigned long)*x);
  }

  void g (long *x)
  {
// no warning here
__builtin_sprintf (d, "%08lx", *x);
  }

It's probably possible to tweak the pass to try to detect this
case and avoid the warning, but I'm not sure it's worth it.  What
do you think?



In c.opt:


+C ObjC C++ ObjC++ Warning Alias (Wformat-length=, 1, 0)

Can't this take on values 0, 1, 2? And if so, is the "1" above correct?

In invoke.texi we have:

+-Wno-format-contains-nul -Wno-format-extra-args -Wformat-length=2 @gol
Note Wformat-length=2.

Then in the actual documentation it says the default level is 1.

+in output truncation.  GCC counts the number of bytes that each format
+string and directive within it writes into the provided buffer and, when
+it detects that more bytes that fit in the destination buffer may be
output,
+it emits a warning.


s/that fit/than fit

In that same section is the text which says the default level is 1


Good catch!  The default level is meant to be 1, so the manual was
wrong. I've fixed it.



I'm curious about the heuristics for level 1.  I guess if you get a
warning at level 1, then you've got a situation where we know we'll
write out of bounds.  level 2 is a may write out of bounds.  Which makes
me wonder if rather than levels we should use symbolic names?

I guess my worry here is that if we don't message this right folks will
make their code level 1 clean and think they're done.  When in reality
all they've done is mitigate the most egregious problems.


Right.  I'm not sure what the right solution is.  The biggest
difference between the levels is in the values they assume
unknown integers can take on.  Level one assumes 1, level two
assumes a value that results in the longest output (such as
INT_MIN).  This can cause false positives but in my experience
there aren't too many of those.  I would be comfortable making
level 2 the default if we had good range information.  Without
it, though, even some obviously safe cases tend to be diagnosed,
like this one:

  char d [4];

  void g (int i)
  {
if (0 <= i && i < 1000)
  __builtin_sprintf (d, "%i", i);
  }

  warning: ‘%i’ directive writing between 1 and 11 bytes into a region 
of size 4 [-Wformat-length=]

   __builtin_sprintf (d, "%i", i);
  ^~
  note: using the range [‘1’, ‘-2147483648’] for directive argument
  note: format output between 2 and 12 bytes into a destination of size 4

Maybe leave level 1 the default for now and bump it up to 2 when
the new and improved VRP pass is in?


In passes.c:


+
+  // inform (0, "executing pass: %s", pass->name);
+
   if (execute_one_pass (pass) && pass->sub)
-execute_pass_list_1 (pass->sub);
+   {
+ // inform (0, "executing subpass: %s", pass->sub->name);
+ execute_pass_list_1 (pass->sub);
+   }
+



The comments left over from debugging?


Yes.  Someone else pointed it out in the first patch and I forgot
to remove it.  It's gone now.



Looks like ~1/3 of the patch are tests.  Good stuff.  I'm going to
assume those are correct.


They should be for the most part.  As we've discussed, I used actual
sprintf calls to validate them within GCC itself.  Now that I've
removed those calls from the GCC source I need to add tests to verify
that the return value computed by the pass for the sprintf calls is
what the call actually returns.  I have added a new test that does
that.  I'll post an updated patch soon.

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Joseph Myers]

On Mon, 18 Jul 2016, Martin Sebor wrote:

> +  /* Number of exponent digits or -1 when unknown.  */
> +  int expdigs = -1;
> +  /* 1 when ARG < 0, 0 when ARG >= 0, -1 when unknown.  */
> +  int negative = -1;
> +  /* Log10 of EXPDIGS.  */
> +  int logexpdigs = 2;
> +
> +  const double log10_2 = .30102999566398119521;
> +
> +  if (arg && TREE_CODE (arg) == REAL_CST)
> +{
> +  expdigs = real_exponent (TREE_REAL_CST_PTR (arg)) * log10_2;
> +  negative = real_isneg (TREE_REAL_CST_PTR (arg));
> +  logexpdigs = ilog (expdigs, 10);

This looks wrong for the case of a constant with a negative exponent.

Also, what if the constant has a decimal floating-point type?

> +}
> +  else if (REAL_MODE_FORMAT (TYPE_MODE (type))->b == 2)
> +{
> +  /* Compute T_MAX_EXP for base 2.  */
> +  expdigs = REAL_MODE_FORMAT (TYPE_MODE (type))->emax * log10_2;

Shouldn't you also compute logexpdigs here?  The comment on logexpdigs 
implies it's always meant to have a given relation to expdigs.  Or maybe 
those variables need to be split into min/max cases, since you're 
computing both min/max values below.

> +case 'E':
> +case 'e':
> +  /* The minimum output is "[-+]1.234567e+00" for an IEEE double
> +  regardless of the value of the actual argument. */
> +  res.min = ((0 < negative || spec.get_flag ('+') || spec.get_flag (' '))
> +  + 1 /* unit */ + (prec < 0 ? 7 : prec ? prec + 1 : 0)
> +  + 2 /* e+ */ + (logexpdigs < 2 ? 2 : logexpdigs));

As I understand your logic, for the case of a constant expdigs and so 
logexpdigs may sometimes be 1 bigger than the correct value for that 
constant, say for 9.999e+99, so this may not actually be the minimum.

> +  /* The maximum output is "-1.234567e+123" for a double and one
> +  more byte for a large exponent for a long louble.  */
> +  res.max = negative < 0 ? res.min + 2 + (spec.get_flag ('L')) : res.min;

No, don't embed such assumptions about particular formats.  Compute 
correct max/min values based on the exponent range of the format in 
question.

> +  /* The maximum depends on the magnitude of the value but it's
> +  at most 316 bytes for double and 4940 for long double, plus
> +  precision if non-negative, or 6.  */

I don't think comments should embed such assumptions, either.

> +case 'G':
> +case 'g':
> +  /* Treat this the same as '%F' for now even though that's
> +  inaccurate.  */
> +  res.min = 2 + (prec < 0 ? 6 : prec);
> +  res.max = ((spec.get_flag ('L') ? 4934 : 310)
> +  + (prec < 0 ? 6 : prec ? prec + 1 : 0));

Again, avoid embedding properties of given formats.

(I realise tree-call-cdce.c embeds some such assumptions, but that's bad 
practice that should not be repeated.)

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Jeff Law]

Working through the new pass...  Overall it looks pretty good.  There's 
a certain level of trust I'll extend WRT getting the low level details 
right -- a thorough testsuite obviously helps there.





+const pass_data pass_data_sprintf_length = {
+  GIMPLE_PASS,// pass type
+  "sprintf_length",   // pass name
+  OPTGROUP_NONE,  // optinfo_flags
+  TV_NONE,// tv_id
+  PROP_cfg,   // properties_required
+  0, // properties_provided
+  0, // properties_destroyed
+  0, // properties_start
+  0, // properties_finish
So the worry here is that you need PROP_ssa in the late pass.  I'm not 
sure if we have the infrastructure to allow you to distinguish the two. 
I guess we could literally make it two passes with two distinct 
pass_data structures.



In handle_gimple_call, you don't handle anti ranges -- I haven't looked 
closely at canonicalization rules in VRP, so I don't know if you're 
likely to see a range like ![MININT, -1] which would be a synonym for 
[0..MAXINT].  It might be worth doing some instrumentation to see if 
you're getting useful anti-ranges with any kind of consistency.  ISTM 
the most interesting ones are going to allow you to drop or insist on a 
"-" sign.





+  /* As a special case, bounded function allow the explicitly
+specified destination size argument to be zero as a request
+to determine the number of bytes on output without actually
+writing any output.  Disable length checking for those.  */

This doesn't parse well.



+  /* Try to use __builtin_object_size although it rarely returns
+ a useful result even for straighforward cases.  */
Hopefully you've stashed away some tests for this so that we can work on 
them independently of the sprintf checking itself?  ISTM that the 
recursive handling of POINTER_PLUS_EXPR really ought to move into the 
generic builtin_object_size handling itself as an independent change.





+  /* Bail early if format length checking is disabled, either
+ via a command line option, or as a result of the size of
+ the destination object not being available, or due to
+ format directives or arguments encountered during processing
+ that prevent length tracking with any reliability.  */
+
+  if (HOST_WIDE_INT_MAX <= info.objsize)
+  return;

I think the return is indented too deep.


+  if (*pf == 0)
+   {
+ /* Incomplete directive.  */
+ return;
+   }
For this and the other early returns from compute_format_length, do we 
know if -Wformat will catch these errors?  Might that be a low priority 
follow-up project if it doesn't?




+static void
+add_bytes (const pass_sprintf_length::call_info ,
+  const char   *beg,
+  const char   *end,
+  format_result*res)
In general, don't try to line up parameters, args or variable 
declarations like you've done here.  Similarly in other places.



+  /* if (warn_format_length */
+  /* && (overflowed || navail < nbytes */
+  /* || (1 < warn_format_length && )) */

Presumably old implementation and/or debugging code...  Please remove it.

Please check indention of the curleys & code block in the loop over the 
phi arguments in get_string_length.



In format_floating, we end up using actual host floating point 
arithmetic.  That's generally been frowned upon.  We're not doing 
anything in terms of code generation here, so ultimately that might be 
what allows us to still be safe from a cross compilation standpoint.


It's also not clear if that routine handles the other target floating 
point formats.  For things like VAX FP, I'm comfortable issuing a 
warning that this stuff isn't supported on that target.   We have other 
targets where a double might be the same size as a float.   Though I 
guess in that case the worst we get is false positives, so that may not 
be a huge issue either.  I'm not sure how to check for this stuff 
without bringing in aspects of the target though, which we'd like to avoid.


In format_integer you have target specific ifdefs (solaris and aix). 
First we want to avoid conditionally compiled code.  Second, from a 
modularity something like TARGET_AIX and TARGET_SOLARIS really aren't 
appropriate in here. I suspect what you're going to want to do is add 
something to the targetm structure that you can query and/or call here 
is the best we can do.


Again, overall it looks good.  My biggest concern is format_integer and 
format_float and the bleeding of target dependencies into this code.  To 
some degree it may be unavoidable and we can mitigate the damage with 
stuff in targetm -- I'd like to hear your thoughts.


Jeff

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Jakub Jelinek]

On Mon, Jul 18, 2016 at 03:59:11PM -0600, Martin Sebor wrote:
> +  /* Try to use __builtin_object_size although it rarely returns
> + a useful result even for straighforward cases.  */
> +  tree ost = warn_format_length < 2
> +? integer_zero_node : build_int_cst (size_type_node, 2);
> +  tree args[] = { dest, ost };
> +  tree func = builtin_decl_explicit (BUILT_IN_OBJECT_SIZE);
> +  if (tree size = fold_builtin_n (UNKNOWN_LOCATION, func, args, 2, false))

What is the point of going through fold etc.?  You can just
(for ADDR_EXPR and SSA_NAME) call compute_builtin_object_size without,
and don't have to convert the result back to tree and then back to uhwi.

> +return tree_to_uhwi (STRIP_NOPS  (size));

Formatting.
> +
> +  /* If __builtin_object_size fails to deliver, try to compute
> + it for the very basic (but common) cases.  */
> +  if (TREE_CODE (dest) == SSA_NAME
> +  && POINTER_TYPE_P (TREE_TYPE (dest)))
> +{
> +  gimple *def = SSA_NAME_DEF_STMT (dest);
> +  if (gimple_code (def) == GIMPLE_ASSIGN)

is_gimple_assign (def) ?

> + {
> +   tree_code code = gimple_assign_rhs_code (def);
> +   if (code == POINTER_PLUS_EXPR)
> + {
> +   tree off = gimple_assign_rhs2 (def);
> +   dest = gimple_assign_rhs1 (def);
> +
> +   if (cst_and_fits_in_hwi (off))
> + {
> +   unsigned HOST_WIDE_INT size = get_destination_size (dest);
> +   if (size != HOST_WIDE_INT_M1U)
> + return size - tree_to_shwi (off);
> + }

I think you need to be very careful on negative offsets here (or don't allow
them).

> + }

Shouldn't this have some upper bound for the recursion?
E.g. PARAM_VALUE (PARAM_MAX_SSA_NAME_QUERY_DEPTH)?
> + }
> +}
> +
> +  return -1;
> +}

Jakub

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Jeff Law]

On 07/21/2016 03:57 PM, Martin Sebor wrote:


The pass only runs once.  Without optimization, it's invoked early,
does its thing, and is never invoked again.  With optimization,
it's also invoked early but returns without doing anything, only
to to invoked again much later on to do real work.  I wasn't sure
where among all the passes might be the best spot to insert it so
I put in the same place as Aldy's -Walloca patch because it relies
on the same prior passes.
Seems reasonable.  I'm not sure were best to put the late call either; 
it's a balancing act many passes have opportunities to simplify stuff in 
ways that may make a false positive go away, but many passes can also 
muck up the range information that you're likely depending on.


I'd say stick with its current position in the pipeline, then we should 
reevaluate after some of the range reimplementation lands.


jeff

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

To that end, the attached patch adds the checker under its own new
pass.  The pass runs very early without optimization, and relatively
late with it to benefit from the passes above.  With optimization
the pass also can (but doesn't yet) fold the return value from these
functions into a constant.  In the Linux kernel, it folds 88 snprintf
calls (I didn't find any instances where the whole call could be
folded into a string constant).

So I haven't dug into the patch yet, but what (if any) interaction is
there between the early and late passes?  Do they both emit warnings, or
is it more that the early passe builds a candidate set which is refined
by the later pass, or something totally different?


Thanks for the feedback.

The pass only runs once.  Without optimization, it's invoked early,
does its thing, and is never invoked again.  With optimization,
it's also invoked early but returns without doing anything, only
to to invoked again much later on to do real work.  I wasn't sure
where among all the passes might be the best spot to insert it so
I put in the same place as Aldy's -Walloca patch because it relies
on the same prior passes.


I tested the patch with LTO but due to bug 71907 no warnings are
issued.  Once the bug is resolved I'll re-test it and see about
adding test cases to verify it.

Note Richi checked in a patch a couple days ago to stream the abstract
origins, which is supposed to fix 71907.


Thanks for the heads up.  I'll give it a whirl.


Onward to looking at the patch :-)


Re-reading it again myself I see a few typos, commented out chunks
of code left over from debugging, and missing comments.  I'll clean
this up in the next update along with other requested changes.

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Jeff Law]

I saw a few places in GCC itself where you increased buffer sizes.  Were 
any of those level 1 failures?


In c.opt:


+C ObjC C++ ObjC++ Warning Alias (Wformat-length=, 1, 0)

Can't this take on values 0, 1, 2? And if so, is the "1" above correct?

In invoke.texi we have:

+-Wno-format-contains-nul -Wno-format-extra-args -Wformat-length=2 @gol
Note Wformat-length=2.

Then in the actual documentation it says the default level is 1.

+in output truncation.  GCC counts the number of bytes that each format
+string and directive within it writes into the provided buffer and, when
+it detects that more bytes that fit in the destination buffer may be output,
+it emits a warning.


s/that fit/than fit

In that same section is the text which says the default level is 1

I'm curious about the heuristics for level 1.  I guess if you get a 
warning at level 1, then you've got a situation where we know we'll 
write out of bounds.  level 2 is a may write out of bounds.  Which makes 
me wonder if rather than levels we should use symbolic names?


I guess my worry here is that if we don't message this right folks will 
make their code level 1 clean and think they're done.  When in reality 
all they've done is mitigate the most egregious problems.


Elsewhere in invoke.texi:


+buffer in an inforational note following the warning.

informational I assume


+the minimum buffer size.  For exampe, if @var{a} and @var{b} above can

example

In genmatch.c:

+  char id[13];   /* Big enough for a 32-bit UINT_MAX.  */
In general we don't add comments to the end of a line like that.  If a 
comment is needed, put it before the declaration.  I'm not sure if a 
comment is needed here or not -- your call.


Similarly for various other places were you increased buffer lengths.


In passes.c:


+
+  // inform (0, "executing pass: %s", pass->name);
+
   if (execute_one_pass (pass) && pass->sub)
-execute_pass_list_1 (pass->sub);
+   {
+ // inform (0, "executing subpass: %s", pass->sub->name);
+ execute_pass_list_1 (pass->sub);
+   }
+



The comments left over from debugging?

Looks like ~1/3 of the patch are tests.  Good stuff.  I'm going to 
assume those are correct.


I'll get into gimple-ssa-sprintf.c either later tonight or tomorrow. 
But so far nothing major, just nits.


Jeff

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Jeff Law]

On 07/18/2016 03:59 PM, Martin Sebor wrote:


Unfortunately, linking with C_COMMON_OBJS isn't enough and linking
with C_OBJS doesn't work because of multiple definitions for symbols
like gt_ggc_mx_language_function.  I don't know this part of of GCC
but it seems that each front end gets a set of these generated
functions, some with the same names.  I spent a couple of hours
trying to get it to work but eventually gave up.
Correct.  gt_ggc_mx_* are generated functions to support the garbage 
collection system.  In this specific case, I think it's the GC bits for 
struct language_function which is specific to each front-end.  So each 
front-end is going to have that function hence the multiple definitions.




The only option left to make it work with LTO is to extract the
checker from c-format.c and move it where it gets linked with lto1.

I think you're right.



To that end, the attached patch adds the checker under its own new
pass.  The pass runs very early without optimization, and relatively
late with it to benefit from the passes above.  With optimization
the pass also can (but doesn't yet) fold the return value from these
functions into a constant.  In the Linux kernel, it folds 88 snprintf
calls (I didn't find any instances where the whole call could be
folded into a string constant).
So I haven't dug into the patch yet, but what (if any) interaction is 
there between the early and late passes?  Do they both emit warnings, or 
is it more that the early passe builds a candidate set which is refined 
by the later pass, or something totally different?





I tested the patch with LTO but due to bug 71907 no warnings are
issued.  Once the bug is resolved I'll re-test it and see about
adding test cases to verify it.
Note Richi checked in a patch a couple days ago to stream the abstract 
origins, which is supposed to fix 71907.





In the updated patch I tried to address the majority of everyone's
comments, including those I got from the bug submitter.  I also
made a number of enhancements based on the results I saw with the
Linux kernel and other code (e.g., support for %p, additional
heuristics to improve %s coverage, and support for POSIX numbered
arguments).

Once a patch along these lines is approved and committed, I'd like
to enhance it by adding one or more of the following:

 *  -fdump-sprintf-length option to have the pass dump details
about opportunities to fold expressions as well as instances
where the checker was unable to check a call because of lack
of object size or argument value or range information.

That'll definitely be useful as we've discussed in the past.



 *  Support for the return value folding (I have implemented and
lightly tested thid but I would prefer to treat it it as
a separate enhancement independent of this one).

Seems reasonable to me.



 *  If/when David's patch for on-demand locations within string
literals is accepted and committed
  https://gcc.gnu.org/ml/gcc-patches/2016-07/msg00441.html
replace the location handling code I copied from c-format.c
with the new API.

Also sounds good.



 *  Enhance the heuristics usesd to find likely %s argument
lengths to improve the checker's coverage.

 *  Add support for %n and perhaps other functions (e.g., scanf).

Also good stuff.

Onward to looking at the patch :-)

jeff

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Manuel López-Ibáñez]

On 01/07/16 19:15, Martin Sebor wrote:
+ /* Differentiate between an exact and inexact buffer overflow
+or truncation.  */
+ const char *fmtstr;
+ if (res->number_chars < 0)
+   fmtstr = info->bounded
+ ? "output may be truncated at or before format character "
+   "%qc at offset %qlu past the end of a region of size %qlu"
+ : "writing format character %qc at offset %qlu "
+   "in a region of size %qlu";
+ else
+   fmtstr = info->bounded
+ ? "output truncated at format character %qc at offset %qlu "
+   "just past the end of a region of size %qlu"
+ : "writing format character %qc at offset %qlu "
+   "just past the end of a region of size %qlu";
+ warning_at (loc, OPT_Wformat_length_, fmtstr,
+ format_chars [-1], off - 1,
+ (unsigned long)info->objsize);
+   }


I'm not sure gettext can parse the text of format strings given like this. It 
may be smarter enough if the conditional expression is directly the argument to 
warning_at. GCC's -Wformat has the same limitations. Of course, the fool-proof 
way is to use multiple calls to warning_at.


Cheers,
Manuel.

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[David Malcolm]

On Tue, 2016-07-12 at 16:35 -0600, Martin Sebor wrote:
> On 07/12/2016 10:12 AM, Manuel López-Ibáñez wrote:
> > On 12/07/16 16:59, Martin Sebor wrote:
> > > You're probably right.  I suspect I have a tendency to overuse
> > > the quotes (e.g, the -Wplacement-new warning also quotes the
> > > sizes).  If there aren't yet (I vague recall coming across
> > > something on the GCC Wiki but can't find it now), it would be
> > > helpful to put in place some diagnostic style conventions like
> > > there are for formatting code to guide us in cases like this.
> > > I'm willing to help put the document together or add this to
> > > it if one already exists.
> > 
> > https://gcc.gnu.org/wiki/DiagnosticsGuidelines
> 
> That's it!  Thanks!  Looks like there are two places that talk
> about GCC diagnostics: one on the Wiki and one in the Coding
> Conventions (plus the GNU Coding Standard).  But, AFAICS, none
> of these gives guidance for what to quote.
> 
> Based on the gcc.pot file it does look like quoted numbers are
> far less common than unquoted ones (just 10 messages where they
> are quoted vs 528 unquoted).
> 
> I've added this as a guideline to the Wiki and assuming no one
> suggests otherwise I'll remove the quotes from this patch and
> from the other changes I already committed.

For reference, Martin's wiki change was:
https://gcc.gnu.org/wiki/DiagnosticsGuidelines?action=diff=8=7

(looks good to me, fwiw)

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Marek Polacek]

On Tue, Jul 12, 2016 at 04:35:50PM -0600, Martin Sebor wrote:
> Based on the gcc.pot file it does look like quoted numbers are
> far less common than unquoted ones (just 10 messages where they
> are quoted vs 528 unquoted).
> 
> I've added this as a guideline to the Wiki and assuming no one
> suggests otherwise I'll remove the quotes from this patch and
> from the other changes I already committed.

I will support the change as I don't see any need for quoting numbers,
as opposed to e.g. identifiers.

Marek

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 07/12/2016 10:12 AM, Manuel López-Ibáñez wrote:

On 12/07/16 16:59, Martin Sebor wrote:

You're probably right.  I suspect I have a tendency to overuse
the quotes (e.g, the -Wplacement-new warning also quotes the
sizes).  If there aren't yet (I vague recall coming across
something on the GCC Wiki but can't find it now), it would be
helpful to put in place some diagnostic style conventions like
there are for formatting code to guide us in cases like this.
I'm willing to help put the document together or add this to
it if one already exists.


https://gcc.gnu.org/wiki/DiagnosticsGuidelines


That's it!  Thanks!  Looks like there are two places that talk
about GCC diagnostics: one on the Wiki and one in the Coding
Conventions (plus the GNU Coding Standard).  But, AFAICS, none
of these gives guidance for what to quote.

Based on the gcc.pot file it does look like quoted numbers are
far less common than unquoted ones (just 10 messages where they
are quoted vs 528 unquoted).

I've added this as a guideline to the Wiki and assuming no one
suggests otherwise I'll remove the quotes from this patch and
from the other changes I already committed.

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 07/12/2016 06:32 AM, Bernd Schmidt wrote:

On 07/01/2016 08:15 PM, Martin Sebor wrote:

The attached patch enhances compile-time checking for buffer overflow
and output truncation in non-trivial calls to the sprintf family of
functions under a new option -Wformat-length=[12].  This initial
patch handles printf directives with string, integer, and simple
floating arguments but eventually I'd like to extend it all other
functions and directives for which it makes sense.


On the whole I think this looks good.


Thanks the detailed review!


Beyond these I have no objections to the patch but ideally a C frontend
maintainer would given an explicit ack as well.


In response to prior comments from Jakub and Richard I have actually
moved the patch to the middle end, into a pass of its own where it
works with LTO, and where it can also be used to optimize branches
based on the functions' return value (when they are known to be
exact).

I will make the changes you suggested (those that apply) and post
an updated patch for review soon that should be closer to final
than the initial prototype.

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Manuel López-Ibáñez]

On 12/07/16 16:59, Martin Sebor wrote:

You're probably right.  I suspect I have a tendency to overuse
the quotes (e.g, the -Wplacement-new warning also quotes the
sizes).  If there aren't yet (I vague recall coming across
something on the GCC Wiki but can't find it now), it would be
helpful to put in place some diagnostic style conventions like
there are for formatting code to guide us in cases like this.
I'm willing to help put the document together or add this to
it if one already exists.


https://gcc.gnu.org/wiki/DiagnosticsGuidelines

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

Does it warn for the code that Florian actually posted?  I tried it
with a recent (unpatched) build of trunk and got no warning (with -O2
-Wall -D_FORTIFY_SOURCE=2), but it strikes me that we ought to warn if
someone passes about the above code (for the uninitialized format
string, at least; I don't know if it's legal to pass a string literal
as the destination).

Should I file a PR for this?


It doesn't warn for it because in C, string literals are of type
char[] and so implicitly convertible to char* (shudder).  GCC
does warn for it with -Wwrite-strings or -Wdiscarded-qualifiers,
and will give a pedantic warning in C++ (it seems an error would
be preferable).




Thanks for giving it a try!  Based on the feedback I received
I've since updated the patch and will post the latest version
for review soon.  In simple cases like this one it warns even
without _FORTIFY_SOURCE or optimization (though without the
latter it doesn't benefit from VRP information).  Let me see
about adding a warning to detect and warn when the arguments
are transposed.

$ /build/gcc-49905/gcc/xgcc -B /build/gcc-49905/gcc -S -Wall -Wextra
-Wpedantic -Wformat-length=2 xyz.c

xyz.c: In function ‘format_1’:
xyz.c:13:29: warning: may write a terminating nul past the end of the
destination [-Wformat-length=]
 sprintf (buf, "%u.%u.%u.%u", a, b, c, d);
   ^
xyz.c:13:3: note: destination size is ‘15’ bytes, output size between
‘8’ and ‘16’
 sprintf (buf, "%u.%u.%u.%u", a, b, c, d);
 ^~~~


Style question: should the numbers in these diagnostic messages be in
quotes?  That looks a bit strange to me.  It seems clearer to me to
have:


You're probably right.  I suspect I have a tendency to overuse
the quotes (e.g, the -Wplacement-new warning also quotes the
sizes).  If there aren't yet (I vague recall coming across
something on the GCC Wiki but can't find it now), it would be
helpful to put in place some diagnostic style conventions like
there are for formatting code to guide us in cases like this.
I'm willing to help put the document together or add this to
it if one already exists.

Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[David Malcolm]

On Tue, 2016-07-12 at 08:45 -0600, Martin Sebor wrote:
> On 07/12/2016 04:01 AM, Florian Weimer wrote:
> > On 07/12/2016 11:54 AM, Jakub Jelinek wrote:
> > > On Tue, Jul 12, 2016 at 11:51:50AM +0200, Florian Weimer wrote:
> > > > On 07/01/2016 08:15 PM, Martin Sebor wrote:
> > > > > The attached patch enhances compile-time checking for buffer
> > > > > overflow
> > > > > and output truncation in non-trivial calls to the sprintf
> > > > > family of
> > > > > functions under a new option -Wformat-length=[12].  This
> > > > > initial
> > > > > patch handles printf directives with string, integer, and
> > > > > simple
> > > > > floating arguments but eventually I'd like to extend it all
> > > > > other
> > > > > functions and directives for which it makes sense.
> > > > 
> > > > I tried your patch with the following code, which is close to a
> > > > real-world
> > > > example:
> > > > 
> > > > #include 
> > > > 
> > > > void print (const char *);
> > > > 
> > > > void
> > > > format_1 (unsigned address)
> > > > {
> > > >   unsigned char a = address >> 24;
> > > >   unsigned char b = address >> 16;
> > > >   unsigned char c = address >> 8;
> > > >   unsigned char d = address;
> > > >   char buf[15];
> > > >   sprintf ("%u.%u.%u.%u", buf, a, b, c, d);
> > > 
> > > Are you sure this is real-world code?  sprintf's first argument
> > > is the
> > > buffer and second the format string, so if this doesn't warn at
> > > compile
> > > time, it will surely crash at runtime when trying to store into
> > > .rodata.
> > 
> > Argh!  You are right, I swapped the arguments.
> > 
> > And further attempts showed that I was missing -D_FORTIFY_SOURCE=2.
> > With
> > it, I get a nice diagnostic.  Wow!

Does it warn for the code that Florian actually posted?  I tried it
with a recent (unpatched) build of trunk and got no warning (with -O2 
-Wall -D_FORTIFY_SOURCE=2), but it strikes me that we ought to warn if
someone passes about the above code (for the uninitialized format
string, at least; I don't know if it's legal to pass a string literal
as the destination).

Should I file a PR for this?

> Thanks for giving it a try!  Based on the feedback I received
> I've since updated the patch and will post the latest version
> for review soon.  In simple cases like this one it warns even
> without _FORTIFY_SOURCE or optimization (though without the
> latter it doesn't benefit from VRP information).  Let me see
> about adding a warning to detect and warn when the arguments
> are transposed.
> 
> $ /build/gcc-49905/gcc/xgcc -B /build/gcc-49905/gcc -S -Wall -Wextra 
> -Wpedantic -Wformat-length=2 xyz.c
> 
> xyz.c: In function ‘format_1’:
> xyz.c:13:29: warning: may write a terminating nul past the end of the
> destination [-Wformat-length=]
> sprintf (buf, "%u.%u.%u.%u", a, b, c, d);
>   ^
> xyz.c:13:3: note: destination size is ‘15’ bytes, output size between
> ‘8’ and ‘16’
> sprintf (buf, "%u.%u.%u.%u", a, b, c, d);
> ^~~~

Style question: should the numbers in these diagnostic messages be in
quotes?  That looks a bit strange to me.  It seems clearer to me to
have:

xyz.c:13:3: note: destination size is 15 bytes, output size between 8
and 16

> xyz.c: In function ‘format_2’:
> xyz.c:21:3: warning: ‘%u’ directive writing between ‘1’ and ‘10’
> bytes 
> into a region of size ‘4’ [-Wformat-length=]
> sprintf (buf, "%u.%u.%u.%u",
> ^
> xyz.c:21:3: note: using the range [‘1u’, ‘2147483648u’] for directive
> argument
> xyz.c:21:3: note: destination size is ‘15’ bytes, output size between
> ‘4’ and ‘22’
> sprintf (buf, "%u.%u.%u.%u",
> ^~~~
>  address >> 24,
>  ~~
>  (address >> 16) & 0xff,
>  ~~~
>  (address >> 8) & 0xff,
>  ~~
>  address & 0xff);
>  ~~~
> xyz.c:21:3: note: destination size is ‘15’ bytes, output size between
> ‘6’ and ‘33’
> 
> Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Martin Sebor]

On 07/12/2016 04:01 AM, Florian Weimer wrote:

On 07/12/2016 11:54 AM, Jakub Jelinek wrote:

On Tue, Jul 12, 2016 at 11:51:50AM +0200, Florian Weimer wrote:

On 07/01/2016 08:15 PM, Martin Sebor wrote:

The attached patch enhances compile-time checking for buffer overflow
and output truncation in non-trivial calls to the sprintf family of
functions under a new option -Wformat-length=[12].  This initial
patch handles printf directives with string, integer, and simple
floating arguments but eventually I'd like to extend it all other
functions and directives for which it makes sense.


I tried your patch with the following code, which is close to a
real-world
example:

#include 

void print (const char *);

void
format_1 (unsigned address)
{
  unsigned char a = address >> 24;
  unsigned char b = address >> 16;
  unsigned char c = address >> 8;
  unsigned char d = address;
  char buf[15];
  sprintf ("%u.%u.%u.%u", buf, a, b, c, d);


Are you sure this is real-world code?  sprintf's first argument is the
buffer and second the format string, so if this doesn't warn at compile
time, it will surely crash at runtime when trying to store into .rodata.


Argh!  You are right, I swapped the arguments.

And further attempts showed that I was missing -D_FORTIFY_SOURCE=2. With
it, I get a nice diagnostic.  Wow!


Thanks for giving it a try!  Based on the feedback I received
I've since updated the patch and will post the latest version
for review soon.  In simple cases like this one it warns even
without _FORTIFY_SOURCE or optimization (though without the
latter it doesn't benefit from VRP information).  Let me see
about adding a warning to detect and warn when the arguments
are transposed.

$ /build/gcc-49905/gcc/xgcc -B /build/gcc-49905/gcc -S -Wall -Wextra 
-Wpedantic -Wformat-length=2 xyz.c


xyz.c: In function ‘format_1’:
xyz.c:13:29: warning: may write a terminating nul past the end of the 
destination [-Wformat-length=]

   sprintf (buf, "%u.%u.%u.%u", a, b, c, d);
 ^
xyz.c:13:3: note: destination size is ‘15’ bytes, output size between 
‘8’ and ‘16’

   sprintf (buf, "%u.%u.%u.%u", a, b, c, d);
   ^~~~
xyz.c: In function ‘format_2’:
xyz.c:21:3: warning: ‘%u’ directive writing between ‘1’ and ‘10’ bytes 
into a region of size ‘4’ [-Wformat-length=]

   sprintf (buf, "%u.%u.%u.%u",
   ^
xyz.c:21:3: note: using the range [‘1u’, ‘2147483648u’] for directive 
argument
xyz.c:21:3: note: destination size is ‘15’ bytes, output size between 
‘4’ and ‘22’

   sprintf (buf, "%u.%u.%u.%u",
   ^~~~
address >> 24,
~~
(address >> 16) & 0xff,
~~~
(address >> 8) & 0xff,
~~
address & 0xff);
~~~
xyz.c:21:3: note: destination size is ‘15’ bytes, output size between 
‘6’ and ‘33’


Martin

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Bernd Schmidt]

On 07/01/2016 08:15 PM, Martin Sebor wrote:

The attached patch enhances compile-time checking for buffer overflow
and output truncation in non-trivial calls to the sprintf family of
functions under a new option -Wformat-length=[12].  This initial
patch handles printf directives with string, integer, and simple
floating arguments but eventually I'd like to extend it all other
functions and directives for which it makes sense.


On the whole I think this looks good.


+  /* Check the whole format strings and ist arguments for possible
+buffer overflow or truncation.  */


"its"


 struct function_format_info
 {
+  built_in_function fncode;
   int format_type; /* type of format (printf, scanf, etc.) 
*/
   unsigned HOST_WIDE_INT format_num;   /* number of format argument */
   unsigned HOST_WIDE_INT first_arg_num;/* number of first arg (zero 
for varargs) */
+  /* The destination object size or -1 if unknown.  */
+  unsigned HOST_WIDE_INT objsize;
+
+  /* True for functions whose output is bounded by a size argument
+ (e.g., snprintf and vsnprintf).  */
+  bool bounded;
 };


While you're here could you fix the existing comments to be formatted 
like the ones you're adding (in front of the thing they're documenting, 
etc.)?



+static void
+check_format_length (location_t,
+format_check_results *,
+const function_format_info *,
+const format_char_info *,
+const char *, size_t, size_t,
+const conversion_spec *, tree);


Only break the line before the function name for definitions, not 
declarations.



+unsigned char c = chr & 0xff;
+return flags [c / (CHAR_BIT * sizeof *flags)]
+  & (1 << (c % (CHAR_BIT * sizeof *flags)));


> +unsigned char c = chr & 0xff;
> +flags [c / (CHAR_BIT * sizeof *flags)]
> +  |= (1 << (c % (CHAR_BIT * sizeof *flags)));

Multi-line expressions should be wrapped in parentheses for correct 
formatting. Also, no space before [] brackets.



+/* Return the logarithm of tree node X in BASE, incremented by 1 when
+   the optional PLUS sign is True, plus the length of the octal ('0')
+   or hexadecimal ('0x') prefix when PREFIX is True.  Return -1 when
+   X cannot be represented.  */


Maybe this could be clearer as
/* Return the number of characters required to print X, which must be
   an INTEGER_CST, in BASE.  PLUS indicates whether a plus sign should
   be prepended to positive numbers, and PREFIX indicates whether an
   octal ('O') or hexadecimal ('0x') prefix should be prepended to
   nonzero numbesr.  Return -1 if X cannot be represented.  */


+  if (prefix && absval)


This surprised me, but I checked and printf really does not seem to 
print 0x0.



+/* Return a range representing the minimum and maximum number of bytes
+   that the conversion specification SPEC will write on output for the
+   integer argument ARG.  */


Should indicate that ARG can be null, and what happens in that case. 
It's not entirely clear to me actually why it would be NULL sometimes.



+width = (TREE_CODE (spec->star_width) == INTEGER_CST)
+? tree_to_shwi (spec->star_width) : 0;



+prec = (TREE_CODE (spec->star_precision) == INTEGER_CST)
+   ? tree_to_shwi (spec->star_precision) : 0;


Wrapping. Lose parens around the condition, and wrap the whole expression.


+  /* A type of the argument to the directive, either deduced from
+ the actual non-constant argument if one is known, or from
+ the directive itself when none has been provided because it's
+ a va_list.  */
+  tree argtype = NULL_TREE;


"The type" maybe? Also, the structure of this function gets confusing 
around this point. It looks like we set argtype for everything except 
integer constants, then have a big block handling nonnull argtype and 
returning, then we have another block dealing with the other case. I 
think it would be clearer to check for INTEGER_CST first, deal with it, 
and then doing the type-based estimation.



+  else if (TREE_CODE (arg) == INTEGER_CST)
+res.bounded = true;


The assignment appears to be repeated at the end of the function.


+width = (TREE_CODE (spec->star_width) == INTEGER_CST)
+? tree_to_shwi (spec->star_width) : 0;



+prec = (TREE_CODE (spec->star_precision) == INTEGER_CST)
+? tree_to_shwi (spec->star_precision) : 0;


See other instances. These lines occur several times in multiple functions.


+  int expdigs = -1;/* Number of exponent digits or -1 when unknown.  */
+  int negative = -1;   /* 1 when arg < 0, 0 when arg >= 0, -1 when unknown.  */


I think we should avoid end-of-line comments. I tend to sometimes feel 
they are ok-ish in struct definitions, but let's not have them in code.



+  res.min = (0 < negative || spec->get_flag ('+') || spec->get_flag (' '))
+   + 1 /* unit */ + (prec < 0 ? 7 : prec ? prec + 1 : 0)
+   + 2 /* e+ */ + 

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Florian Weimer]

On 07/12/2016 11:54 AM, Jakub Jelinek wrote:

On Tue, Jul 12, 2016 at 11:51:50AM +0200, Florian Weimer wrote:

On 07/01/2016 08:15 PM, Martin Sebor wrote:

The attached patch enhances compile-time checking for buffer overflow
and output truncation in non-trivial calls to the sprintf family of
functions under a new option -Wformat-length=[12].  This initial
patch handles printf directives with string, integer, and simple
floating arguments but eventually I'd like to extend it all other
functions and directives for which it makes sense.


I tried your patch with the following code, which is close to a real-world
example:

#include 

void print (const char *);

void
format_1 (unsigned address)
{
  unsigned char a = address >> 24;
  unsigned char b = address >> 16;
  unsigned char c = address >> 8;
  unsigned char d = address;
  char buf[15];
  sprintf ("%u.%u.%u.%u", buf, a, b, c, d);


Are you sure this is real-world code?  sprintf's first argument is the
buffer and second the format string, so if this doesn't warn at compile
time, it will surely crash at runtime when trying to store into .rodata.


Argh!  You are right, I swapped the arguments.

And further attempts showed that I was missing -D_FORTIFY_SOURCE=2. 
With it, I get a nice diagnostic.  Wow!


Florian

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Jakub Jelinek]

On Tue, Jul 12, 2016 at 11:51:50AM +0200, Florian Weimer wrote:
> On 07/01/2016 08:15 PM, Martin Sebor wrote:
> >The attached patch enhances compile-time checking for buffer overflow
> >and output truncation in non-trivial calls to the sprintf family of
> >functions under a new option -Wformat-length=[12].  This initial
> >patch handles printf directives with string, integer, and simple
> >floating arguments but eventually I'd like to extend it all other
> >functions and directives for which it makes sense.
> 
> I tried your patch with the following code, which is close to a real-world
> example:
> 
> #include 
> 
> void print (const char *);
> 
> void
> format_1 (unsigned address)
> {
>   unsigned char a = address >> 24;
>   unsigned char b = address >> 16;
>   unsigned char c = address >> 8;
>   unsigned char d = address;
>   char buf[15];
>   sprintf ("%u.%u.%u.%u", buf, a, b, c, d);

Are you sure this is real-world code?  sprintf's first argument is the
buffer and second the format string, so if this doesn't warn at compile
time, it will surely crash at runtime when trying to store into .rodata.

Jakub

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Florian Weimer]

On 07/01/2016 08:15 PM, Martin Sebor wrote:

The attached patch enhances compile-time checking for buffer overflow
and output truncation in non-trivial calls to the sprintf family of
functions under a new option -Wformat-length=[12].  This initial
patch handles printf directives with string, integer, and simple
floating arguments but eventually I'd like to extend it all other
functions and directives for which it makes sense.


I tried your patch with the following code, which is close to a 
real-world example:


#include 

void print (const char *);

void
format_1 (unsigned address)
{
  unsigned char a = address >> 24;
  unsigned char b = address >> 16;
  unsigned char c = address >> 8;
  unsigned char d = address;
  char buf[15];
  sprintf ("%u.%u.%u.%u", buf, a, b, c, d);
  print (buf);
}

void
format_2 (unsigned address)
{
  char buf[15];
  sprintf ("%u.%u.%u.%u", buf,
   address >> 24,
   (address >> 16) & 0xff,
   (address >> 8) & 0xff,
   address & 0xff);
  print (buf);
}

I didn't get a warning (with -O2 and -Wformat-length=1 or 
-Wformat-length=2).  If the warning is implemented in builtin folding, I 
guess this has to be expected because there is no range information, and 
warning for all %us similar to those in the example would produce too 
many false positives.


Florian

Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Florian Weimer]

On 07/01/2016 08:15 PM, Martin Sebor wrote:

diff --git a/gcc/passes.c b/gcc/passes.c
index 0565cfa..008799c 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -2425,8 +2425,15 @@ execute_pass_list_1 (opt_pass *pass)

   if (cfun == NULL)
return;
+
+  // inform (0, "executing pass: %s", pass->name);
+
   if (execute_one_pass (pass) && pass->sub)
-execute_pass_list_1 (pass->sub);
+   {
+ // inform (0, "executing subpass: %s", pass->sub->name);
+ execute_pass_list_1 (pass->sub);
+   }
+
   pass = pass->next;
 }
   while (pass);


This seems to be some leftover from debugging.

Florian