[apparmor] [PATCH v2 06/42] Use the gcc cleanup extension attribute to handle freeing temp allocations
Tyler Hicks
tyhicks at canonical.com
Mon Mar 23 23:24:43 UTC 2015
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.
> @@ -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
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
-------------- 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/0eb63355/attachment.pgp>
More information about the AppArmor
mailing list