commit c4812698c3df0bd8aa51c615b0274bbb53b9eb6c Author: Nick Mathewson <nickm@torproject.org> Date: Wed Sep 23 14:08:24 2020 -0400 Remove long-obsolete members from the state file. Tor has a feature to preserve unrecognized state file entries in order to maintain forward compatibility. But this feature, along with some unused code that we never actually removed, led to us keeping items that were of no use to the user, other than at worst to preserve ancient information about them. This commit adds a feature to remove obsolete entries when we load the file. Closes ticket 40137. --- changes/ticket40137 | 6 ++++ src/app/config/or_state_st.h | 8 +---- src/app/config/statefile.c | 77 ++++++++++++++++++++++++++++++++++---------- src/app/config/statefile.h | 1 + src/test/include.am | 1 + src/test/test.c | 1 + src/test/test.h | 1 + src/test/test_statefile.c | 56 ++++++++++++++++++++++++++++++++ 8 files changed, 127 insertions(+), 24 deletions(-) diff --git a/changes/ticket40137 b/changes/ticket40137 new file mode 100644 index 0000000000..056f1bc4a5 --- /dev/null +++ b/changes/ticket40137 @@ -0,0 +1,6 @@ + o Minor features (state): + - When loading the state file, remove entries from the statefile that + have been obsolete for a long time. Ordinarily Tor preserves + unrecognized entries in order to keep forward-compatibility, but + these statefile entries have not actually been used in any release + since before the 0.3.5.x. Closes ticket 40137. diff --git a/src/app/config/or_state_st.h b/src/app/config/or_state_st.h index 31b7f8a983..6769ef7b87 100644 --- a/src/app/config/or_state_st.h +++ b/src/app/config/or_state_st.h @@ -38,17 +38,11 @@ struct or_state_t { uint64_t AccountingBytesAtSoftLimit; uint64_t AccountingExpectedUsage; - /** A list of Entry Guard-related configuration lines. (pre-prop271) */ - struct config_line_t *EntryGuards; - - /** A list of guard-related configuration lines. (post-prop271) */ + /** A list of guard-related configuration lines. */ struct config_line_t *Guard; struct config_line_t *TransportProxies; - /** Cached revision counters for active hidden services on this host */ - struct config_line_t *HidServRevCounter; - /** These fields hold information on the history of bandwidth usage for * servers. The "Ends" fields hold the time when we last updated the * bandwidth usage. The "Interval" fields hold the granularity, in seconds, diff --git a/src/app/config/statefile.c b/src/app/config/statefile.c index b25167d2ec..22b15fcf24 100644 --- a/src/app/config/statefile.c +++ b/src/app/config/statefile.c @@ -58,16 +58,38 @@ /** A list of state-file "abbreviations," for compatibility. */ static config_abbrev_t state_abbrevs_[] = { - { "AccountingBytesReadInterval", "AccountingBytesReadInInterval", 0, 0 }, - { "HelperNode", "EntryGuard", 0, 0 }, - { "HelperNodeDownSince", "EntryGuardDownSince", 0, 0 }, - { "HelperNodeUnlistedSince", "EntryGuardUnlistedSince", 0, 0 }, - { "EntryNode", "EntryGuard", 0, 0 }, - { "EntryNodeDownSince", "EntryGuardDownSince", 0, 0 }, - { "EntryNodeUnlistedSince", "EntryGuardUnlistedSince", 0, 0 }, { NULL, NULL, 0, 0}, }; +/** A list of obsolete keys that we do not and should not preserve. + * + * We could just let these live in ExtraLines indefinitely, but they're + * never going to be used again, and every version that used them + * has been obsolete for a long time. + * */ +static const char *obsolete_state_keys[] = { + /* These were renamed in 0.1.1.11-alpha */ + "AccountingBytesReadInterval", + "HelperNode", + "HelperNodeDownSince", + "HelperNodeUnlistedSince", + "EntryNode", + "HelperNodeDownSince", + "EntryNodeUnlistedSince", + /* These were replaced by "Guard" in 0.3.0.1-alpha. */ + "EntryGuard", + "EntryGuardDownSince", + "EntryGuardUnlistedSince", + "EntryGuardAddedBy", + "EntryGuardPathBias", + "EntryGuardPathUseBias", + /* This was replaced by OPE-based revision numbers in 0.3.5.1-alpha, + * and was never actually used in a released version. */ + "HidServRevCounter", + + NULL, +}; + /** dummy instance of or_state_t, used for type-checking its * members with CONF_CHECK_VAR_TYPE. */ DUMMY_TYPECHECK_INSTANCE(or_state_t); @@ -91,19 +113,9 @@ static const config_var_t state_vars_[] = { V(AccountingSoftLimitHitAt, ISOTIME, NULL), V(AccountingBytesAtSoftLimit, MEMUNIT, NULL), - VAR("EntryGuard", LINELIST_S, EntryGuards, NULL), - VAR("EntryGuardDownSince", LINELIST_S, EntryGuards, NULL), - VAR("EntryGuardUnlistedSince", LINELIST_S, EntryGuards, NULL), - VAR("EntryGuardAddedBy", LINELIST_S, EntryGuards, NULL), - VAR("EntryGuardPathBias", LINELIST_S, EntryGuards, NULL), - VAR("EntryGuardPathUseBias", LINELIST_S, EntryGuards, NULL), - V(EntryGuards, LINELIST_V, NULL), - VAR("TransportProxy", LINELIST_S, TransportProxies, NULL), V(TransportProxies, LINELIST_V, NULL), - V(HidServRevCounter, LINELIST, NULL), - V(BWHistoryReadEnds, ISOTIME, NULL), V(BWHistoryReadInterval, POSINT, "900"), V(BWHistoryReadValues, CSV, ""), @@ -475,6 +487,7 @@ or_state_load(void) } else { log_info(LD_GENERAL, "Initialized state"); } + or_state_remove_obsolete_lines(&new_state->ExtraLines); if (or_state_set(new_state) == -1) { or_state_save_broken(fname); } @@ -494,6 +507,36 @@ or_state_load(void) return r; } +/** Remove from `extra_lines` every element whose key appears in + * `obsolete_state_keys`. */ +STATIC void +or_state_remove_obsolete_lines(config_line_t **extra_lines) +{ + /* make a strmap for the obsolete state names, so we can have O(1) + lookup. */ + strmap_t *bad_keys = strmap_new(); + for (unsigned i = 0; obsolete_state_keys[i] != NULL; ++i) { + strmap_set_lc(bad_keys, obsolete_state_keys[i], (void*)"rmv"); + } + + config_line_t **line = extra_lines; + while (*line) { + if (strmap_get_lc(bad_keys, (*line)->key) != NULL) { + /* This key is obsolete; remove it. */ + config_line_t *victim = *line; + *line = (*line)->next; + + victim->next = NULL; // prevent double-free. + config_free_lines(victim); + } else { + /* This is just an unrecognized key; keep it. */ + line = &(*line)->next; + } + } + + strmap_free(bad_keys, NULL); +} + /** Did the last time we tried to write the state file fail? If so, we * should consider disabling such features as preemptive circuit generation * to compute circuit-build-time. */ diff --git a/src/app/config/statefile.h b/src/app/config/statefile.h index 98d9d2dda1..89b10560f3 100644 --- a/src/app/config/statefile.h +++ b/src/app/config/statefile.h @@ -33,6 +33,7 @@ STATIC void or_state_free_(or_state_t *state); STATIC or_state_t *or_state_new(void); struct config_mgr_t; STATIC const struct config_mgr_t *get_state_mgr(void); +STATIC void or_state_remove_obsolete_lines(struct config_line_t **extra_lines); #endif /* defined(STATEFILE_PRIVATE) */ #endif /* !defined(TOR_STATEFILE_H) */ diff --git a/src/test/include.am b/src/test/include.am index 215e423834..173f007fbf 100644 --- a/src/test/include.am +++ b/src/test/include.am @@ -235,6 +235,7 @@ src_test_test_SOURCES += \ src/test/test_sendme.c \ src/test/test_shared_random.c \ src/test/test_socks.c \ + src/test/test_statefile.c \ src/test/test_stats.c \ src/test/test_status.c \ src/test/test_storagedir.c \ diff --git a/src/test/test.c b/src/test/test.c index 38c8206eea..77aa6db975 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -770,6 +770,7 @@ struct testgroup_t testgroups[] = { { "sendme/", sendme_tests }, { "shared-random/", sr_tests }, { "socks/", socks_tests }, + { "statefile/", statefile_tests }, { "stats/", stats_tests }, { "status/" , status_tests }, { "storagedir/", storagedir_tests }, diff --git a/src/test/test.h b/src/test/test.h index 71066db48e..bd3a4102f5 100644 --- a/src/test/test.h +++ b/src/test/test.h @@ -187,6 +187,7 @@ extern struct testcase_t scheduler_tests[]; extern struct testcase_t sendme_tests[]; extern struct testcase_t socks_tests[]; extern struct testcase_t sr_tests[]; +extern struct testcase_t statefile_tests[]; extern struct testcase_t stats_tests[]; extern struct testcase_t status_tests[]; extern struct testcase_t storagedir_tests[]; diff --git a/src/test/test_statefile.c b/src/test/test_statefile.c new file mode 100644 index 0000000000..dc9ecfee3e --- /dev/null +++ b/src/test/test_statefile.c @@ -0,0 +1,56 @@ +/* Copyright (c) 2001-2004, Roger Dingledine. + * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson. + * Copyright (c) 2007-2020, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +#include "orconfig.h" + +#define STATEFILE_PRIVATE + +#include "core/or/or.h" +#include "lib/encoding/confline.h" +#include "app/config/statefile.h" + +#include "test/test.h" + +static void +test_statefile_remove_obsolete(void *arg) +{ + (void)arg; + config_line_t *inp = NULL; + /* try empty config */ + or_state_remove_obsolete_lines(&inp); + tt_assert(!inp); + + /* try removing every line */ + config_line_append(&inp, "EntryGuard", "doesn't matter"); + config_line_append(&inp, "HidServRevCounter", "ignore"); + config_line_append(&inp, "hidservrevcounter", "foobar"); // note case + or_state_remove_obsolete_lines(&inp); + tt_assert(!inp); + + /* Now try removing a subset of lines. */ + config_line_append(&inp, "EntryGuard", "doesn't matter"); + config_line_append(&inp, "Guard", "in use"); + config_line_append(&inp, "HidServRevCounter", "ignore"); + config_line_append(&inp, "TorVersion", "this test doesn't care"); + or_state_remove_obsolete_lines(&inp); + tt_assert(inp); + tt_str_op(inp->key, OP_EQ, "Guard"); + tt_str_op(inp->value, OP_EQ, "in use"); + tt_assert(inp->next); + tt_str_op(inp->next->key, OP_EQ, "TorVersion"); + tt_str_op(inp->next->value, OP_EQ, "this test doesn't care"); + tt_assert(! inp->next->next); + + done: + config_free_lines(inp); +} + +#define T(name) \ + { #name, test_statefile_##name, 0, NULL, NULL } + +struct testcase_t statefile_tests[] = { + T(remove_obsolete), + END_OF_TESTCASES +};