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

[John Johansen]

On 03/23/2015 05:38 PM, Tyler Hicks wrote:
> 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?
> 
yeah that works and keeps us from having to have the extra if for every asprintf