From 947a6daa311ebc139043fe39b775ee5928014fd6 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Tue, 18 Mar 2014 11:04:20 -0700 Subject: Always check returns from tor_munmap_file() in routerlist.c --- src/or/routerlist.c | 42 +++++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 9 deletions(-) (limited to 'src/or') diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 8d29b89ea..725718be9 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -990,6 +990,7 @@ router_rebuild_store(int flags, desc_store_t *store) size_t total_expected_len = 0; int had_any; int force = flags & RRS_FORCE; + int res; if (!force && !router_should_rebuild_store(store)) { r = 0; @@ -1064,8 +1065,12 @@ router_rebuild_store(int flags, desc_store_t *store) /* Our mmap is now invalid. */ if (store->mmap) { - tor_munmap_file(store->mmap); - store->mmap = NULL; + res = tor_munmap_file(store->mmap); + if (res == 0) { + store->mmap = NULL; + } else { + log_warn(LD_FS, "Unable to munmap route store in %s", fname); + } } if (replace_file(fname_tmp, fname)<0) { @@ -1136,12 +1141,21 @@ router_reload_router_list_impl(desc_store_t *store) struct stat st; int extrainfo = (store->type == EXTRAINFO_STORE); store->journal_len = store->store_len = 0; + int res; fname = get_datadir_fname(store->fname_base); - if (store->mmap) /* get rid of it first */ - tor_munmap_file(store->mmap); - store->mmap = NULL; + if (store->mmap) { + /* get rid of it first */ + res = tor_munmap_file(store->mmap); + if (res == 0) { + store->mmap = NULL; + } else { + log_warn(LD_FS, "Failed to munmap %s", fname); + tor_free(fname); + return -1; + } + } store->mmap = tor_mmap_file(fname); if (store->mmap) { @@ -2782,6 +2796,8 @@ extrainfo_free_(void *e) void routerlist_free(routerlist_t *rl) { + int res; + if (!rl) return; rimap_free(rl->identity_map, NULL); @@ -2794,10 +2810,18 @@ routerlist_free(routerlist_t *rl) signed_descriptor_free(sd)); smartlist_free(rl->routers); smartlist_free(rl->old_routers); - if (routerlist->desc_store.mmap) - tor_munmap_file(routerlist->desc_store.mmap); - if (routerlist->extrainfo_store.mmap) - tor_munmap_file(routerlist->extrainfo_store.mmap); + if (routerlist->desc_store.mmap) { + res = tor_munmap_file(routerlist->desc_store.mmap); + if (res != 0) { + log_warn(LD_FS, "Failed to munmap routerlist->desc_store.mmap"); + } + } + if (routerlist->extrainfo_store.mmap) { + res = tor_munmap_file(routerlist->extrainfo_store.mmap); + if (res != 0) { + log_warn(LD_FS, "Failed to munmap routerlist->extrainfo_store.mmap"); + } + } tor_free(rl); router_dir_info_changed(); -- cgit v1.2.3 From df076eccfaa680ee08b8ae866690d9a2a8ba5555 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Tue, 18 Mar 2014 12:39:02 -0700 Subject: Always check returns from tor_munmap_file() in microdesc.c --- src/or/microdesc.c | 20 ++++++++++++++++---- src/or/routerlist.c | 22 ++++++++-------------- 2 files changed, 24 insertions(+), 18 deletions(-) (limited to 'src/or') diff --git a/src/or/microdesc.c b/src/or/microdesc.c index 6419ea79f..f196536da 100644 --- a/src/or/microdesc.c +++ b/src/or/microdesc.c @@ -275,6 +275,7 @@ void microdesc_cache_clear(microdesc_cache_t *cache) { microdesc_t **entry, **next; + for (entry = HT_START(microdesc_map, &cache->map); entry; entry = next) { microdesc_t *md = *entry; next = HT_NEXT_RMV(microdesc_map, &cache->map, entry); @@ -283,7 +284,13 @@ microdesc_cache_clear(microdesc_cache_t *cache) } HT_CLEAR(microdesc_map, &cache->map); if (cache->cache_content) { - tor_munmap_file(cache->cache_content); + int res = tor_munmap_file(cache->cache_content); + if (res != 0) { + log_warn(LD_FS, + "tor_munmap_file() failed clearing microdesc cache; " + "we are probably about to leak memory."); + /* TODO something smarter? */ + } cache->cache_content = NULL; } cache->total_len_seen = 0; @@ -429,7 +436,7 @@ int microdesc_cache_rebuild(microdesc_cache_t *cache, int force) { open_file_t *open_file; - int fd = -1; + int fd = -1, res; microdesc_t **mdp; smartlist_t *wrote; ssize_t size; @@ -496,8 +503,13 @@ microdesc_cache_rebuild(microdesc_cache_t *cache, int force) /* We must do this unmap _before_ we call finish_writing_to_file(), or * windows will not actually replace the file. */ - if (cache->cache_content) - tor_munmap_file(cache->cache_content); + if (cache->cache_content) { + res = tor_munmap_file(cache->cache_content); + if (res != 0) { + log_warn(LD_FS, + "Failed to unmap old microdescriptor cache while rebuilding"); + } + } if (finish_writing_to_file(open_file) < 0) { log_warn(LD_DIR, "Error rebuilding microdescriptor cache: %s", diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 725718be9..c96fb2c11 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -990,7 +990,6 @@ router_rebuild_store(int flags, desc_store_t *store) size_t total_expected_len = 0; int had_any; int force = flags & RRS_FORCE; - int res; if (!force && !router_should_rebuild_store(store)) { r = 0; @@ -1065,10 +1064,9 @@ router_rebuild_store(int flags, desc_store_t *store) /* Our mmap is now invalid. */ if (store->mmap) { - res = tor_munmap_file(store->mmap); - if (res == 0) { - store->mmap = NULL; - } else { + int res = tor_munmap_file(store->mmap); + store->mmap = NULL; + if (res != 0) { log_warn(LD_FS, "Unable to munmap route store in %s", fname); } } @@ -1141,16 +1139,14 @@ router_reload_router_list_impl(desc_store_t *store) struct stat st; int extrainfo = (store->type == EXTRAINFO_STORE); store->journal_len = store->store_len = 0; - int res; fname = get_datadir_fname(store->fname_base); if (store->mmap) { /* get rid of it first */ - res = tor_munmap_file(store->mmap); - if (res == 0) { - store->mmap = NULL; - } else { + int res = tor_munmap_file(store->mmap); + store->mmap = NULL; + if (res != 0) { log_warn(LD_FS, "Failed to munmap %s", fname); tor_free(fname); return -1; @@ -2796,8 +2792,6 @@ extrainfo_free_(void *e) void routerlist_free(routerlist_t *rl) { - int res; - if (!rl) return; rimap_free(rl->identity_map, NULL); @@ -2811,13 +2805,13 @@ routerlist_free(routerlist_t *rl) smartlist_free(rl->routers); smartlist_free(rl->old_routers); if (routerlist->desc_store.mmap) { - res = tor_munmap_file(routerlist->desc_store.mmap); + int res = tor_munmap_file(routerlist->desc_store.mmap); if (res != 0) { log_warn(LD_FS, "Failed to munmap routerlist->desc_store.mmap"); } } if (routerlist->extrainfo_store.mmap) { - res = tor_munmap_file(routerlist->extrainfo_store.mmap); + int res = tor_munmap_file(routerlist->extrainfo_store.mmap); if (res != 0) { log_warn(LD_FS, "Failed to munmap routerlist->extrainfo_store.mmap"); } -- cgit v1.2.3 From abdf1878a3f8fe366d8bb7f649127880bdbdf82d Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Tue, 18 Mar 2014 17:52:31 -0700 Subject: Always check returns from unlink() --- src/common/util.c | 9 ++++++++- src/or/config.c | 5 ++++- src/or/hibernate.c | 10 +++++++++- src/or/main.c | 17 +++++++++++++---- src/or/networkstatus.c | 21 +++++++++++++++++---- src/or/statefile.c | 10 ++++++++-- 6 files changed, 59 insertions(+), 13 deletions(-) (limited to 'src/or') diff --git a/src/common/util.c b/src/common/util.c index 3c2f6643a..c4f8c8802 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -2141,6 +2141,7 @@ static int finish_writing_to_file_impl(open_file_t *file_data, int abort_write) { int r = 0; + tor_assert(file_data && file_data->filename); if (file_data->stdio_file) { if (fclose(file_data->stdio_file)) { @@ -2157,7 +2158,13 @@ finish_writing_to_file_impl(open_file_t *file_data, int abort_write) if (file_data->rename_on_close) { tor_assert(file_data->tempname && file_data->filename); if (abort_write) { - unlink(file_data->tempname); + int res = unlink(file_data->tempname); + if (res != 0) { + /* We couldn't unlink and we'll leave a mess behind */ + log_warn(LD_FS, "Failed to unlink %s: %s", + file_data->tempname, strerror(errno)); + r = -1; + } } else { tor_assert(strcmp(file_data->filename, file_data->tempname)); if (replace_file(file_data->tempname, file_data->filename)) { diff --git a/src/or/config.c b/src/or/config.c index 0da4877a5..5b590f21c 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -6464,7 +6464,10 @@ remove_file_if_very_old(const char *fname, time_t now) format_local_iso_time(buf, st.st_mtime); log_notice(LD_GENERAL, "Obsolete file %s hasn't been modified since %s. " "Removing it.", fname, buf); - unlink(fname); + if (unlink(fname) != 0) { + log_warn(LD_FS, "Failed to unlink %s: %s", + fname, strerror(errno)); + } } } diff --git a/src/or/hibernate.c b/src/or/hibernate.c index 607dec8cd..bbda8424f 100644 --- a/src/or/hibernate.c +++ b/src/or/hibernate.c @@ -648,7 +648,15 @@ read_bandwidth_usage(void) { char *fname = get_datadir_fname("bw_accounting"); - unlink(fname); + int res; + + res = unlink(fname); + if (res != 0) { + log_warn(LD_FS, + "Failed to unlink %s: %s", + fname, strerror(errno)); + } + tor_free(fname); } diff --git a/src/or/main.c b/src/or/main.c index 7294c8955..3d5ce10cf 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -2574,10 +2574,19 @@ tor_cleanup(void) time_t now = time(NULL); /* Remove our pid file. We don't care if there was an error when we * unlink, nothing we could do about it anyways. */ - if (options->PidFile) - unlink(options->PidFile); - if (options->ControlPortWriteToFile) - unlink(options->ControlPortWriteToFile); + if (options->PidFile) { + if (unlink(options->PidFile) != 0) { + log_warn(LD_FS, "Couldn't unlink pid file %s: %s", + options->PidFile, strerror(errno)); + } + } + if (options->ControlPortWriteToFile) { + if (unlink(options->ControlPortWriteToFile) != 0) { + log_warn(LD_FS, "Couldn't unlink control port file %s: %s", + options->ControlPortWriteToFile, + strerror(errno)); + } + } if (accounting_is_enabled(options)) accounting_record_bandwidth_usage(now, get_or_state()); or_state_mark_dirty(get_or_state(), 0); /* force an immediate save. */ diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 49478a734..74c4ca45a 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -1254,7 +1254,11 @@ networkstatus_set_current_consensus(const char *consensus, /* Even if we had enough signatures, we'd never use this as the * latest consensus. */ if (was_waiting_for_certs && from_cache) - unlink(unverified_fname); + if (unlink(unverified_fname) != 0) { + log_warn(LD_FS, + "Failed to unlink %s: %s", + unverified_fname, strerror(errno)); + } } goto done; } else { @@ -1264,8 +1268,13 @@ networkstatus_set_current_consensus(const char *consensus, "consensus"); result = -2; } - if (was_waiting_for_certs && (r < -1) && from_cache) - unlink(unverified_fname); + if (was_waiting_for_certs && (r < -1) && from_cache) { + if (unlink(unverified_fname) != 0) { + log_warn(LD_FS, + "Failed to unlink %s: %s", + unverified_fname, strerror(errno)); + } + } goto done; } } @@ -1313,7 +1322,11 @@ networkstatus_set_current_consensus(const char *consensus, waiting->body = NULL; waiting->set_at = 0; waiting->dl_failed = 0; - unlink(unverified_fname); + if (unlink(unverified_fname) != 0) { + log_warn(LD_FS, + "Failed to unlink %s: %s", + unverified_fname, strerror(errno)); + } } /* Reset the failure count only if this consensus is actually valid. */ diff --git a/src/or/statefile.c b/src/or/statefile.c index 8ab04763d..2251f25e9 100644 --- a/src/or/statefile.c +++ b/src/or/statefile.c @@ -260,7 +260,7 @@ or_state_set(or_state_t *new_state) static void or_state_save_broken(char *fname) { - int i; + int i, res; file_status_t status; char *fname2 = NULL; for (i = 0; i < 100; ++i) { @@ -274,7 +274,13 @@ or_state_save_broken(char *fname) log_warn(LD_BUG, "Unable to parse state in \"%s\"; too many saved bad " "state files to move aside. Discarding the old state file.", fname); - unlink(fname); + res = unlink(fname); + if (res != 0) { + log_warn(LD_FS, + "Also couldn't discard old state file \"%s\" because " + "unlink() failed: %s", + fname, strerror(errno)); + } } else { log_warn(LD_BUG, "Unable to parse state in \"%s\". Moving it aside " "to \"%s\". This could be a bug in Tor; please tell " -- cgit v1.2.3 From 449b87791d5a9f15c63b39e0ff36f8ba89676c6c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 31 Mar 2014 11:42:49 -0400 Subject: NULL out all mappings after tor_munmap_file() --- src/or/microdesc.c | 1 + 1 file changed, 1 insertion(+) (limited to 'src/or') diff --git a/src/or/microdesc.c b/src/or/microdesc.c index f196536da..9c91f17a6 100644 --- a/src/or/microdesc.c +++ b/src/or/microdesc.c @@ -509,6 +509,7 @@ microdesc_cache_rebuild(microdesc_cache_t *cache, int force) log_warn(LD_FS, "Failed to unmap old microdescriptor cache while rebuilding"); } + cache->cache_content = NULL; } if (finish_writing_to_file(open_file) < 0) { -- cgit v1.2.3 From 1a9b4bd28cf22e28d2c1b7b24a092d85c7c61139 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 31 Mar 2014 11:43:11 -0400 Subject: Munmap the right pointers in routerlist_free() --- src/or/routerlist.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/or') diff --git a/src/or/routerlist.c b/src/or/routerlist.c index c96fb2c11..44c698738 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -2804,13 +2804,13 @@ routerlist_free(routerlist_t *rl) signed_descriptor_free(sd)); smartlist_free(rl->routers); smartlist_free(rl->old_routers); - if (routerlist->desc_store.mmap) { + if (rl->desc_store.mmap) { int res = tor_munmap_file(routerlist->desc_store.mmap); if (res != 0) { log_warn(LD_FS, "Failed to munmap routerlist->desc_store.mmap"); } } - if (routerlist->extrainfo_store.mmap) { + if (rl->extrainfo_store.mmap) { int res = tor_munmap_file(routerlist->extrainfo_store.mmap); if (res != 0) { log_warn(LD_FS, "Failed to munmap routerlist->extrainfo_store.mmap"); -- cgit v1.2.3