April 2019 - tor-commits - lists.torproject.org

[tor/master] Even more diagnostic messages for bug 28223.
by asn＠torproject.org 18 Apr '19

18 Apr '19

commit 99b87d7ca48c5ed80aec5f24902843f72975bfea Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Apr 3 13:53:06 2019 -0400 Even more diagnostic messages for bug 28223. Try to figure out _where exactly_ we are first encountering NULs in microdescriptors, and what we are doing when that happens. --- changes/diagnostic_28223_redux | 4 ++++ src/feature/dirparse/microdesc_parse.c | 17 ++++++++++++++++- src/feature/nodelist/microdesc.c | 21 +++++++++++++++++++++ 3 files changed, 41 insertions(+), 1 deletion(-) diff --git a/changes/diagnostic_28223_redux b/changes/diagnostic_28223_redux new file mode 100644 index 000000000..0d7499832 --- /dev/null +++ b/changes/diagnostic_28223_redux @@ -0,0 +1,4 @@ + o Minor features (diagnostic): + - Add more diagnostic log messages in an attempt to solve + the issue of NUL bytes appearing in a microdescriptor cache. + Related to ticket 28223. diff --git a/src/feature/dirparse/microdesc_parse.c b/src/feature/dirparse/microdesc_parse.c index 5a75af399..1e26901ce 100644 --- a/src/feature/dirparse/microdesc_parse.c +++ b/src/feature/dirparse/microdesc_parse.c @@ -159,7 +159,22 @@ microdescs_parse_from_string(const char *s, const char *eos, if (tokenize_string(area, s, start_of_next_microdesc, tokens, microdesc_token_table, flags)) { - log_warn(LD_DIR, "Unparseable microdescriptor"); + const char *location; + switch (where) { + case SAVED_NOWHERE: + location = "download or generated string"; + break; + case SAVED_IN_CACHE: + location = "cache"; + break; + case SAVED_IN_JOURNAL: + location = "journal"; + break; + default: + location = "unknown location"; + break; + } + log_warn(LD_DIR, "Unparseable microdescriptor found in %s", location); goto next; } diff --git a/src/feature/nodelist/microdesc.c b/src/feature/nodelist/microdesc.c index dafaabb5e..af4f608d2 100644 --- a/src/feature/nodelist/microdesc.c +++ b/src/feature/nodelist/microdesc.c @@ -221,6 +221,13 @@ dump_microdescriptor(int fd, microdesc_t *md, size_t *annotation_len_out) } md->off = tor_fd_getpos(fd); + const char *nulpos = memchr(md->body, 0, md->bodylen); + if (BUG(nulpos)) { + log_warn(LD_BUG, "About to dump a NUL into a microdescriptor file. " + "offset %"PRId64", bodylen %zu, nul position %zu", + (int64_t)md->off, md->bodylen, + (size_t)(nulpos - md->body)); + } written = write_all_to_fd(fd, md->body, md->bodylen); if (written != (ssize_t)md->bodylen) { written = written < 0 ? 0 : written; @@ -480,6 +487,17 @@ microdesc_cache_clear(microdesc_cache_t *cache) cache->bytes_dropped = 0; } +static void +warn_if_nul_found(const char *inp, size_t len, const char *description) +{ + const char *nul_found = memchr(inp, 0, len); + if (BUG(nul_found)) { + log_warn(LD_BUG, "Found unexpected NUL while reading %s, at " + "position %zu/%zu.", + description, (nul_found - inp), len); + } +} + /** Reload the contents of <b>cache</b> from disk. If it is empty, load it * for the first time. Return 0 on success, -1 on failure. */ int @@ -497,6 +515,7 @@ microdesc_cache_reload(microdesc_cache_t *cache) mm = cache->cache_content = tor_mmap_file(cache->cache_fname); if (mm) { + warn_if_nul_found(mm->data, mm->size, "microdesc cache"); added = microdescs_add_to_cache(cache, mm->data, mm->data+mm->size, SAVED_IN_CACHE, 0, -1, NULL); if (added) { @@ -509,6 +528,8 @@ microdesc_cache_reload(microdesc_cache_t *cache) RFTS_IGNORE_MISSING, &st); if (journal_content) { cache->journal_len = (size_t) st.st_size; + warn_if_nul_found(journal_content, cache->journal_len, + "microdesc journal"); added = microdescs_add_to_cache(cache, journal_content, journal_content+st.st_size, SAVED_IN_JOURNAL, 0, -1, NULL);

1 0

[tor/master] scripts: In git-pull-all.sh, also fetch the latest tor-github pull requests
by asn＠torproject.org 18 Apr '19

18 Apr '19

commit 5722c6d12d75ad1ddb28c32a11aae7aed7ccf5db Author: teor <teor(a)torproject.org> Date: Wed Apr 10 19:26:47 2019 +1000 scripts: In git-pull-all.sh, also fetch the latest tor-github pull requests Implements ticket 30114. --- changes/ticket30114 | 3 +++ scripts/git/git-pull-all.sh | 18 +++++++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/changes/ticket30114 b/changes/ticket30114 new file mode 100644 index 000000000..a80f7f4dc --- /dev/null +++ b/changes/ticket30114 @@ -0,0 +1,3 @@ + o Minor features (git scripts): + - In git-pull-all.sh, also fetch the latest tor-github pull requests. + Implements ticket 30114. diff --git a/scripts/git/git-pull-all.sh b/scripts/git/git-pull-all.sh index 0a4898a11..5d1d58e4b 100755 --- a/scripts/git/git-pull-all.sh +++ b/scripts/git/git-pull-all.sh @@ -174,6 +174,19 @@ function fetch_origin fi } +# Fetch tor-github pull requests. No arguments. +function fetch_tor_github +{ + local cmd="git fetch tor-github" + printf " %s Fetching tor-github..." "$MARKER" + if [ $DRY_RUN -eq 0 ]; then + msg=$( eval "$cmd" 2>&1 ) + validate_ret $? "$msg" + else + printf "\\n %s\\n" "${IWTH}$cmd${CNRM}" + fi +} + ############### # Entry point # ############### @@ -188,8 +201,11 @@ while getopts "n" opt; do esac done -# First, fetch the origin. +# First, fetch tor-github. goto_repo "$ORIGIN_PATH" +fetch_tor_github + +# Then, fetch the origin. fetch_origin # Go over all configured worktree.

1 0

[tor/master] fdio.c: add more includes.
by asn＠torproject.org 18 Apr '19

18 Apr '19

commit 821d29e42040789c59d271ba2f1d87a54d6d1435 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Apr 3 13:53:36 2019 -0400 fdio.c: add more includes. This is just in case there is some rogue platform that uses a nonstandard value for SEEK_*, and does not define that macro in unistd.h. I think that's unlikely, but it's conceivable. --- src/lib/fdio/fdio.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/lib/fdio/fdio.c b/src/lib/fdio/fdio.c index 6c87af791..078af6a9b 100644 --- a/src/lib/fdio/fdio.c +++ b/src/lib/fdio/fdio.c @@ -17,12 +17,16 @@ #ifdef _WIN32 #include <windows.h> #endif +#ifdef HAVE_SYS_TYPES_H +#include <sys/types.h> +#endif #include "lib/fdio/fdio.h" #include "lib/cc/torint.h" #include "lib/err/torerr.h" #include <stdlib.h> +#include <stdio.h> /** @{ */ /** Some old versions of Unix didn't define constants for these values,

1 0

[tor/master] Add an assertion to pathbias_get_scale_ratio()
by asn＠torproject.org 18 Apr '19

18 Apr '19

commit 55690d05bd8856798d2e6f6b56261629847c068b Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Apr 11 18:38:59 2019 -0400 Add an assertion to pathbias_get_scale_ratio() This should please coverity, and fix CID 1415723. It didn't understand that networkstatus_get_param() always returns a value between its minimum and maximum values. --- src/feature/client/circpathbias.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/feature/client/circpathbias.c b/src/feature/client/circpathbias.c index 1743ab5a8..e6af649ba 100644 --- a/src/feature/client/circpathbias.c +++ b/src/feature/client/circpathbias.c @@ -176,6 +176,7 @@ pathbias_get_scale_threshold(const or_options_t *options) static double pathbias_get_scale_ratio(const or_options_t *options) { + (void) options; /* * The scale factor is the denominator for our scaling * of circuit counts for our path bias window. @@ -185,7 +186,8 @@ pathbias_get_scale_ratio(const or_options_t *options) */ int denominator = networkstatus_get_param(NULL, "pb_scalefactor", 2, 2, INT32_MAX); - (void) options; + tor_assert(denominator > 0); + /** * The mult factor is the numerator for our scaling * of circuit counts for our path bias window. It

1 0

[tor/master] Add an assertion to compute_weighted_bandwidths()
by asn＠torproject.org 18 Apr '19

18 Apr '19

commit 96e310911fa14b8a8fb1861d08fbda7abe61eb10 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Apr 11 18:38:59 2019 -0400 Add an assertion to compute_weighted_bandwidths() This should please coverity, and fix CID 1415722. It didn't understand that networkstatus_get_param() always returns a value between its minimum and maximum values. --- src/feature/nodelist/node_select.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/feature/nodelist/node_select.c b/src/feature/nodelist/node_select.c index e31abb247..93ddb066d 100644 --- a/src/feature/nodelist/node_select.c +++ b/src/feature/nodelist/node_select.c @@ -585,6 +585,7 @@ compute_weighted_bandwidths(const smartlist_t *sl, } weight_scale = networkstatus_get_weight_scale_param(NULL); + tor_assert(weight_scale >= 1); if (rule == WEIGHT_FOR_GUARD) { Wg = networkstatus_get_bw_weight(NULL, "Wgg", -1);

1 0

[tor/master] Add an assertion to num_ntors_per_tap().
by asn＠torproject.org 18 Apr '19

18 Apr '19

commit 66b07e7ec13feb57d696b920493590d232e62763 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Apr 11 18:38:59 2019 -0400 Add an assertion to num_ntors_per_tap(). This should please coverity, and fix CID 1415721. It didn't understand that networkstatus_get_param() always returns a value between its minimum and maximum values. --- src/feature/relay/onion_queue.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/feature/relay/onion_queue.c b/src/feature/relay/onion_queue.c index 696905cf5..c37745cf3 100644 --- a/src/feature/relay/onion_queue.c +++ b/src/feature/relay/onion_queue.c @@ -212,10 +212,12 @@ num_ntors_per_tap(void) #define MIN_NUM_NTORS_PER_TAP 1 #define MAX_NUM_NTORS_PER_TAP 100000 - return networkstatus_get_param(NULL, "NumNTorsPerTAP", - DEFAULT_NUM_NTORS_PER_TAP, - MIN_NUM_NTORS_PER_TAP, - MAX_NUM_NTORS_PER_TAP); + int result = networkstatus_get_param(NULL, "NumNTorsPerTAP", + DEFAULT_NUM_NTORS_PER_TAP, + MIN_NUM_NTORS_PER_TAP, + MAX_NUM_NTORS_PER_TAP); + tor_assert(result > 0); + return result; } /** Choose which onion queue we'll pull from next. If one is empty choose

1 0

[tor/master] Remove an extraneous _ from __COVERITY__
by asn＠torproject.org 18 Apr '19

18 Apr '19

commit 48a574604bef9a4fe77d9bb43dfbfc50328e13e0 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Apr 11 18:56:02 2019 -0400 Remove an extraneous _ from __COVERITY__ We had a typo in this check, so that coverity wasn't taking the right path. Bug not in any released Tor. --- src/lib/math/prob_distr.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/math/prob_distr.h b/src/lib/math/prob_distr.h index 2eb935e4a..8fccf8d01 100644 --- a/src/lib/math/prob_distr.h +++ b/src/lib/math/prob_distr.h @@ -53,7 +53,7 @@ struct dist { * We define this conditionally to suppress false positives from * Coverity, which gets confused by the sizeof business. */ -#ifdef __COVERITY___ +#ifdef __COVERITY__ #define TYPE_CHECK_OBJ(OPS, OBJ, TYPE) 0 #else #define TYPE_CHECK_OBJ(OPS, OBJ, TYPE) \

1 0

[tor/master] Changes file and practracker updates for 30149.
by asn＠torproject.org 18 Apr '19

18 Apr '19

commit 73323460023807259d3cd1dd52d33749ffe53886 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Apr 11 18:58:44 2019 -0400 Changes file and practracker updates for 30149. --- changes/ticket30149 | 3 +++ scripts/maint/practracker/exceptions.txt | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/changes/ticket30149 b/changes/ticket30149 new file mode 100644 index 000000000..a21687ac2 --- /dev/null +++ b/changes/ticket30149 @@ -0,0 +1,3 @@ + o Code simplification and refactoring: + - Add several assertions in an attempt to fix some Coverity warnings. + Closes ticket 30149. diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt index 7d03bf27d..ecc58012c 100644 --- a/scripts/maint/practracker/exceptions.txt +++ b/scripts/maint/practracker/exceptions.txt @@ -218,7 +218,7 @@ problem include-count /src/feature/nodelist/networkstatus.c 61 problem function-size /src/feature/nodelist/networkstatus.c:networkstatus_check_consensus_signature() 176 problem function-size /src/feature/nodelist/networkstatus.c:networkstatus_set_current_consensus() 293 problem function-size /src/feature/nodelist/node_select.c:router_pick_directory_server_impl() 123 -problem function-size /src/feature/nodelist/node_select.c:compute_weighted_bandwidths() 205 +problem function-size /src/feature/nodelist/node_select.c:compute_weighted_bandwidths() 206 problem function-size /src/feature/nodelist/node_select.c:router_pick_trusteddirserver_impl() 114 problem function-size /src/feature/nodelist/nodelist.c:compute_frac_paths_available() 193 problem file-size /src/feature/nodelist/routerlist.c 3234

1 0

[tor/master] fixup! Even more diagnostic messages for bug 28223.
by asn＠torproject.org 18 Apr '19

18 Apr '19

commit 0c42ddf28c330280f66feb7cc92b63f3998468e1 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Apr 15 15:21:18 2019 -0400 fixup! Even more diagnostic messages for bug 28223. Use TOR_PRIuSZ in place of %zu. --- src/feature/nodelist/microdesc.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/feature/nodelist/microdesc.c b/src/feature/nodelist/microdesc.c index af4f608d2..eaa7f0caf 100644 --- a/src/feature/nodelist/microdesc.c +++ b/src/feature/nodelist/microdesc.c @@ -224,7 +224,8 @@ dump_microdescriptor(int fd, microdesc_t *md, size_t *annotation_len_out) const char *nulpos = memchr(md->body, 0, md->bodylen); if (BUG(nulpos)) { log_warn(LD_BUG, "About to dump a NUL into a microdescriptor file. " - "offset %"PRId64", bodylen %zu, nul position %zu", + "offset %"PRId64", bodylen %"TOR_PRIuSZ", " + "nul position %"TOR_PRIuSZ".", (int64_t)md->off, md->bodylen, (size_t)(nulpos - md->body)); } @@ -493,7 +494,7 @@ warn_if_nul_found(const char *inp, size_t len, const char *description) const char *nul_found = memchr(inp, 0, len); if (BUG(nul_found)) { log_warn(LD_BUG, "Found unexpected NUL while reading %s, at " - "position %zu/%zu.", + "position %"TOR_PRIuSZ"/%"TOR_PRIuSZ".", description, (nul_found - inp), len); } }

1 0

[tor/master] In warn_if_nul_found, log surrounding context.
by asn＠torproject.org 18 Apr '19

18 Apr '19

commit 950d890f77d8f060702b100e6075d5083d536577 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Apr 15 15:33:09 2019 -0400 In warn_if_nul_found, log surrounding context. We need to encode here instead of doing escaped(), since fwict escaped() does not currently handle NUL bytes. Also, use warn_if_nul_found in more cases to avoid duplication. --- src/feature/nodelist/microdesc.c | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/src/feature/nodelist/microdesc.c b/src/feature/nodelist/microdesc.c index eaa7f0caf..f4fa8eeae 100644 --- a/src/feature/nodelist/microdesc.c +++ b/src/feature/nodelist/microdesc.c @@ -69,6 +69,8 @@ struct microdesc_cache_t { }; static microdesc_cache_t *get_microdesc_cache_noload(void); +static void warn_if_nul_found(const char *inp, size_t len, int64_t offset, + const char *activity); /** Helper: computes a hash of <b>md</b> to place it in a hash table. */ static inline unsigned int @@ -221,14 +223,8 @@ dump_microdescriptor(int fd, microdesc_t *md, size_t *annotation_len_out) } md->off = tor_fd_getpos(fd); - const char *nulpos = memchr(md->body, 0, md->bodylen); - if (BUG(nulpos)) { - log_warn(LD_BUG, "About to dump a NUL into a microdescriptor file. " - "offset %"PRId64", bodylen %"TOR_PRIuSZ", " - "nul position %"TOR_PRIuSZ".", - (int64_t)md->off, md->bodylen, - (size_t)(nulpos - md->body)); - } + warn_if_nul_found(md->body, md->bodylen, (int64_t) md->off, + "dumping a microdescriptor"); written = write_all_to_fd(fd, md->body, md->bodylen); if (written != (ssize_t)md->bodylen) { written = written < 0 ? 0 : written; @@ -489,13 +485,23 @@ microdesc_cache_clear(microdesc_cache_t *cache) } static void -warn_if_nul_found(const char *inp, size_t len, const char *description) +warn_if_nul_found(const char *inp, size_t len, int64_t offset, + const char *activity) { const char *nul_found = memchr(inp, 0, len); if (BUG(nul_found)) { - log_warn(LD_BUG, "Found unexpected NUL while reading %s, at " - "position %"TOR_PRIuSZ"/%"TOR_PRIuSZ".", - description, (nul_found - inp), len); + log_warn(LD_BUG, "Found unexpected NUL while %s, offset %"PRId64 + "at position %"TOR_PRIuSZ"/%"TOR_PRIuSZ".", + activity, offset, (nul_found - inp), len); + const char *start_excerpt_at, *eos = inp + len; + if ((nul_found - inp) >= 16) + start_excerpt_at = nul_found - 16; + else + start_excerpt_at = inp; + size_t excerpt_len = MIN(32, eos - start_excerpt_at); + char tmp[65]; + base16_encode(tmp, sizeof(tmp), start_excerpt_at, excerpt_len); + log_warn(LD_BUG, " surrounding string: %s", tmp); } } @@ -516,7 +522,7 @@ microdesc_cache_reload(microdesc_cache_t *cache) mm = cache->cache_content = tor_mmap_file(cache->cache_fname); if (mm) { - warn_if_nul_found(mm->data, mm->size, "microdesc cache"); + warn_if_nul_found(mm->data, mm->size, 0, "scanning microdesc cache"); added = microdescs_add_to_cache(cache, mm->data, mm->data+mm->size, SAVED_IN_CACHE, 0, -1, NULL); if (added) { @@ -529,8 +535,8 @@ microdesc_cache_reload(microdesc_cache_t *cache) RFTS_IGNORE_MISSING, &st); if (journal_content) { cache->journal_len = (size_t) st.st_size; - warn_if_nul_found(journal_content, cache->journal_len, - "microdesc journal"); + warn_if_nul_found(journal_content, cache->journal_len, 0, + "reading microdesc journal"); added = microdescs_add_to_cache(cache, journal_content, journal_content+st.st_size, SAVED_IN_JOURNAL, 0, -1, NULL);

1 0