commit 482c4972b996fc7b1a3a8cc13f93c8ecc8748590 Author: David Goulet dgoulet@torproject.org Date: Tue May 21 15:19:30 2019 -0400
sendme: Clarify how sendme_circuit_cell_is_next() works
Commit 4ef8470fa5480d3b was actually reverted before because in the end we needed to do this minus 1 check on the window.
This commit clarifies that in the code, takes the useful comment changes from 4ef8470fa5480d3b and makes sendme_circuit_cell_is_next() private since it behaves in a very specific way that one external caller might expect.
Part of #30428.
Signed-off-by: David Goulet dgoulet@torproject.org --- src/core/or/circuit_st.h | 9 ++++----- src/core/or/sendme.c | 30 ++++++++++++++++++++++-------- src/core/or/sendme.h | 3 --- 3 files changed, 26 insertions(+), 16 deletions(-)
diff --git a/src/core/or/circuit_st.h b/src/core/or/circuit_st.h index a68547ecb..499bf93d6 100644 --- a/src/core/or/circuit_st.h +++ b/src/core/or/circuit_st.h @@ -115,12 +115,11 @@ struct circuit_t { * list can not contain more than 10 digests of DIGEST_LEN bytes (20). * * At position i in the list, the digest corresponds to the - * ((CIRCWINDOW_INCREMENT * i) - 1)-nth cell received since we expect the - * (CIRCWINDOW_INCREMENT * i)-nth cell to be the SENDME and thus containing - * the previous cell digest. + * (CIRCWINDOW_INCREMENT * i)-nth cell received since we expect a SENDME to + * be received containing that cell digest. * - * For example, position 2 (starting at 0) means that we've received 299 - * cells and the 299th cell digest is kept at index 2. + * For example, position 2 (starting at 0) means that we've received 300 + * cells so the 300th cell digest is kept at index 2. * * At maximum, this list contains 200 bytes plus the smartlist overhead. */ smartlist_t *sendme_last_digests; diff --git a/src/core/or/sendme.c b/src/core/or/sendme.c index eba7c37cd..d914ba5e2 100644 --- a/src/core/or/sendme.c +++ b/src/core/or/sendme.c @@ -312,15 +312,29 @@ record_cell_digest_on_circ(circuit_t *circ, const uint8_t *sendme_digest) * We are able to know that because the package or deliver window value minus * one cell (the possible SENDME cell) should be a multiple of the increment * window value. */ -bool -sendme_circuit_cell_is_next(int window) +static bool +circuit_sendme_cell_is_next(int window) { - /* Is this the last cell before a SENDME? The idea is that if the package or - * deliver window reaches a multiple of the increment, after this cell, we - * should expect a SENDME. */ + /* At the start of the window, no SENDME will be expected. */ + if (window == CIRCWINDOW_START) { + return false; + } + + /* Are we at the limit of the increment and if not, we don't expect next + * cell is a SENDME. + * + * We test against the window minus 1 because when we are looking if the + * next cell is a SENDME, the window (either package or deliver) hasn't been + * decremented just yet so when this is called, we are currently processing + * the "window - 1" cell. + * + * This function is used when recording a cell digest and this is done quite + * low in the stack when decrypting or encrypting a cell. The window is only + * updated once the cell is actually put in the outbuf. */ if (((window - 1) % CIRCWINDOW_INCREMENT) != 0) { return false; } + /* Next cell is expected to be a SENDME. */ return true; } @@ -596,7 +610,7 @@ sendme_record_cell_digest_on_circ(circuit_t *circ, crypt_path_t *cpath) /* Is this the last cell before a SENDME? The idea is that if the * package_window reaches a multiple of the increment, after this cell, we * should expect a SENDME. */ - if (!sendme_circuit_cell_is_next(package_window)) { + if (!circuit_sendme_cell_is_next(package_window)) { return; }
@@ -621,7 +635,7 @@ sendme_record_received_cell_digest(circuit_t *circ, crypt_path_t *cpath) tor_assert(circ);
/* Only record if the next cell is expected to be a SENDME. */ - if (!sendme_circuit_cell_is_next(cpath ? cpath->deliver_window : + if (!circuit_sendme_cell_is_next(cpath ? cpath->deliver_window : circ->deliver_window)) { return; } @@ -644,7 +658,7 @@ sendme_record_sending_cell_digest(circuit_t *circ, crypt_path_t *cpath) tor_assert(circ);
/* Only record if the next cell is expected to be a SENDME. */ - if (!sendme_circuit_cell_is_next(cpath ? cpath->package_window: + if (!circuit_sendme_cell_is_next(cpath ? cpath->package_window : circ->package_window)) { goto end; } diff --git a/src/core/or/sendme.h b/src/core/or/sendme.h index 847fcdd67..cdbdf55ac 100644 --- a/src/core/or/sendme.h +++ b/src/core/or/sendme.h @@ -40,9 +40,6 @@ void sendme_record_cell_digest_on_circ(circuit_t *circ, crypt_path_t *cpath); void sendme_record_received_cell_digest(circuit_t *circ, crypt_path_t *cpath); void sendme_record_sending_cell_digest(circuit_t *circ, crypt_path_t *cpath);
-/* Circuit level information. */ -bool sendme_circuit_cell_is_next(int window); - /* Private section starts. */ #ifdef SENDME_PRIVATE