[tor-commits] [tor/master] sendme: Clarify how sendme_circuit_cell_is_next() works

[nickm at torproject.org]

commit 482c4972b996fc7b1a3a8cc13f93c8ecc8748590
Author: David Goulet <dgoulet at 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 at 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