[apparmor] [PATCH v2 06/42] Use the gcc cleanup extension attribute to handle freeing temp allocations

[Tyler Hicks]

On 2015-03-23 18:24:43, Tyler Hicks wrote:
> On 2015-03-06 15:48:22, Tyler Hicks wrote:
> > From: John Johansen <john.johansen at canonical.com>
> > 
> > While some of these allocations will go away as we convert to C++,
> > some of these need to stay C as the are going to be moved into a
> > library to support loading cache from init daemons etc.
> > 
> > For the bits that will eventually be C++ this helps clean things up,
> > in the interim.
> > 
> > TODO: apply to libapparmor as well
> > 
> > Signed-off-by: John Johansen <john.johansen at canonical.com>
> > ---
> >  parser/lib.c              | 11 ++++++++---
> >  parser/lib.h              |  3 +++
> >  parser/parser_interface.c | 13 ++++---------
> >  parser/parser_lex.l       | 16 ++++------------
> >  parser/parser_main.c      | 39 ++++++++++++---------------------------
> >  5 files changed, 31 insertions(+), 51 deletions(-)
> > 
> > diff --git a/parser/lib.c b/parser/lib.c
> > index 85e39e0..fb9df49 100644
> > --- a/parser/lib.c
> > +++ b/parser/lib.c
> > @@ -33,6 +33,13 @@
> >  #include "lib.h"
> >  #include "parser.h"
> >  
> > +/* automaticly free allocated variables tagged with autofree on fn exit */
> > +void __autofree(void *p)
> > +{
> > +	void **_p = (void**)p;
> > +	free(*_p);
> > +}
> > +
> >  /**
> >   * dirat_for_each: iterate over a directory calling cb for each entry
> >   * @dir: already opened directory (MAY BE NULL)
> > @@ -62,7 +69,7 @@
> >  int dirat_for_each(DIR *dir, const char *name, void *data,
> >  		   int (* cb)(DIR *, const char *, struct stat *, void *))
> >  {
> > -	struct dirent *dirent = NULL;
> > +	autofree struct dirent *dirent = NULL;
> >  	DIR *d = NULL;
> >  	int error;
> >  
> > @@ -132,7 +139,6 @@ int dirat_for_each(DIR *dir, const char *name, void *data,
> >  
> >  	if (d != dir)
> >  		closedir(d);
> > -	free(dirent);
> >  
> >  	return 0;
> >  
> > @@ -140,7 +146,6 @@ fail:
> >  	error = errno;
> >  	if (d && d != dir)
> >  		closedir(d);
> > -	free(dirent);
> >  	errno = error;
> >  
> >  	return -1;
> > diff --git a/parser/lib.h b/parser/lib.h
> > index aac2223..73e1ffc 100644
> > --- a/parser/lib.h
> > +++ b/parser/lib.h
> > @@ -3,6 +3,9 @@
> >  
> >  #include <dirent.h>
> >  
> > +#define autofree __attribute((cleanup(__autofree)))
> > +void __autofree(void *p);
> > +
> >  int dirat_for_each(DIR *dir, const char *name, void *data,
> >  		   int (* cb)(DIR *, const char *, struct stat *, void *));
> >  
> > diff --git a/parser/parser_interface.c b/parser/parser_interface.c
> > index 0d6626d..485c30b 100644
> > --- a/parser/parser_interface.c
> > +++ b/parser/parser_interface.c
> > @@ -28,6 +28,7 @@
> >  #include <string>
> >  #include <sstream>
> >  
> > +#include "lib.h"
> >  #include "parser.h"
> >  #include "profile.h"
> >  #include "libapparmor_re/apparmor_re.h"
> > @@ -374,13 +375,11 @@ void sd_serialize_profile(std::ostringstream &buf, Profile *profile,
> >  	sd_write_struct(buf, "profile");
> >  	if (flattened) {
> >  		assert(profile->parent);
> > -		char *name = (char *) malloc(3 + strlen(profile->name) +
> > -				    strlen(profile->parent->name));
> > +		autofree char *name = (char *) malloc(3 + strlen(profile->name) + strlen(profile->parent->name));
> >  		if (!name)
> >  			return;
> >  		sprintf(name, "%s//%s", profile->parent->name, profile->name);
> >  		sd_write_string(buf, name, NULL);
> > -		free(name);
> >  	} else {
> >  		sd_write_string(buf, profile->name, NULL);
> >  	}
> > @@ -483,7 +482,7 @@ int __sd_serialize_profile(int option, Profile *prof)
> >  	int fd = -1;
> >  	int error = -ENOMEM, size, wsize;
> >  	std::ostringstream work_area;
> > -	char *filename = NULL;
> > +	autofree char *filename = NULL;
> >  
> >  	switch (option) {
> >  	case OPTION_ADD:
> 
> Adding some missing context:
> 
>         switch (option) {
>         case OPTION_ADD:
>                 if (asprintf(&filename, "%s/.load", subdomainbase) == -1)
>                         goto exit;
>                 if (kernel_load) fd = open(filename, O_WRONLY);
>                 break;
>         case OPTION_REPLACE:
>                 if (asprintf(&filename, "%s/.replace", subdomainbase) == -1)
>                         goto exit;
>                 if (kernel_load) fd = open(filename, O_WRONLY);
>                 break;
>         case OPTION_REMOVE:
>                 if (asprintf(&filename, "%s/.remove", subdomainbase) == -1)
>                         goto exit;
>                 if (kernel_load) fd = open(filename, O_WRONLY);
>                 break;
>         case OPTION_STDOUT:
>                 filename = strdup("stdout");
>                 fd = dup(1);
>                 break;
>         case OPTION_OFILE:
>                 fd = dup(fileno(ofile));
>                 break;
>         default:
>                 error = -EINVAL;
>                 goto exit;
>                 break;
>         }
> 
> If those asprintf() calls fail, the value of filename is undefined so we can't
> be sure what the __autofree() function will receive as input. I'll follow up
> with a patch to set filename to NULL in the asprintf() error paths.

I started to do this and it causes a bunch of churn in this series.
Additionally, it doesn't prevent the same mistake from happening again.
I'm now leaning towards doing something like this so that the entire
codebase benefits:

aa_asprintf(char **strp, const char *fmt, ...)
{
	va_list args;

	va_start(args, fmt);
	if(vasprintf(strp, fmt, args) == -1)
		*strp = NULL;
	va_end(args); 
}

#define asprintf aa_asprintf

Any thoughts?

Tyler
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20150323/f71f2d6a/attachment.pgp>