commit 71eaebd971f4d42b26fb6b85780163bbc0111aae Author: Nick Mathewson nickm@torproject.org Date: Fri Apr 11 03:04:16 2014 -0400
Drop 'fr' parameter from sandbox code.
Appearently, the majority of the filenames we pass to sandbox_cfg_allow() functions are "freeable right after". So, consider _all_ of them safe-to-steal, and add a tor_strdup() in the few cases that aren't.
(Maybe buggy; revise when I can test.) --- src/common/sandbox.c | 55 ++++++++++--------------- src/common/sandbox.h | 53 +++++++++---------------- src/or/main.c | 108 +++++++++++++++++++++++++------------------------- 3 files changed, 92 insertions(+), 124 deletions(-)
diff --git a/src/common/sandbox.c b/src/common/sandbox.c index c2f482d..b97b900 100644 --- a/src/common/sandbox.c +++ b/src/common/sandbox.c @@ -1056,11 +1056,11 @@ new_element(int syscall, int index, intptr_t value) #endif
int -sandbox_cfg_allow_stat_filename(sandbox_cfg_t **cfg, char *file, int fr) +sandbox_cfg_allow_stat_filename(sandbox_cfg_t **cfg, char *file) { sandbox_cfg_t *elem = NULL;
- elem = new_element(SCMP_stat, 0, (intptr_t)(void*) tor_strdup(file)); + elem = new_element(SCMP_stat, 0, (intptr_t)(void*) file); if (!elem) { log_err(LD_BUG,"(Sandbox) failed to register parameter!"); return -1; @@ -1069,7 +1069,6 @@ sandbox_cfg_allow_stat_filename(sandbox_cfg_t **cfg, char *file, int fr) elem->next = *cfg; *cfg = elem;
- if (fr) tor_free(file); return 0; }
@@ -1083,9 +1082,7 @@ sandbox_cfg_allow_stat_filename_array(sandbox_cfg_t **cfg, ...) va_start(ap, cfg);
while ((fn = va_arg(ap, char*)) != NULL) { - int fr = va_arg(ap, int); - - rc = sandbox_cfg_allow_stat_filename(cfg, fn, fr); + rc = sandbox_cfg_allow_stat_filename(cfg, fn); if (rc) { log_err(LD_BUG,"(Sandbox) sandbox_cfg_allow_stat_filename_array fail"); goto end; @@ -1098,11 +1095,11 @@ sandbox_cfg_allow_stat_filename_array(sandbox_cfg_t **cfg, ...) }
int -sandbox_cfg_allow_open_filename(sandbox_cfg_t **cfg, char *file, int fr) +sandbox_cfg_allow_open_filename(sandbox_cfg_t **cfg, char *file) { sandbox_cfg_t *elem = NULL;
- elem = new_element(SCMP_SYS(open), 0, (intptr_t)(void *)tor_strdup(file)); + elem = new_element(SCMP_SYS(open), 0, (intptr_t)(void *) file); if (!elem) { log_err(LD_BUG,"(Sandbox) failed to register parameter!"); return -1; @@ -1111,8 +1108,6 @@ sandbox_cfg_allow_open_filename(sandbox_cfg_t **cfg, char *file, int fr) elem->next = *cfg; *cfg = elem;
- if (fr) tor_free(file); - return 0; }
@@ -1122,8 +1117,8 @@ sandbox_cfg_allow_rename(sandbox_cfg_t **cfg, char *file1, char *file2) sandbox_cfg_t *elem = NULL;
elem = new_element2(SCMP_SYS(rename), 0, 1, - (intptr_t)(void *)tor_strdup(file1), - (intptr_t)(void *)tor_strdup(file2)); + (intptr_t)(void *) file1, + (intptr_t)(void *) file2);
if (!elem) { log_err(LD_BUG,"(Sandbox) failed to register parameter!"); @@ -1142,8 +1137,6 @@ sandbox_cfg_allow_rename(sandbox_cfg_t **cfg, char *file1, char *file2) elem->next = *cfg; *cfg = elem;
- tor_free(file1); - tor_free(file2); return 0; }
@@ -1157,9 +1150,7 @@ sandbox_cfg_allow_open_filename_array(sandbox_cfg_t **cfg, ...) va_start(ap, cfg);
while ((fn = va_arg(ap, char*)) != NULL) { - int fr = va_arg(ap, int); - - rc = sandbox_cfg_allow_open_filename(cfg, fn, fr); + rc = sandbox_cfg_allow_open_filename(cfg, fn); if (rc) { log_err(LD_BUG,"(Sandbox) sandbox_cfg_allow_open_filename_array fail"); goto end; @@ -1172,11 +1163,11 @@ sandbox_cfg_allow_open_filename_array(sandbox_cfg_t **cfg, ...) }
int -sandbox_cfg_allow_openat_filename(sandbox_cfg_t **cfg, char *file, int fr) +sandbox_cfg_allow_openat_filename(sandbox_cfg_t **cfg, char *file) { sandbox_cfg_t *elem = NULL;
- elem = new_element(SCMP_SYS(openat), 1, (intptr_t)(void *)tor_strdup(file)); + elem = new_element(SCMP_SYS(openat), 1, (intptr_t)(void *) file); if (!elem) { log_err(LD_BUG,"(Sandbox) failed to register parameter!"); return -1; @@ -1185,8 +1176,6 @@ sandbox_cfg_allow_openat_filename(sandbox_cfg_t **cfg, char *file, int fr) elem->next = *cfg; *cfg = elem;
- if (fr) tor_free(file); - return 0; }
@@ -1200,9 +1189,7 @@ sandbox_cfg_allow_openat_filename_array(sandbox_cfg_t **cfg, ...) va_start(ap, cfg);
while ((fn = va_arg(ap, char*)) != NULL) { - int fr = va_arg(ap, int); - - rc = sandbox_cfg_allow_openat_filename(cfg, fn, fr); + rc = sandbox_cfg_allow_openat_filename(cfg, fn); if (rc) { log_err(LD_BUG,"(Sandbox) sandbox_cfg_allow_openat_filename_array fail"); goto end; @@ -1219,7 +1206,7 @@ sandbox_cfg_allow_execve(sandbox_cfg_t **cfg, const char *com) { sandbox_cfg_t *elem = NULL;
- elem = new_element(SCMP_SYS(execve), 1, (intptr_t)(void *)tor_strdup(com)); + elem = new_element(SCMP_SYS(execve), 1, (intptr_t)(void *) com); if (!elem) { log_err(LD_BUG,"(Sandbox) failed to register parameter!"); return -1; @@ -1519,7 +1506,8 @@ register_cfg(sandbox_cfg_t* cfg) return 0; }
- for (elem = filter_dynamic; elem->next != NULL; elem = elem->next); + for (elem = filter_dynamic; elem->next != NULL; elem = elem->next) + ;
elem->next = cfg;
@@ -1583,10 +1571,9 @@ sandbox_init(sandbox_cfg_t *cfg)
#ifndef USE_LIBSECCOMP int -sandbox_cfg_allow_open_filename(sandbox_cfg_t **cfg, char *file, - int fr) +sandbox_cfg_allow_open_filename(sandbox_cfg_t **cfg, char *file) { - (void)cfg; (void)file; (void)fr; + (void)cfg; (void)file; return 0; }
@@ -1598,10 +1585,9 @@ sandbox_cfg_allow_open_filename_array(sandbox_cfg_t **cfg, ...) }
int -sandbox_cfg_allow_openat_filename(sandbox_cfg_t **cfg, char *file, - int fr) +sandbox_cfg_allow_openat_filename(sandbox_cfg_t **cfg, char *file) { - (void)cfg; (void)file; (void)fr; + (void)cfg; (void)file; return 0; }
@@ -1627,10 +1613,9 @@ sandbox_cfg_allow_execve_array(sandbox_cfg_t **cfg, ...) }
int -sandbox_cfg_allow_stat_filename(sandbox_cfg_t **cfg, char *file, - int fr) +sandbox_cfg_allow_stat_filename(sandbox_cfg_t **cfg, char *file) { - (void)cfg; (void)file; (void)fr; + (void)cfg; (void)file; return 0; }
diff --git a/src/common/sandbox.h b/src/common/sandbox.h index b15f31c..8fcd221 100644 --- a/src/common/sandbox.h +++ b/src/common/sandbox.h @@ -171,12 +171,10 @@ sandbox_cfg_t * sandbox_cfg_new(void);
/** * Function used to add a open allowed filename to a supplied configuration. - * The (char*) specifies the path to the allowed file, fr = 1 tells the - * function that the char* needs to be free-ed, 0 means the pointer does not - * need to be free-ed. + * The (char*) specifies the path to the allowed file; we take ownership + * of the pointer. */ -int sandbox_cfg_allow_open_filename(sandbox_cfg_t **cfg, char *file, - int fr); +int sandbox_cfg_allow_open_filename(sandbox_cfg_t **cfg, char *file);
/**DOCDOC*/ int sandbox_cfg_allow_rename(sandbox_cfg_t **cfg, char *file1, char *file2); @@ -184,66 +182,51 @@ int sandbox_cfg_allow_rename(sandbox_cfg_t **cfg, char *file1, char *file2); /** Function used to add a series of open allowed filenames to a supplied * configuration. * @param cfg sandbox configuration. - * @param ... all future parameters are specified as pairs of <(char*), 1 / 0> - * the char* specifies the path to the allowed file, 1 tells the function - * that the char* needs to be free-ed, 0 means the pointer does not need to - * be free-ed; the final parameter needs to be <NULL, 0>. - */ + * @param ... a list of stealable pointers to permitted files. The last + * one must be NULL. +*/ int sandbox_cfg_allow_open_filename_array(sandbox_cfg_t **cfg, ...);
/** * Function used to add a openat allowed filename to a supplied configuration. - * The (char*) specifies the path to the allowed file, fr = 1 tells the - * function that the char* needs to be free-ed, 0 means the pointer does not - * need to be free-ed. + * The (char*) specifies the path to the allowed file; we steal the pointer to + * that file. */ -int sandbox_cfg_allow_openat_filename(sandbox_cfg_t **cfg, char *file, - int fr); +int sandbox_cfg_allow_openat_filename(sandbox_cfg_t **cfg, char *file);
/** Function used to add a series of openat allowed filenames to a supplied * configuration. * @param cfg sandbox configuration. - * @param ... all future parameters are specified as pairs of <(char*), 1 / 0> - * the char* specifies the path to the allowed file, 1 tells the function - * that the char* needs to be free-ed, 0 means the pointer does not need to - * be free-ed; the final parameter needs to be <NULL, 0>. + * @param ... a list of stealable pointers to permitted files. The last + * one must be NULL. */ int sandbox_cfg_allow_openat_filename_array(sandbox_cfg_t **cfg, ...);
/** * Function used to add a execve allowed filename to a supplied configuration. - * The (char*) specifies the path to the allowed file, fr = 1 tells the - * function that the char* needs to be free-ed, 0 means the pointer does not - * need to be free-ed. + * The (char*) specifies the path to the allowed file; that pointer is stolen. */ int sandbox_cfg_allow_execve(sandbox_cfg_t **cfg, const char *com);
/** Function used to add a series of execve allowed filenames to a supplied * configuration. * @param cfg sandbox configuration. - * @param ... all future parameters are specified as pairs of <(char*), 1 / 0> - * the char* specifies the path to the allowed file, 1 tells the function - * that the char* needs to be free-ed, 0 means the pointer does not need to - * be free-ed; the final parameter needs to be <NULL, 0>. + * @param ... an array of stealable pointers to permitted files. The last + * one must be NULL. */ int sandbox_cfg_allow_execve_array(sandbox_cfg_t **cfg, ...);
/** * Function used to add a stat/stat64 allowed filename to a configuration. - * The (char*) specifies the path to the allowed file, fr = 1 tells the - * function that the char* needs to be free-ed, 0 means the pointer does not - * need to be free-ed. + * The (char*) specifies the path to the allowed file; that pointer is stolen. */ -int sandbox_cfg_allow_stat_filename(sandbox_cfg_t **cfg, char *file, - int fr); +int sandbox_cfg_allow_stat_filename(sandbox_cfg_t **cfg, char *file);
/** Function used to add a series of stat64 allowed filenames to a supplied * configuration. * @param cfg sandbox configuration. - * @param ... all future parameters are specified as pairs of <(char*), 1 / 0> - * the char* specifies the path to the allowed file, 1 tells the function - * that the char* needs to be free-ed, 0 means the pointer does not need to - * be free-ed; the final parameter needs to be <NULL, 0>. + * @param ... an array of stealable pointers to permitted files. The last + * one must be NULL. */ int sandbox_cfg_allow_stat_filename_array(sandbox_cfg_t **cfg, ...);
diff --git a/src/or/main.c b/src/or/main.c index e6c1636..341f22a 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -2726,41 +2726,41 @@ sandbox_init_filter(void) sandbox_cfg_t *cfg = sandbox_cfg_new();
sandbox_cfg_allow_openat_filename(&cfg, - get_datadir_fname("cached-status"), 1); + get_datadir_fname("cached-status"));
sandbox_cfg_allow_open_filename_array(&cfg, - get_datadir_fname("cached-certs"), 1, - get_datadir_fname("cached-certs.tmp"), 1, - get_datadir_fname("cached-consensus"), 1, - get_datadir_fname("cached-consensus.tmp"), 1, - get_datadir_fname("unverified-consensus"), 1, - get_datadir_fname("unverified-consensus.tmp"), 1, - get_datadir_fname("unverified-microdesc-consensus"), 1, - get_datadir_fname("unverified-microdesc-consensus.tmp"), 1, - get_datadir_fname("cached-microdesc-consensus"), 1, - get_datadir_fname("cached-microdesc-consensus.tmp"), 1, - get_datadir_fname("cached-microdescs"), 1, - get_datadir_fname("cached-microdescs.tmp"), 1, - get_datadir_fname("cached-microdescs.new"), 1, - get_datadir_fname("cached-microdescs.new.tmp"), 1, - get_datadir_fname("cached-descriptors"), 1, - get_datadir_fname("cached-descriptors.new"), 1, - get_datadir_fname("cached-descriptors.tmp"), 1, - get_datadir_fname("cached-descriptors.new.tmp"), 1, - get_datadir_fname("cached-descriptors.tmp.tmp"), 1, - get_datadir_fname("cached-extrainfo"), 1, - get_datadir_fname("cached-extrainfo.new"), 1, - get_datadir_fname("cached-extrainfo.tmp"), 1, - get_datadir_fname("cached-extrainfo.new.tmp"), 1, - get_datadir_fname("cached-extrainfo.tmp.tmp"), 1, - get_datadir_fname("state.tmp"), 1, - get_datadir_fname("unparseable-desc.tmp"), 1, - get_datadir_fname("unparseable-desc"), 1, - get_datadir_fname("v3-status-votes"), 1, - get_datadir_fname("v3-status-votes.tmp"), 1, - "/dev/srandom", 0, - "/dev/urandom", 0, - "/dev/random", 0, + get_datadir_fname("cached-certs"), + get_datadir_fname("cached-certs.tmp"), + get_datadir_fname("cached-consensus"), + get_datadir_fname("cached-consensus.tmp"), + get_datadir_fname("unverified-consensus"), + get_datadir_fname("unverified-consensus.tmp"), + get_datadir_fname("unverified-microdesc-consensus"), + get_datadir_fname("unverified-microdesc-consensus.tmp"), + get_datadir_fname("cached-microdesc-consensus"), + get_datadir_fname("cached-microdesc-consensus.tmp"), + get_datadir_fname("cached-microdescs"), + get_datadir_fname("cached-microdescs.tmp"), + get_datadir_fname("cached-microdescs.new"), + get_datadir_fname("cached-microdescs.new.tmp"), + get_datadir_fname("cached-descriptors"), + get_datadir_fname("cached-descriptors.new"), + get_datadir_fname("cached-descriptors.tmp"), + get_datadir_fname("cached-descriptors.new.tmp"), + get_datadir_fname("cached-descriptors.tmp.tmp"), + get_datadir_fname("cached-extrainfo"), + get_datadir_fname("cached-extrainfo.new"), + get_datadir_fname("cached-extrainfo.tmp"), + get_datadir_fname("cached-extrainfo.new.tmp"), + get_datadir_fname("cached-extrainfo.tmp.tmp"), + get_datadir_fname("state.tmp"), + get_datadir_fname("unparseable-desc.tmp"), + get_datadir_fname("unparseable-desc"), + get_datadir_fname("v3-status-votes"), + get_datadir_fname("v3-status-votes.tmp"), + tor_strdup("/dev/srandom"), + tor_strdup("/dev/urandom"), + tor_strdup("/dev/random"), NULL, 0 );
@@ -2793,31 +2793,31 @@ sandbox_init_filter(void) RENAME_SUFFIX("v3-status-votes", ".tmp");
sandbox_cfg_allow_stat_filename_array(&cfg, - get_datadir_fname(NULL), 1, - get_datadir_fname("lock"), 1, - get_datadir_fname("state"), 1, - get_datadir_fname("router-stability"), 1, - get_datadir_fname("cached-extrainfo.new"), 1, + get_datadir_fname(NULL), + get_datadir_fname("lock"), + get_datadir_fname("state"), + get_datadir_fname("router-stability"), + get_datadir_fname("cached-extrainfo.new"), NULL, 0 );
// orport if (server_mode(get_options())) { sandbox_cfg_allow_open_filename_array(&cfg, - get_datadir_fname2("keys", "secret_id_key"), 1, - get_datadir_fname2("keys", "secret_onion_key"), 1, - get_datadir_fname2("keys", "secret_onion_key_ntor"), 1, - get_datadir_fname2("keys", "secret_onion_key_ntor.tmp"), 1, - get_datadir_fname2("keys", "secret_id_key.old"), 1, - get_datadir_fname2("keys", "secret_onion_key.old"), 1, - get_datadir_fname2("keys", "secret_onion_key_ntor.old"), 1, - get_datadir_fname2("keys", "secret_onion_key.tmp"), 1, - get_datadir_fname2("keys", "secret_id_key.tmp"), 1, - get_datadir_fname("fingerprint"), 1, - get_datadir_fname("fingerprint.tmp"), 1, - get_datadir_fname("hashed-fingerprint"), 1, - get_datadir_fname("hashed-fingerprint.tmp"), 1, - "/etc/resolv.conf", 0, + get_datadir_fname2("keys", "secret_id_key"), + get_datadir_fname2("keys", "secret_onion_key"), + get_datadir_fname2("keys", "secret_onion_key_ntor"), + get_datadir_fname2("keys", "secret_onion_key_ntor.tmp"), + get_datadir_fname2("keys", "secret_id_key.old"), + get_datadir_fname2("keys", "secret_onion_key.old"), + get_datadir_fname2("keys", "secret_onion_key_ntor.old"), + get_datadir_fname2("keys", "secret_onion_key.tmp"), + get_datadir_fname2("keys", "secret_id_key.tmp"), + get_datadir_fname("fingerprint"), + get_datadir_fname("fingerprint.tmp"), + get_datadir_fname("hashed-fingerprint"), + get_datadir_fname("hashed-fingerprint.tmp"), + tor_strdup("/etc/resolv.conf"), NULL, 0 );
@@ -2830,8 +2830,8 @@ sandbox_init_filter(void) RENAME_SUFFIX("hashed-fingerprint", ".tmp");
sandbox_cfg_allow_stat_filename_array(&cfg, - get_datadir_fname("keys"), 1, - get_datadir_fname("stats/dirreq-stats"), 1, + get_datadir_fname("keys"), + get_datadir_fname("stats/dirreq-stats"), NULL, 0 ); }