On 2015-03-23 18:24:43, Tyler Hicks wrote: > On 2015-03-06 15:48:22, Tyler Hicks wrote: > > From: John Johansen <[email protected]> > > > > 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 <[email protected]> > > --- > > 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
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
