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.johan...@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.johan...@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
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
