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.
> @@ -523,8 +522,6 @@ int __sd_serialize_profile(int option, Profile *prof)
>
> error = 0;
>
> - free(filename);
> -
> if (option == OPTION_REMOVE) {
> char *name, *ns = NULL;
> int len = 0;
> @@ -646,7 +643,7 @@ int sd_load_buffer(int option, char *buffer, int size)
> {
> int fd = -1;
> int error, bsize;
> - char *filename = NULL;
> + autofree char *filename = NULL;
>
> /* TODO: push backup into caller */
> if (!kernel_load)
Same for these asprintf() calls:
switch (option) {
case OPTION_ADD:
if (asprintf(&filename, "%s/.load", subdomainbase) == -1)
return -ENOMEM;
break;
case OPTION_REPLACE:
if (asprintf(&filename, "%s/.replace", subdomainbase) == -1)
return -ENOMEM;
break;
default:
return -EINVAL;
}
> @@ -694,7 +691,5 @@ int sd_load_buffer(int option, char *buffer, int size)
> close(fd);
>
> out:
> - free(filename);
> -
> return error;
> }
> diff --git a/parser/parser_lex.l b/parser/parser_lex.l
> index 2f6f914..0456843 100644
> --- a/parser/parser_lex.l
> +++ b/parser/parser_lex.l
> @@ -125,15 +125,13 @@ static int include_dir_cb(DIR *dir unused, const char
> *name, struct stat *st,
> {
> struct cb_struct *d = (struct cb_struct *) data;
>
> - char *path;
> + autofree char *path = NULL;
>
> if (asprintf(&path, "%s/%s", d->fullpath, name) < 0)
> yyerror("Out of memory");
And this one.
>
> - if (is_blacklisted(name, path)) {
> - free(path);
> + if (is_blacklisted(name, path))
> return 0;
> - }
>
> if (S_ISREG(st->st_mode)) {
> if (!(yyin = fopen(path,"r")))
> @@ -144,8 +142,6 @@ static int include_dir_cb(DIR *dir unused, const char
> *name, struct stat *st,
> yypush_buffer_state(yy_create_buffer(yyin, YY_BUF_SIZE));
> }
>
> - free(path);
> -
> return 0;
> }
>
> @@ -153,7 +149,7 @@ void include_filename(char *filename, int search)
> {
> FILE *include_file = NULL;
> struct stat my_stat;
> - char *fullpath = NULL;
> + autofree char *fullpath = NULL;
>
> if (search) {
> if (preprocess_only)
> @@ -188,9 +184,6 @@ void include_filename(char *filename, int search)
> " '%s' in '%s'"), fullpath, filename);;
> }
> }
> -
> - if (fullpath)
> - free(fullpath);
> }
>
> %}
> @@ -281,9 +274,8 @@ LT_EQUAL <=
>
> <INCLUDE>{
> (\<([^\> \t\n]+)\>|\"([^\" \t\n]+)\") { /* <filename> */
> - char *filename = strndup(yytext, yyleng - 1);
> + autofree char *filename = strndup(yytext, yyleng - 1);
> include_filename(filename + 1, *filename == '<');
> - free(filename);
> POP_NODUMP();
> }
>
> diff --git a/parser/parser_main.c b/parser/parser_main.c
> index f075618..f18d5f4 100644
> --- a/parser/parser_main.c
> +++ b/parser/parser_main.c
> @@ -555,16 +555,13 @@ static void set_features_by_match_file(void)
> {
> FILE *ms = fopen(MATCH_FILE, "r");
> if (ms) {
> - char *match_string = (char *) malloc(1000);
> + autofree char *match_string = (char *) malloc(1000);
> if (!match_string)
> goto no_match;
> - if (!fgets(match_string, 1000, ms)) {
> - free(match_string);
> + if (!fgets(match_string, 1000, ms))
> goto no_match;
> - }
> if (strstr(match_string, " perms=c"))
> perms_create = 1;
> - free(match_string);
> kernel_supports_network = 1;
> goto out;
> }
> @@ -618,7 +615,7 @@ static void set_supported_features(void) {
>
> int process_binary(int option, const char *profilename)
> {
> - char *buffer = NULL;
> + autofree char *buffer = NULL;
> int retval = 0, size = 0, asize = 0, rsize;
> int chunksize = 1 << 14;
> int fd;
> @@ -637,7 +634,7 @@ int process_binary(int option, const char *profilename)
>
> do {
> if (asize - size == 0) {
> - buffer = (char *) realloc(buffer, chunksize);
> + buffer = (char *) realloc(buffer, chunksize);
> asize = chunksize;
> chunksize <<= 1;
> if (!buffer) {
> @@ -658,8 +655,6 @@ int process_binary(int option, const char *profilename)
> else
> retval = rsize;
>
> - free(buffer);
> -
> if (conf_verbose) {
> switch (option) {
> case OPTION_ADD:
> @@ -694,7 +689,7 @@ int test_for_dir_mode(const char *basename, const char
> *linkdir)
> int rc = 0;
>
> if (!skip_mode_force) {
> - char *target = NULL;
> + autofree char *target = NULL;
> if (asprintf(&target, "%s/%s/%s", basedir, linkdir, basename) <
> 0) {
> perror("asprintf");
> exit(1);
Here too.
> @@ -702,8 +697,6 @@ int test_for_dir_mode(const char *basename, const char
> *linkdir)
>
> if (access(target, R_OK) == 0)
> rc = 1;
> -
> - free(target);
> }
>
> return rc;
> @@ -713,8 +706,8 @@ int process_profile(int option, const char *profilename)
> {
> struct stat stat_bin;
> int retval = 0;
> - char * cachename = NULL;
> - char * cachetemp = NULL;
> + autofree char *cachename = NULL;
> + autofree char *cachetemp = NULL;
> const char *basename = NULL;
>
> /* per-profile states */
Another here:
/* setup cachename and tstamp */
if (!force_complain && !skip_cache) {
if (asprintf(&cachename, "%s/%s", cacheloc,
basename)<0) {
PERROR(_("Memory allocation error."));
exit(1);
}
...
}
And here:
/* Otherwise, set up to save a cached copy */
if (asprintf(&cachetemp, "%s-XXXXXX", cachename)<0) {
perror("asprintf");
return ENOMEM;
}
> @@ -879,10 +872,8 @@ out:
> unlink(cachetemp);
> PERROR("Warning failed to create cache: %s\n",
> basename);
> }
> - free(cachetemp);
> }
> - if (cachename)
> - free(cachename);
> +
> return retval;
> }
>
> @@ -894,11 +885,10 @@ static int profile_dir_cb(DIR *dir unused, const char
> *name, struct stat *st,
>
> if (!S_ISDIR(st->st_mode) && !is_blacklisted(name, NULL)) {
> const char *dirname = (const char *)data;
> - char *path;
> + autofree char *path = NULL;
> if (asprintf(&path, "%s/%s", dirname, name) < 0)
> PERROR(_("Out of memory"));
Here.
> rc = process_profile(option, path);
> - free(path);
> }
> return rc;
> }
> @@ -911,19 +901,18 @@ static int binary_dir_cb(DIR *dir unused, const char
> *name, struct stat *st,
>
> if (!S_ISDIR(st->st_mode) && !is_blacklisted(name, NULL)) {
> const char *dirname = (const char *)data;
> - char *path;
> + autofree char *path = NULL;
> if (asprintf(&path, "%s/%s", dirname, name) < 0)
> PERROR(_("Out of memory"));
Here, too.
> rc = process_binary(option, path);
> - free(path);
> }
> return rc;
> }
>
> static void setup_flags(void)
> {
> - char *cache_features_path = NULL;
> - char *cache_flags = NULL;
> + autofree char *cache_features_path = NULL;
> + autofree char *cache_flags = NULL;
>
> /* Get the match string to determine type of regex support needed */
> set_supported_features();
One more:
if (asprintf(&cache_features_path, "%s/.features", cacheloc) == -1) {
PERROR(_("Memory allocation error."));
exit(1);
}
Tyler
> @@ -964,13 +953,9 @@ static void setup_flags(void)
> skip_read_cache = 1;
> }
> }
> - free(cache_flags);
> - cache_flags = NULL;
> } else if (write_cache) {
> create_cache(cacheloc, cache_features_path, features_string);
> }
> -
> - free(cache_features_path);
> }
>
> int main(int argc, char *argv[])
> --
> 2.1.4
>
>
> --
> AppArmor mailing list
> [email protected]
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/apparmor
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
