Skip to content
Permalink
Browse files
wip: try detect_compression_method and log
  • Loading branch information
@juga0
juga0 committed Dec 3, 2018
1 parent 41008a7 commit b03091842bc4590e11e3ac026daae8ed6d8f7554
Showing with 24 additions and 7 deletions.
  1. +5 −2 src/feature/dircache/dircache.c
  2. +19 −5 src/test/test_dir_handle_get.c
7 src/feature/dircache/dircache.c
View file
@@ -1451,17 +1451,20 @@ handle_get_next_bandwidth(dir_connection_t *conn,
const or_options_t *options = get_options();
const compress_method_t compress_method =
find_best_compression_method(args->compression_supported, 1);
// this will log: [notice] compress_method 2
// ie, ZLIB_METHOD
log_notice(LD_DIR, "compress_method %d", compress_method);

if (options->V3BandwidthsFile) {
char *bandwidth = read_file_to_str(options->V3BandwidthsFile,
RFTS_IGNORE_MISSING, NULL);
if (bandwidth != NULL) {
size_t len = strlen(bandwidth);
ssize_t len = strlen(bandwidth);
write_http_response_header(conn, compress_method != NO_METHOD ? -1 : len,
compress_method, BANDWIDTH_CACHE_LIFETIME);
if (compress_method != NO_METHOD)
conn->compress_state = tor_compress_new(1, compress_method,
choose_compression_level(len));
choose_compression_level(len/2));
connection_buf_add(bandwidth, len, TO_CONN(conn));

This comment has been minimized.

Sign in to view
Copy link
@teor2345

teor2345 Dec 11, 2018 •
edited

Contributor

This code sets the compression state on the connection, then adds the data uncompressed.

Here's some similar code that correctly adds the data compressed:

tor/src/feature/dircache/dircache.c

Lines 1034 to 1048 in 8020d6f

write_http_response_header(conn, body_len ? body_len : -1,
compress_method,
lifetime);
if (smartlist_len(items)) {
if (compress_method != NO_METHOD) {
conn->compress_state = tor_compress_new(1, compress_method,
choose_compression_level(estimated_len));
SMARTLIST_FOREACH(items, const char *, c,
connection_buf_add_compress(c, strlen(c), conn, 0));
connection_buf_add_compress("", 0, conn, 1);
} else {
SMARTLIST_FOREACH(items, const char *, c,
connection_buf_add(c, strlen(c), TO_CONN(conn)));
}

tor/src/feature/dircache/dircache.c

Lines 1292 to 1310 in 8020d6f

write_http_response_header(conn,
compress_method != NO_METHOD ? -1 : len,
compress_method,
60*60);
if (compress_method != NO_METHOD) {
conn->compress_state = tor_compress_new(1, compress_method,
choose_compression_level(len));
SMARTLIST_FOREACH(certs, authority_cert_t *, c,
connection_buf_add_compress(
c->cache_info.signed_descriptor_body,
c->cache_info.signed_descriptor_len,
conn, 0));
connection_buf_add_compress("", 0, conn, 1);
} else {
SMARTLIST_FOREACH(certs, authority_cert_t *, c,
connection_buf_add(c->cache_info.signed_descriptor_body,
c->cache_info.signed_descriptor_len,
TO_CONN(conn)));
}

It's not great to copy code, but I opened a ticket to clean up:
https://trac.torproject.org/projects/tor/ticket/28815

This comment has been minimized.

Sign in to view
Copy link
@juga0

juga0 Dec 12, 2018

Author Contributor

Ah, i see, connection_buf_add_compress, not connection_buf_add

This comment has been minimized.

Sign in to view
Copy link
@juga0

juga0 Dec 12, 2018

Author Contributor

Also thanks for pointing out the code copied, i wondered about it.
tor_free(bandwidth);
return 0;
24 src/test/test_dir_handle_get.c
View file
@@ -1851,7 +1851,7 @@ static void
test_dir_handle_get_status_vote_current_consensus_ns(void* data)
{
char *header = NULL;
char *body = NULL, *comp_body = NULL;
char *body = NULL, *comp_body;
size_t body_used = 0, comp_body_used = 0;
char *stats = NULL, *hist = NULL;
(void) data;
@@ -2493,6 +2493,8 @@ test_dir_handle_get_status_vote_next_bandwidth(void* data)
dir_connection_t *conn = NULL;
char *header = NULL, *body = NULL;
size_t body_used = 0;
char *comp_body;
size_t comp_body_used = 0;
(void) data;

const char *content =
@@ -2544,16 +2546,28 @@ test_dir_handle_get_status_vote_next_bandwidth(void* data)
tt_int_op(0, OP_EQ, directory_handle_command_get(conn,
GET("/tor/status-vote/next/bandwidth.z"), NULL, 0));

// this will log: [debug] fetch_from_buf_http(): headerlen 163, bodylen 167.
fetch_from_buf_http(TO_CONN(conn)->outbuf, &header, MAX_HEADERS_SIZE,
&body, &body_used, strlen(content)+1, 0);
&comp_body, &comp_body_used, strlen(content)+1, 0);

tt_assert(header);
tt_ptr_op(strstr(header, "HTTP/1.0 200 OK\r\n"), OP_EQ, header);
tt_assert(strstr(header, "Content-Encoding: deflate\r\n"));
tt_assert(strstr(header, "Content-Length: 167\r\n"));
log_notice(LD_DIR, "%s", header);
// assert there is no content-lenght

// !!, if it's compressed, this should not be the case
tt_int_op(comp_body_used, OP_EQ, strlen(comp_body));

This comment has been minimized.

Sign in to view
Copy link
@teor2345

teor2345 Dec 11, 2018

Contributor

In rare cases, compression can result in data that is the same size, or even slightly larger.
If you want to test if the content is compressed, test that it is not the same as the uncompressed content.

This comment has been minimized.

Sign in to view
Copy link
@juga0

juga0 Dec 12, 2018

Author Contributor

Now, using connection_buf_add_compress, but detect_compression_method still returns NO_METHOD, and

tt_str_op(content, OP_NE, comp_body);

still fails.
So, it seems that even if find_best_compression_method returns ZLIB and the header has Content-Encoding: deflate, the data is not compressed and it's the same.
I'm not sure if i'd need to try with a bigger string.

This comment has been minimized.

Sign in to view
Copy link
@nmathewson

nmathewson Dec 12, 2018

Contributor

So one easy way to debug this would be to fetch and log the entire contents of "TO_CONN(conn)->outbuf" right before the fetch_from_buf_http. That way you'll be able to see whether it's compressing or not, and localize the failure there.

Incidentally, strlen(content)+1 is not a great upper bound for the compressed body length, since it could be larger than the original content.

Also, the compressed body, if it's really compressed, is going to be a sequence of bytes with some 0 bytes, so you can't rely on it being nul-terminated, so you can't use strcmp() or tt_str_op() on it.

This comment has been minimized.

Sign in to view
Copy link
@nmathewson

nmathewson Dec 12, 2018

Contributor

If it turns out that the data isnt' being compressed, one way to debug that would be to log the chosen compression_method from inside directory_handle_command_get.

This comment has been minimized.

Sign in to view
Copy link
@juga0

juga0 Dec 14, 2018

Author Contributor

logging that says actually that the compression_method is 5 (UNKNOWN_METHOD).
Why is happening this?, because the body is too short? (167 bytes).

This comment has been minimized.

Sign in to view
Copy link
@teor2345

teor2345 Dec 17, 2018

Contributor

Now, using connection_buf_add_compress, but detect_compression_method still returns NO_METHOD

I can't find this new code.
Please open a pull request.
tt_str_op(content, OP_EQ, comp_body);

compress_method_t compression = detect_compression_method(comp_body,
comp_body_used);
// this will log: [notice] 5
// ie, UNKNOWN_METHOD
log_notice(LD_DIR, "%d", compression);

tor_free(comp_body);

tt_int_op(body_used, OP_EQ, strlen(body));
tt_str_op(content, OP_EQ, body);

done:
UNMOCK(get_options);

0 comments on commit b030918

Please sign in to comment.