Bug 49905 - Better sanity checking on sprintf src & dest to produce warning for dodgy code ?
Summary: Better sanity checking on sprintf src & dest to produce warning for dodgy code ?
Status: UNCONFIRMED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.7.0
: P3 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-29 19:53 UTC by David Binderman
Modified: 2011-08-04 07:40 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David Binderman 2011-07-29 19:53:42 UTC
I just tried to compile the following C++ code with the latest trunk
snapshot 20110723 on an AMD x86_64 box.

# include <stdio.h>

void f()
{
    char * p = new char [4];
    char q[4];

    sprintf(p, "ian");  // Legal
    sprintf(q, "ian");

    sprintf(p, "bert"); // One over
    sprintf(q, "bert");

    sprintf(p, "harry");    // definately wrong.
    sprintf(q, "harry");

    sprintf(p, "%s", "harry");  // more subtle.
    sprintf(q, "%s", "harry");

    sprintf(p, "%s %s", "ab", "cd");    // more subtle still.
    sprintf(q, "%s %s", "ab", "cd");

    sprintf(p, "%s %d", "ab", 1000);    // overpoints.
    sprintf(q, "%s %d", "ab", 1000);
}

Much to my surprise, the compiler, even with lots of flags, said not much

bash-4.2$ ~/gcc/20110723/results/bin/g++ -c -O2 -Wall -Wextra -pedantic bug29.cc
bash-4.2$

I'd have expected a few warnings, at very least.

Surely something in the compiler could be done to check that sprintf is
called, the destination buffer size is known, the minimum source buffer
size is known, and compare the two to make sure the source fits inside
the destination ?

Such a warning will in my experience find plenty of bugs in application code.
Comment 1 Jakub Jelinek 2011-07-29 21:10:06 UTC
Just use also -D_FORTIFY_SOURCE=1 -O2 or -D_FORTIFY_SOURCE=2 -O2.
For the first three overflows on q you'll get compile time warnings, and for all overflows on q you'll get the program killed at runtime.
If you use char * p = (char *) malloc (4);
instead of char * p = new char [4];
you'll get protection for the p overflows too, I'll see if __builtin_object_size could be taught about C++ new, at least some forms thereof.
Comment 2 Jakub Jelinek 2011-08-04 07:40:29 UTC
Author: jakub
Date: Thu Aug  4 07:40:24 2011
New Revision: 177316

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=177316
Log:
	PR middle-end/49905
	* tree.h (init_attributes): New prototype.
	* attribs.c (init_attributes): No longer static.

	* decl.c (cxx_init_decl_processing): Add alloc_size (1) attribute
	for operator new and operator new [].  Call init_attributes.

	* g++.dg/ext/builtin-object-size3.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/ext/builtin-object-size3.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/attribs.c
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/decl.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree.h