Bug 83859 - Please add new attribute which will establish relation between parameters for buffer and its size
Summary: Please add new attribute which will establish relation between parameters for...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 8.0
: P3 enhancement
Target Milestone: 10.0
Assignee: Martin Sebor
URL:
Keywords: diagnostic, patch
: 31893 (view as bug list)
Depends on:
Blocks: 31279
  Show dependency treegraph
 
Reported: 2018-01-15 15:20 UTC by Daniel Fruzynski
Modified: 2019-11-23 10:06 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2018-01-15 00:00:00


Attachments
Slies from Cauldron 2017 presentation. (82.81 KB, application/pdf)
2018-01-15 17:22 UTC, Martin Sebor
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Fruzynski 2018-01-15 15:20:31 UTC
gcc can detect if buffer size passed to function like strncpy is incorrect, e.g. it is sizeof pointer. It would be good to have similar diagnostics enabled for custom functions also accepts buffer and its size. Please add new function attribute which would allow to do this and appropriate diagnostics which will use it. I propose to add following attribute with two parameters - indices of buffer and its size arguments. Note that function may accept multiple such pairs, so it should be possible to use this attribute multiple times.

__attribute__((buffer_size(1, 2)))
void foo(char* dst, size_t dstsize);
Comment 1 Martin Sebor 2018-01-15 17:22:33 UTC
Created attachment 43140 [details]
Slies from Cauldron 2017 presentation.

Thanks for the suggestion.  Let me confirm this as a useful enhancement.

I have a patch with three such attributes that I was hoping to submit for GCC 8 but I didn't resolve all the design issues with it.  The current solution provides four attributes:

  read_only (ptr-arg, size-arg)
  write_only (ptr-arg, size-arg)
  read_write (ptr-arg, size-arg)
  no_side_effect

They let GCC know at the call site not just the size of the array the pointer points to but also how the function accesses the contents of the array.  The last one means that a function has no side effects except as annotated by the other attributes (which is a superset of what attributes const and pure do).  Together they not only enable all sorts of diagnostics but can also improve the emitted code.  Attached are a few slides from my presentation on this project at GNU Tools Cauldron 2017.

Some of the open design questions I have with this solution are:

1) How to annotate constant size buffers.  I'd like to be able to express that a function requires a buffer of at least N elements without making N an argument to the function.  E.g., annotate the declaration 'void f (int[2])' to let GCC understand that it requires an array of at least 2 ints.

2) Can partial writes for attribute write_only functions be handled?  I.e., would it be worthwhile to provide an annotation to let GCC know at the call site where to look for the number of elements written into an write_only or read_write buffer.

3) How to annotate strings (i.e., sequences of elements terminated by a sentinel element).  E.g., in my_strcpy(char *d, const char *s), what should the attribute syntax look like to tell GCC that d and s are nul-terminated strings.  As a separate question, is it worthwhile to also provide annotation to express a relationship between d and s (e.g., that they have to have the same number of elements).
Comment 2 Martin Sebor 2018-01-15 17:51:19 UTC
So confirmed.
Comment 3 Martin Sebor 2018-01-15 17:52:01 UTC
Since I have a patch I might as well assign this to myself.
Comment 4 joseph@codesourcery.com 2018-01-15 18:16:25 UTC
On Mon, 15 Jan 2018, msebor at gcc dot gnu.org wrote:

> 1) How to annotate constant size buffers.  I'd like to be able to express that
> a function requires a buffer of at least N elements without making N an
> argument to the function.  E.g., annotate the declaration 'void f (int[2])' to
> let GCC understand that it requires an array of at least 2 ints.

"void f (int[static 2])" means that; no GNU-specific syntax is needed.
Comment 5 Martin Sebor 2018-01-15 19:56:40 UTC
Yes,'void f (int[static 2])' does mean that and I had hoped to be able to rely on it.  Unfortunately, the VLA specification suffers from a number of limitations that made it impractical.  Some of them are discussed in WG14 N2074: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2074.htm.  Using an attribute (either explicitly specified by the programmer or implicitly inserted by GCC) would overcome some of these.  Another limitation, one that's not discussed in N2074 but that's relevant to GCC, is that the VLA syntax is available only in C and not in C++.  I'd like the feature to be usable in both languages.

But even in C it's common to use 'void f (int[2])' to mean essentially the same thing as 'void f (int[static 2])'.  It would be helpful if GCC could provide a mechanism to help detect bugs when using the former, and I'm hoping to use the attributes to do that.
Comment 6 Richard Biener 2018-01-16 09:00:48 UTC
Note you are partly exposing features of "fn spec".
Comment 7 Eric Gallager 2018-02-07 07:13:35 UTC
(In reply to joseph@codesourcery.com from comment #4)
> On Mon, 15 Jan 2018, msebor at gcc dot gnu.org wrote:
> 
> > 1) How to annotate constant size buffers.  I'd like to be able to express that
> > a function requires a buffer of at least N elements without making N an
> > argument to the function.  E.g., annotate the declaration 'void f (int[2])' to
> > let GCC understand that it requires an array of at least 2 ints.
> 
> "void f (int[static 2])" means that; no GNU-specific syntax is needed.

For that to be useful, both bug 50584 and bug 67793 would have to be fixed
Comment 8 Martin Sebor 2019-09-29 19:53:40 UTC
Patch: https://gcc.gnu.org/ml/gcc-patches/2019-09/msg01690.html
Comment 9 Martin Sebor 2019-09-29 22:03:09 UTC
*** Bug 31893 has been marked as a duplicate of this bug. ***
Comment 10 Martin Sebor 2019-11-22 17:14:48 UTC
Author: msebor
Date: Fri Nov 22 17:14:17 2019
New Revision: 278624

URL: https://gcc.gnu.org/viewcvs?rev=278624&root=gcc&view=rev
Log:
PR middle-end/83859 - attributes to associate pointer arguments and sizes

gcc/ChangeLog:

	PR middle-end/83859
	* attribs.h (struct attr_access): New.
	* attribs.c (decl_attributes): Add an informational note.
	* builtins.c (check_access): Make extern.  Consistently set no-warning
	after issuing a warning.  Handle calls through function pointers.  Set
	no-warning.
	* builtins.h (check_access): Declare.
	* calls.c (rdwr_access_hash): New type.
	(rdwr_map): Same.
	(init_attr_rdwr_indices): New function.
	(maybe_warn_rdwr_sizes): Same.
	(initialize_argument_information): Call init_attr_rdwr_indices.
	Call maybe_warn_rdwr_sizes.
	(get_size_range): Avoid null argument.
	* doc/extend.texi (attribute access): Document new attribute.

gcc/c-family/ChangeLog:

	PR middle-end/83859
	* c-attribs.c (handle_access_attribute): New function.
	(c_common_attribute_table): Add new attribute.
	(get_argument_type): New function.
	(append_access_attrs): New function.
	(get_nonnull_operand): Rename...
	(get_attribute_operand): ...to this.
	* c-common.c (get_nonnull_operand): Rename...
	(get_attribute_operand): ...to this.

gcc/testsuite/ChangeLog:

	PR middle-end/83859
	* c-c++-common/attr-nonstring-8.c: Adjust text of expected warning.
	* gcc.dg/Wstringop-overflow-23.c: New test.
	* gcc.dg/Wstringop-overflow-24.c: New test.
	* gcc.dg/attr-access-read-only.c: New test.
	* gcc.dg/attr-access-read-write.c: New test.
	* gcc.dg/attr-access-read-write-2.c: New test.
	* gcc.dg/attr-access-write-only.c: New test.


Added:
    trunk/gcc/testsuite/gcc.dg/Wstringop-overflow-23.c
    trunk/gcc/testsuite/gcc.dg/Wstringop-overflow-24.c
    trunk/gcc/testsuite/gcc.dg/attr-access-read-only.c
    trunk/gcc/testsuite/gcc.dg/attr-access-read-write-2.c
    trunk/gcc/testsuite/gcc.dg/attr-access-read-write.c
    trunk/gcc/testsuite/gcc.dg/attr-access-write-only.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/attribs.c
    trunk/gcc/attribs.h
    trunk/gcc/builtins.c
    trunk/gcc/builtins.h
    trunk/gcc/c-family/ChangeLog
    trunk/gcc/c-family/c-attribs.c
    trunk/gcc/c-family/c-common.c
    trunk/gcc/c-family/c-common.h
    trunk/gcc/calls.c
    trunk/gcc/doc/extend.texi
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/c-c++-common/attr-nonstring-8.c
Comment 11 Martin Sebor 2019-11-22 17:16:54 UTC
The part of the patch that adds the attribute has been added in r278624.  The rest of it will be committed separately under other bugs (e.g., those referenced in See Also).
Comment 12 Jakub Jelinek 2019-11-23 10:06:57 UTC
Author: jakub
Date: Sat Nov 23 10:06:26 2019
New Revision: 278641

URL: https://gcc.gnu.org/viewcvs?rev=278641&root=gcc&view=rev
Log:
	PR middle-end/83859
	* doc/extend.texi (attribute access): Fix a typo.

	* c-attribs.c (append_access_attrs): Avoid buffer overflow.  Avoid
	memory leak.  Use XNEWVEC macro.  Use auto_diagnostic_group to
	group warning with inform together.
	(handle_access_attribute): Formatting fix.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/c-family/ChangeLog
    trunk/gcc/c-family/c-attribs.c
    trunk/gcc/doc/extend.texi