From f6841ae26374d3b4d0b479049d7a7c237afffe5c Mon Sep 17 00:00:00 2001 From: teor Date: Sun, 28 May 2017 22:11:22 +1000 Subject: [PATCH 1/6] Fix comment typos in storage.c --- src/common/storagedir.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/common/storagedir.c b/src/common/storagedir.c index 309d42db17..6457f3d009 100644 --- a/src/common/storagedir.c +++ b/src/common/storagedir.c @@ -76,8 +76,8 @@ storage_dir_free(storage_dir_t *d) * operations that d will need. * * The presence of this function is why we need an upper limit on the - * number of filers in a storage_dir_t: we need to approve file - * operaitons one by one. + * number of files in a storage_dir_t: we need to approve file operations + * one by one. */ int storage_dir_register_with_sandbox(storage_dir_t *d, sandbox_cfg_t **cfg) @@ -309,9 +309,8 @@ storage_dir_save_string_to_file(storage_dir_t *d, /** * As storage_dir_save_bytes_to_file, but associates the data with the - * key-value pairs in labels. Files - * stored in this format can be recovered with storage_dir_map_labeled - * or storage_dir_read_labeled(). + * key-value pairs in labels. Files stored in this format can be + * recovered with storage_dir_map_labeled() or storage_dir_read_labeled(). */ int storage_dir_save_labeled_to_file(storage_dir_t *d, @@ -356,12 +355,12 @@ storage_dir_save_labeled_to_file(storage_dir_t *d, } /** - * Map a file that was created with storage_dir_save_labeled(). On failure, - * return NULL. On success, write a set of newly allocated labels into to - * *labels_out, a pointer to the into *data_out, and the data's - * into *sz_out. On success, also return a tor_mmap_t object whose - * contents should not be used -- it needs to be kept around, though, for as - * long as data_out is going to be valid. + * Map a file that was created with storage_dir_save_labeled_to_file(). On + * failure, return NULL. On success, write a set of newly allocated labels + * into *labels_out, a pointer to the data into *data_out, and + * the data's size into *sz_out. On success, also return a tor_mmap_t + * object whose contents should not be used -- it needs to be kept around, + * though, for as long as data_out is going to be valid. */ tor_mmap_t * storage_dir_map_labeled(storage_dir_t *dir, From 9e36b0beb9dc6fe02d43b4c217841c8164f41774 Mon Sep 17 00:00:00 2001 From: teor Date: Sun, 28 May 2017 22:12:09 +1000 Subject: [PATCH 2/6] Always check for usage underflow when removing a file in storage.c Part of #22424. --- src/common/storagedir.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/common/storagedir.c b/src/common/storagedir.c index 6457f3d009..9140ed29bd 100644 --- a/src/common/storagedir.c +++ b/src/common/storagedir.c @@ -425,7 +425,9 @@ storage_dir_remove_file(storage_dir_t *d, } } if (unlink(ipath) == 0) { - d->usage -= size; + if (! BUG(d->usage < size)) { + d->usage -= size; + } } else { log_warn(LD_FS, "Unable to unlink %s", escaped(path)); tor_free(path); From 334fe6bb6b124c2f3eb1186610999446d57cde76 Mon Sep 17 00:00:00 2001 From: teor Date: Sun, 28 May 2017 22:16:00 +1000 Subject: [PATCH 3/6] Don't underflow usage when it is unknown and a file is removed Part of #22424. --- src/common/storagedir.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/common/storagedir.c b/src/common/storagedir.c index 9140ed29bd..532fa2a2c8 100644 --- a/src/common/storagedir.c +++ b/src/common/storagedir.c @@ -425,7 +425,7 @@ storage_dir_remove_file(storage_dir_t *d, } } if (unlink(ipath) == 0) { - if (! BUG(d->usage < size)) { + if (d->usage_known && ! BUG(d->usage < size)) { d->usage -= size; } } else { From 69b234a0a8250103e44a486c038eb5e11b3128ec Mon Sep 17 00:00:00 2001 From: teor Date: Sun, 28 May 2017 22:21:27 +1000 Subject: [PATCH 4/6] Refactor storage usage reductions into a static function No behaviour change. Part of #22424. --- src/common/storagedir.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/common/storagedir.c b/src/common/storagedir.c index 532fa2a2c8..cc2230d502 100644 --- a/src/common/storagedir.c +++ b/src/common/storagedir.c @@ -406,6 +406,22 @@ storage_dir_read_labeled(storage_dir_t *dir, return result; } +/* Reduce the cached usage amount in d by removed_file_size. + * This function is a no-op if d->usage_known is 0. */ +static void +storage_dir_reduce_usage(storage_dir_t *d, uint64_t removed_file_size) +{ + if (d->usage_known) { + if (! BUG(d->usage < removed_file_size)) { + /* This bug can also be triggered if an external process resized a file + * between the call to storage_dir_get_usage() that last checked + * actual usage (rather than relaying on cached usage), and the call to + * this function. */ + d->usage -= removed_file_size; + } + } +} + /** * Remove the file called fname from d. */ @@ -425,9 +441,7 @@ storage_dir_remove_file(storage_dir_t *d, } } if (unlink(ipath) == 0) { - if (d->usage_known && ! BUG(d->usage < size)) { - d->usage -= size; - } + storage_dir_reduce_usage(d, size); } else { log_warn(LD_FS, "Unable to unlink %s", escaped(path)); tor_free(path); @@ -506,9 +520,7 @@ storage_dir_shrink(storage_dir_t *d, int idx = 0; while ((d->usage > target_size || min_to_remove > 0) && idx < n) { if (unlink(sandbox_intern_string(ents[idx].path)) == 0) { - if (! BUG(d->usage < ents[idx].size)) { - d->usage -= ents[idx].size; - } + storage_dir_reduce_usage(d, ents[idx].size); --min_to_remove; } ++idx; From 79725289e1f0ed86e2afb37748c83571118d465b Mon Sep 17 00:00:00 2001 From: teor Date: Sun, 28 May 2017 22:28:56 +1000 Subject: [PATCH 5/6] If we do underflow the know usage of a storage, recalculate it Fixes bug #22424 on 0.3.1.1-alpha. --- src/common/storagedir.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/common/storagedir.c b/src/common/storagedir.c index cc2230d502..befcfe693f 100644 --- a/src/common/storagedir.c +++ b/src/common/storagedir.c @@ -418,6 +418,16 @@ storage_dir_reduce_usage(storage_dir_t *d, uint64_t removed_file_size) * actual usage (rather than relaying on cached usage), and the call to * this function. */ d->usage -= removed_file_size; + } else { + /* If we underflowed the cached directory size, re-check the sizes of all + * the files in the directory. This makes storage_dir_shrink() quadratic, + * but only if a process is continually changing file sizes in the + * storage directory (in which case, we have bigger issues). + * + * We can't just reset usage_known, because storage_dir_shrink() relies + * on knowing the usage. */ + storage_dir_rescan(d); + (void)storage_dir_get_usage(d); } } } From af891e7f2ca8185c973805ca620f620a0a3ab8aa Mon Sep 17 00:00:00 2001 From: teor Date: Sun, 28 May 2017 22:34:56 +1000 Subject: [PATCH 6/6] Changes file for bug 22424 --- changes/bug22424 | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changes/bug22424 diff --git a/changes/bug22424 b/changes/bug22424 new file mode 100644 index 0000000000..de4cff7d2e --- /dev/null +++ b/changes/bug22424 @@ -0,0 +1,5 @@ + o Minor bugfixes (storage directories): + - Always check for underflows in the cached storage directory usage amount. + If the usage does underflow, re-calculate the usage. Also, avoid a + separate underflow when the usage is not known. + Fixes bug 22424 in 0.3.1.1-alpha.