[PATCH] RFC: add "deallocated_by" attribute for use by analyzer

[David Malcolm]

On Mon, 2020-11-16 at 17:49 -0700, Martin Sebor wrote:
> On 11/13/20 2:44 PM, Jeff Law via Gcc-patches wrote:
> > On 10/5/20 5:12 PM, David Malcolm via Gcc-patches wrote:
> > > This work-in-progress patch generalizes the malloc/free problem-
> > > checking
> > > in -fanalyzer so that it can work on arbitrary acquire/release
> > > API pairs.
> > > 
> > > It adds a new __attribute__((deallocated_by(FOO))) that could be
> > > used
> > > like this in a library header:
> > > 
> > >    struct foo;
> > > 
> > >    extern void foo_release (struct foo *);
> > > 
> > >    extern struct foo *foo_acquire (void)
> > >      __attribute__ ((deallocated_by(foo_release)));
> > > 
> > > In theory, the analyzer then "knows" these functions are an
> > > acquire/release pair, and can emit diagnostics for leaks, double-
> > > frees,
> > > use-after-frees, mismatching deallocations, etc.
> > > 
> > > My hope was that this would provide a minimal level of markup
> > > that would
> > > support library-checking without requiring lots of further
> > > markup.
> > > I attempted to use this to detect a memory leak within a Linux
> > > driver (CVE-2019-19078), by adding the attribute to mark these
> > > fns:
> > >    extern struct urb *usb_alloc_urb(int iso_packets, gfp_t
> > > mem_flags);
> > >    extern void usb_free_urb(struct urb *urb);
> > > where there is a leak of a "urb" on an error-handling path.
> > > Unfortunately I ran into the problem that there are various other
> > > fns
> > > that take "struct urb *" and the analyzer conservatively assumes
> > > that a
> > > urb passed to them might or might not be freed and thus stops
> > > tracking
> > > state for them.
> > > 
> > > So I don't know how much use this feature would be as-is.
> > > (without either requiring lots of additional attributes for
> > > marking
> > > fndecl args as being merely borrowed, or simply assuming that
> > > they
> > > are borrowed in the absence of a function body to analyze)
> > > 
> > > Thoughts?
> > > Dave
> > > 
> > > gcc/analyzer/ChangeLog:
> > > 	* region-model-impl-calls.cc
> > > 	(region_model::impl_deallocation_call): New.
> > > 	* region-model.cc: Include "attribs.h".
> > > 	(region_model::on_call_post): Handle fndecls referenced by
> > > 	__attribute__((deallocated_by(FOO))).
> > > 	* region-model.h (region_model::impl_deallocation_call): New
> > > decl.
> > > 	* sm-malloc.cc: Include "stringpool.h" and "attribs.h".
> > > 	(enum wording): Add WORDING_DEALLOCATED.
> > > 	(malloc_state_machine::custom_api_map_t): New typedef.
> > > 	(malloc_state_machine::m_custom_apis): New field.
> > > 	(start_p): New.
> > > 	(use_after_free::describe_state_change): Handle
> > > 	WORDING_DEALLOCATED.
> > > 	(use_after_free::describe_final_event): Likewise.
> > > 	(malloc_leak::describe_state_change): Only emit "allocated
> > > here" on
> > > 	a start->nonnull transition, rather than on other transitions
> > > to
> > > 	nonnull.
> > > 	(malloc_state_machine::~malloc_state_machine): New.
> > > 	(malloc_state_machine::on_stmt): Handle
> > > 	"__attribute__((deallocated_by(FOO)))", and the special
> > > attribute
> > > 	set on FOO.
> > > 	(malloc_state_machine::get_or_create_api): New.
> > > 	(malloc_state_machine::on_allocator_call): Add
> > > "returns_nonnull"
> > > 	param and use it to affect which state to transition to.
> > > 
> > > gcc/c-family/ChangeLog:
> > > 	* c-attribs.c (c_common_attribute_table): Add entry for
> > > 	"deallocated_by".
> > > 	(matching_deallocator_type_p): New.
> > > 	(maybe_add_deallocator_attribute): New.
> > > 	(handle_deallocated_by_attribute): New.
> > > 
> > > gcc/ChangeLog:
> > > 	* doc/extend.texi (Common Function Attributes): Add
> > > 	"deallocated_by".
> > > 
> > > gcc/testsuite/ChangeLog:
> > > 	* gcc.dg/analyzer/attr-deallocated_by-1.c: New test.
> > > 	* gcc.dg/analyzer/attr-deallocated_by-1a.c: New test.
> > > 	* gcc.dg/analyzer/attr-deallocated_by-2.c: New test.
> > > 	* gcc.dg/analyzer/attr-deallocated_by-3.c: New test.
> > > 	* gcc.dg/analyzer/attr-deallocated_by-4.c: New test.
> > > 	* gcc.dg/analyzer/attr-deallocated_by-CVE-2019-19078-usb-
> > > leak.c:
> > > 	New test.
> > > 	* gcc.dg/analyzer/attr-deallocated_by-misuses.c: New test.
> > 
> > I'd probably go with something more like acquire/release since I
> > think
> > the same concepts apply to things like file descriptors acquired by
> > open
> > and released by close.  I think the basic concept makes sense and
> > would
> > be useful, so I'd lean towards moving forward even if it hasn't
> > been
> > particularly useful for the analyzer yet.  One could even ponder
> > propagation of the attribute similar to what we do with const/pure
> > so
> > that we could see through wrappers without the user having to do
> > more
> > markup.
> > 
> > 
> > What I wonder here is whether or not Martin's work could take
> > advantage
> > of the attribute.   I don't see that as strictly necessary for the
> > patch
> > to move forward, just a question we should try to answer.
> 
> It could, but it would need at least one change.  The patch I posted
> on Friday introduces a similar attribute:
> https://gcc.gnu.org/pipermail/gcc-patches/2020-November/559053.html
> 
> The main differences between the two are that deallocated_by works
> on integers in addition to pointers, but doesn't support positional
> arguments for the deallocator.  The work I submitted has no support
> for integers (meaning I couldn't support the attribute for those
> APIs even if I had an attribute for that) but for completeness needs
> the positional argument (it's also what the kernel developers asked
> for).
> 
> I propose we introduce two attributes with different names: one
> for pointers and another for integers (and perhaps other kinds of
> handles in the future).  The analyzer would handle both, the rest
> of GCC just the pointer variety (at least for now).
> 
> I don't think introducing two different attributes for the same
> thing (i.e., for pointer APIs), one for the analyzer and another
> for GCC warnings, would be helpful to users.

I spent some time today porting my patch to sit on top of Martin's,
eliminating the "deallocated_by" attribute in favor of the "malloc"
attribute.

It works (though I haven't yet implemented the optional param index,
but I think that won't be hard).

I like the idea of introducing a separate attribute for marking non-
pointer acquire/release pairs; I can rework my patch to do that.

Martin's work supports multiple deallocators, which mine doesn't (but
probably can with a little reworking).

I'm not in love with "__attribute__((malloc (FOO)))", in that I thought
"deallocated_by" was clearer - but it's not a dealbreaker for me.  It's
kind of hidden in the testcases by the way Martin used macros for the
attribute.

One of my testcases detected mismatching types, which the malloc
attribute handler doesn't:

/* Mismatching types.  */
struct foo {};
struct bar {};
extern void takes_foo (struct foo *); /* { dg-message "\\.\\.\\.whereas 'takes_foo' accepts type 'struct foo \\*'" } */
struct bar *wrong_deallocator_type (void)
  __attribute__ ((malloc(takes_foo))); /* { dg-error "'malloc' attribute applied to function returning type 'struct bar \\*'\\.\\.\\." } */

Are we allowing users to be flexible about this, or should we implement
type checking in the malloc attribute handler?
(see matching_deallocator_type_p in my patch for how I did it for
deallocated_by).

So hopefully that gives us a way forward.  I'm about to disappear for a
week and a half, so don't let my analyzer patches stand in the way of
Martin's.  I can finish reworking my stuff on top of Martin's when I
get back, or if they aren't in "master" yet I can combine parts of
Martin's patches and integrate them into mine.

Hope this sounds sane
Dave

> Martin
> 
> > 
> > So I don't mind seeing it go forward.  I leave it as your call.
> > 
> > 
> > jeff
> > 
> >