From 8e1b37a4aa9d6277637f82100ac1823a6a193c63 Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 16 Jun 2017 09:38:18 +1000 Subject: [PATCH 1/6] Check if tor_compress_new() returns NULL in tor_compress_impl() Partial fix to 22626. --- src/common/compress.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/common/compress.c b/src/common/compress.c index 6513029f9c..92b64d1e91 100644 --- a/src/common/compress.c +++ b/src/common/compress.c @@ -128,6 +128,11 @@ tor_compress_impl(int compress, // inputs. tor_compress_free(stream); stream = tor_compress_new(compress, method, compression_level); + if (stream == NULL) { + log_warn(LD_GENERAL, "NULL stream while %scompressing", + compress?"":"de"); + goto err; + } } break; case TOR_COMPRESS_OK: From 952c9073ad255442adcf4e85f7b2150c3bbe91de Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 16 Jun 2017 09:41:29 +1000 Subject: [PATCH 2/6] Check for trailing input garbage in tor_compress_impl() when decompressing Fixes #22629. --- src/common/compress.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/common/compress.c b/src/common/compress.c index 92b64d1e91..ee9de6a768 100644 --- a/src/common/compress.c +++ b/src/common/compress.c @@ -139,7 +139,15 @@ tor_compress_impl(int compress, if (compress || complete_only) { goto err; } else { - goto done; + if (in_len != 0) { + log_fn(protocol_warn_level, LD_PROTOCOL, + "Unexpected extra input while decompressing"); + log_debug(LD_GENERAL, "method: %d level: %d at len: %zd", + method, compression_level, in_len); + goto err; + } else { + goto done; + } } break; case TOR_COMPRESS_BUFFER_FULL: { From 7605bd528e64898d8d62fd89f414185f465d9999 Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 16 Jun 2017 09:45:58 +1000 Subject: [PATCH 3/6] Move a comment to the right place in tor_zstd_compress_process Part of #22502 --- src/common/compress_zstd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/common/compress_zstd.c b/src/common/compress_zstd.c index 99d05c37bd..9143615d4d 100644 --- a/src/common/compress_zstd.c +++ b/src/common/compress_zstd.c @@ -312,6 +312,8 @@ tor_zstd_compress_process(tor_zstd_compress_state_t *state, return TOR_COMPRESS_ERROR; } + // ZSTD_flushStream returns 0 if the frame is done, or >0 if it + // is incomplete. if (retval > 0) return TOR_COMPRESS_BUFFER_FULL; } @@ -339,8 +341,6 @@ tor_zstd_compress_process(tor_zstd_compress_state_t *state, return TOR_COMPRESS_DONE; } else { - // ZSTD_flushStream returns 0 if the frame is done, or >0 if it - // is incomplete. return (retval == 0) ? TOR_COMPRESS_DONE : TOR_COMPRESS_OK; } From 617e1da63655a90dba499e8fe11263d3331aacf7 Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 16 Jun 2017 09:46:46 +1000 Subject: [PATCH 4/6] Remove a redundant conditional in tor_zstd_compress_process Part of #22502 --- src/common/compress_zstd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/common/compress_zstd.c b/src/common/compress_zstd.c index 9143615d4d..11fcf86644 100644 --- a/src/common/compress_zstd.c +++ b/src/common/compress_zstd.c @@ -321,7 +321,7 @@ tor_zstd_compress_process(tor_zstd_compress_state_t *state, if (!finish) { // We're not done with the input, so no need to flush. return TOR_COMPRESS_OK; - } else if (state->compress && finish) { + } else if (state->compress) { retval = ZSTD_endStream(state->u.compress_stream, &output); *out = (char *)output.dst + output.pos; From cbaf0c049c8a73a7bbd2e8bf574589f84f99ccd8 Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 16 Jun 2017 09:47:32 +1000 Subject: [PATCH 5/6] Return TOR_COMPRESS_BUFFER_FULL when zstd has additional input Fixes #22628. --- src/common/compress_zstd.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/common/compress_zstd.c b/src/common/compress_zstd.c index 11fcf86644..a136db48bf 100644 --- a/src/common/compress_zstd.c +++ b/src/common/compress_zstd.c @@ -340,8 +340,20 @@ tor_zstd_compress_process(tor_zstd_compress_state_t *state, return TOR_COMPRESS_BUFFER_FULL; return TOR_COMPRESS_DONE; - } else { - return (retval == 0) ? TOR_COMPRESS_DONE : TOR_COMPRESS_OK; + } else /* if (!state->compress) */ { + // ZSTD_decompressStream returns 0 if the frame is done, or >0 if it + // is incomplete. + // We check this above. + tor_assert_nonfatal(!ZSTD_isError(retval)); + // Start a new frame if this frame is done + if (retval == 0) + return TOR_COMPRESS_DONE; + // Don't check out_len, it might have some space left if the next output + // chunk is larger than the remaining space + else if (*in_len > 0) + return TOR_COMPRESS_BUFFER_FULL; + else + return TOR_COMPRESS_OK; } #else // HAVE_ZSTD. From 7d535ea9d3a0b2706392425c13feac760dfccdee Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 16 Jun 2017 09:48:18 +1000 Subject: [PATCH 6/6] Add extra logging during compression and decompression This helps diagnose failures. Part of #22502. --- src/common/compress.c | 13 +++++++++++- src/or/directory.c | 49 ++++++++++++++++++++++++++++--------------- 2 files changed, 44 insertions(+), 18 deletions(-) diff --git a/src/common/compress.c b/src/common/compress.c index ee9de6a768..e65894d9d2 100644 --- a/src/common/compress.c +++ b/src/common/compress.c @@ -102,8 +102,13 @@ tor_compress_impl(int compress, stream = tor_compress_new(compress, method, compression_level); - if (stream == NULL) + if (stream == NULL) { + log_warn(LD_GENERAL, "NULL stream while %scompressing", + compress?"":"de"); + log_debug(LD_GENERAL, "method: %d level: %d at len: %zd", + method, compression_level, in_len); return -1; + } size_t in_len_orig = in_len; size_t out_remaining, out_alloc; @@ -137,6 +142,12 @@ tor_compress_impl(int compress, break; case TOR_COMPRESS_OK: if (compress || complete_only) { + log_fn(protocol_warn_level, LD_PROTOCOL, + "Unexpected %s while %scompressing", + complete_only?"end of input":"result", + compress?"":"de"); + log_debug(LD_GENERAL, "method: %d level: %d at len: %zd", + method, compression_level, in_len); goto err; } else { if (in_len != 0) { diff --git a/src/or/directory.c b/src/or/directory.c index acd694ffee..ecd98119fe 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -2327,30 +2327,40 @@ connection_dir_client_reached_eof(dir_connection_t *conn) plausible = body_is_plausible(body, body_len, conn->base_.purpose); if (compression != NO_METHOD || !plausible) { + int severity = LOG_DEBUG; char *new_body = NULL; size_t new_len = 0; + const char *description1, *description2; + int want_to_try_both = 0; + int tried_both = 0; compress_method_t guessed = detect_compression_method(body, body_len); - if (compression == UNKNOWN_METHOD || guessed != compression) { - /* Tell the user if we don't believe what we're told about compression.*/ - const char *description1, *description2; - description1 = compression_method_get_human_name(compression); + description1 = compression_method_get_human_name(compression); - if (BUG(description1 == NULL)) - description1 = compression_method_get_human_name(UNKNOWN_METHOD); + if (BUG(description1 == NULL)) + description1 = compression_method_get_human_name(UNKNOWN_METHOD); - if (guessed == UNKNOWN_METHOD && !plausible) - description2 = "confusing binary junk"; - else - description2 = compression_method_get_human_name(guessed); + if (guessed == UNKNOWN_METHOD && !plausible) + description2 = "confusing binary junk"; + else + description2 = compression_method_get_human_name(guessed); - log_info(LD_HTTP, "HTTP body from server '%s:%d' was labeled as %s, " - "but it seems to be %s.%s", - conn->base_.address, conn->base_.port, description1, - description2, - (compression>0 && guessed>0)?" Trying both.":""); + /* Tell the user if we don't believe what we're told about compression.*/ + want_to_try_both = (compression == UNKNOWN_METHOD || + guessed != compression); + if (want_to_try_both) { + severity = LOG_INFO; } + tor_log(severity, LD_HTTP, + "HTTP body from server '%s:%d' was labeled as %s, " + "%s it seems to be %s.%s", + conn->base_.address, conn->base_.port, description1, + guessed != compression?"but":"and", + description2, + (compression>0 && guessed>0 && want_to_try_both)? + " Trying both.":""); + /* Try declared compression first if we can. * tor_compress_supports_method() also returns true for NO_METHOD. * Ensure that the server is not sending us data compressed using a @@ -2376,14 +2386,19 @@ connection_dir_client_reached_eof(dir_connection_t *conn) } if (!new_body && tor_compress_supports_method(guessed) && - compression != guessed) + compression != guessed) { tor_uncompress(&new_body, &new_len, body, body_len, guessed, !allow_partial, LOG_PROTOCOL_WARN); + tried_both = 1; + } /* If we're pretty sure that we have a compressed directory, and * we didn't manage to uncompress it, then warn and bail. */ if (!plausible && !new_body) { log_fn(LOG_PROTOCOL_WARN, LD_HTTP, - "Unable to decompress HTTP body (server '%s:%d').", + "Unable to decompress HTTP body (tried %s%s%s, server '%s:%d').", + description1, + tried_both?" and ":"", + tried_both?description2:"", conn->base_.address, conn->base_.port); rv = -1; goto done;