Browse Source

Gracefully handle disappearing dir_connection_t objects

Previously, if a dir_connection_t closed while the pirserver or
pirclient was computing what to send over it, tor would go ahead and
write to the now-freed dir_connection_t pointer anyway, which was
obviously bad.

Now we keep track of the dir_connection_t pointer associated with any
pending requests to the pirserver/pirclient process, and note if one of
them closes, so that we don't try to write to it afterwards.
Ian Goldberg 5 years ago
parent
commit
d5c99da4fc

+ 5 - 8
src/feature/dirclient/dirclient.c

@@ -81,9 +81,6 @@ static void dir_routerdesc_download_failed(smartlist_t *failed,
 static void dir_microdesc_download_failed(smartlist_t *failed,
 static void dir_microdesc_download_failed(smartlist_t *failed,
                                           int status_code,
                                           int status_code,
                                           const char *dir_id);
                                           const char *dir_id);
-static void directory_send_command(dir_connection_t *conn,
-                                   const int direct,
-                                   const directory_request_t *req);
 static void connection_dir_close_consensus_fetches(
 static void connection_dir_close_consensus_fetches(
                    dir_connection_t *except_this_one, const char *resource);
                    dir_connection_t *except_this_one, const char *resource);
 
 
@@ -1373,7 +1370,7 @@ directory_initiate_request,(directory_request_t *request))
         /* fall through */
         /* fall through */
       case 0:
       case 0:
         /* queue the command on the outbuf */
         /* queue the command on the outbuf */
-        directory_send_command(conn, 1, request);
+        connection_dir_client_directory_send_command(conn, 1, request);
         connection_watch_events(TO_CONN(conn), READ_EVENT | WRITE_EVENT);
         connection_watch_events(TO_CONN(conn), READ_EVENT | WRITE_EVENT);
         /* writable indicates finish, readable indicates broken link,
         /* writable indicates finish, readable indicates broken link,
            error indicates broken link in windowsland. */
            error indicates broken link in windowsland. */
@@ -1427,7 +1424,7 @@ directory_initiate_request,(directory_request_t *request))
     }
     }
     conn->base_.state = DIR_CONN_STATE_CLIENT_SENDING;
     conn->base_.state = DIR_CONN_STATE_CLIENT_SENDING;
     /* queue the command on the outbuf */
     /* queue the command on the outbuf */
-    directory_send_command(conn, 0, request);
+    connection_dir_client_directory_send_command(conn, 0, request);
 
 
     connection_watch_events(TO_CONN(conn), READ_EVENT|WRITE_EVENT);
     connection_watch_events(TO_CONN(conn), READ_EVENT|WRITE_EVENT);
     connection_start_reading(ENTRY_TO_CONN(linked_conn));
     connection_start_reading(ENTRY_TO_CONN(linked_conn));
@@ -1523,8 +1520,8 @@ copy_ipv6_address(char* destination, const char* source, size_t len,
  * <b>conn</b>-\>outbuf.  If <b>direct</b> is true, we're making a
  * <b>conn</b>-\>outbuf.  If <b>direct</b> is true, we're making a
  * non-anonymized connection to the dirport.
  * non-anonymized connection to the dirport.
  */
  */
-static void
-directory_send_command(dir_connection_t *conn,
+void
+connection_dir_client_directory_send_command(dir_connection_t *conn,
                        const int direct,
                        const int direct,
                        const directory_request_t *req)
                        const directory_request_t *req)
 {
 {
@@ -1564,7 +1561,7 @@ directory_send_command(dir_connection_t *conn,
         if (pirparams && pirparams->size > 0 && req->payload == NULL) {
         if (pirparams && pirparams->size > 0 && req->payload == NULL) {
             /* This function will asynchronously construct the PIR
             /* This function will asynchronously construct the PIR
              * request and call back to us when it's ready. */
              * request and call back to us when it's ready. */
-            hs_client_pir_create(directory_send_command, conn);
+            hs_client_pir_create(conn);
             return;
             return;
         }
         }
   }
   }

+ 3 - 0
src/feature/dirclient/dirclient.h

@@ -93,6 +93,9 @@ int router_supports_extrainfo(const char *identity_digest, int is_authority);
 void connection_dir_client_request_failed(dir_connection_t *conn);
 void connection_dir_client_request_failed(dir_connection_t *conn);
 void connection_dir_client_refetch_hsdesc_if_needed(
 void connection_dir_client_refetch_hsdesc_if_needed(
                                           dir_connection_t *dir_conn);
                                           dir_connection_t *dir_conn);
+void connection_dir_client_directory_send_command(dir_connection_t *conn,
+                                   const int direct,
+                                   const directory_request_t *req);
 
 
 typedef struct connection_dir_client_conninfo_st *connection_dir_client_conninfo_t;
 typedef struct connection_dir_client_conninfo_st *connection_dir_client_conninfo_t;
 
 

+ 5 - 0
src/feature/dircommon/dir_connection_st.h

@@ -61,8 +61,13 @@ struct dir_connection_t {
    * needs this for the incoming side, so it's moved here. */
    * needs this for the incoming side, so it's moved here. */
   uint64_t dirreq_id;
   uint64_t dirreq_id;
 
 
+  /* The ID of the PIR process request ID, created by tor */
+  uint64_t pirprocess_reqid;
+
+  /* The state of the PIR client */
   dir_connection_pirclient_state_t pirclient_state;
   dir_connection_pirclient_state_t pirclient_state;
 
 
+  /* The ID of the PIR query, created by the pirclient subprocess */
   char pirclient_queryid[8];
   char pirclient_queryid[8];
 
 
 #ifdef MEASUREMENTS_21206
 #ifdef MEASUREMENTS_21206

+ 3 - 0
src/feature/dircommon/directory.c

@@ -17,6 +17,7 @@
 
 
 #include "feature/dircommon/dir_connection_st.h"
 #include "feature/dircommon/dir_connection_st.h"
 #include "feature/nodelist/routerinfo_st.h"
 #include "feature/nodelist/routerinfo_st.h"
+#include "feature/hs/hs_pirprocess.h"
 
 
 /**
 /**
  * \file directory.c
  * \file directory.c
@@ -390,6 +391,8 @@ connection_dir_about_to_close(dir_connection_t *dir_conn)
 {
 {
   connection_t *conn = TO_CONN(dir_conn);
   connection_t *conn = TO_CONN(dir_conn);
 
 
+  hs_pirprocess_dealloc_reqid(dir_conn);
+
   if (conn->state < DIR_CONN_STATE_CLIENT_FINISHED) {
   if (conn->state < DIR_CONN_STATE_CLIENT_FINISHED) {
     /* It's a directory connection and connecting or fetching
     /* It's a directory connection and connecting or fetching
      * failed: forget about this router, and maybe try again. */
      * failed: forget about this router, and maybe try again. */

+ 15 - 17
src/feature/hs/hs_cache.c

@@ -981,27 +981,29 @@ static void
 hs_cache_pirserver_received(const unsigned char *hdrbuf,
 hs_cache_pirserver_received(const unsigned char *hdrbuf,
     const char *bodybuf, size_t bodylen)
     const char *bodybuf, size_t bodylen)
 {
 {
-    /* PIRONION TODO: as elsewhere, conn may have gone away before we
-     * get here.  In the future, hook the function that closes a
-     * dir_connection_t. */
+    dir_connection_t *conn;
+    uint64_t reqid;
     log_info(LD_DIRSERV,"PIRSERVER response header %p type %02x body len %ld",
     log_info(LD_DIRSERV,"PIRSERVER response header %p type %02x body len %ld",
             *(void**)hdrbuf, hdrbuf[8], bodylen);
             *(void**)hdrbuf, hdrbuf[8], bodylen);
+    memmove(&reqid, hdrbuf, sizeof(reqid));
+    conn = hs_pirprocess_lookup_reqid(reqid);
+    if (!conn) {
+        /* The dir_connection_t has closed between the time we received
+         * the request and now (when we are ready to send back the
+         * response). */
+        return;
+    }
     if (hdrbuf[8] == PIRSERVER_RESPONSE_PARAMS) {
     if (hdrbuf[8] == PIRSERVER_RESPONSE_PARAMS) {
         /* Send the params back to the client */
         /* Send the params back to the client */
-        dir_connection_t *conn;
-        memmove((char *)(&conn), hdrbuf, sizeof(conn));
         dircache_pirserver_reply_params(conn, bodybuf, bodylen);
         dircache_pirserver_reply_params(conn, bodybuf, bodylen);
     } else if (hdrbuf[8] == PIRSERVER_RESPONSE_LOOKUP_SUCCESS) {
     } else if (hdrbuf[8] == PIRSERVER_RESPONSE_LOOKUP_SUCCESS) {
         /* Send the PIR response back to the client */
         /* Send the PIR response back to the client */
-        dir_connection_t *conn;
-        memmove((char *)(&conn), hdrbuf, sizeof(conn));
         dircache_pirserver_reply_lookup(conn, bodybuf, bodylen);
         dircache_pirserver_reply_lookup(conn, bodybuf, bodylen);
     } else if (hdrbuf[8] == PIRSERVER_RESPONSE_LOOKUP_FAILURE) {
     } else if (hdrbuf[8] == PIRSERVER_RESPONSE_LOOKUP_FAILURE) {
         /* Send an error response back to the client */
         /* Send an error response back to the client */
-        dir_connection_t *conn;
-        memmove((char *)(&conn), hdrbuf, sizeof(conn));
         dircache_pirserver_reply_lookup(conn, NULL, 0);
         dircache_pirserver_reply_lookup(conn, NULL, 0);
     }
     }
+    hs_pirprocess_dealloc_reqid(conn);
 }
 }
 
 
 static pir_process_t pirserver;
 static pir_process_t pirserver;
@@ -1075,12 +1077,8 @@ hs_cache_pirserver_get_params(dir_connection_t *conn)
     int res;
     int res;
     unsigned char hdr[PIRPROCESS_HDR_SIZE];
     unsigned char hdr[PIRPROCESS_HDR_SIZE];
 
 
-    /* PIRONION TODO: For now, the request id is just literally the
-     * dir_connection_t pointer itself, so that when we get the
-     * response, we can write to it.  But of course it's possible that
-     * object will have disappeared between now and then.  Fix this
-     * later by hooking the callback that closes a dir_connection_t. */
-    memmove(hdr, (const char *)(&conn), sizeof(conn));
+    uint64_t reqid = hs_pirprocess_alloc_reqid(conn);
+    memmove(hdr, &reqid, sizeof(reqid));
     hdr[8] = PIRSERVER_REQUEST_PARAMS;
     hdr[8] = PIRSERVER_REQUEST_PARAMS;
     memmove(hdr+9, "\0\0\0\0", 4);
     memmove(hdr+9, "\0\0\0\0", 4);
     res = hs_cache_pirserver_send(hdr, PIRPROCESS_HDR_SIZE);
     res = hs_cache_pirserver_send(hdr, PIRPROCESS_HDR_SIZE);
@@ -1098,8 +1096,8 @@ hs_cache_pirserver_query(dir_connection_t *conn, const char *body,
     int res;
     int res;
     unsigned char hdr[PIRPROCESS_HDR_SIZE];
     unsigned char hdr[PIRPROCESS_HDR_SIZE];
 
 
-    /* PIRSERVER TODO: as hs_cache_pirserver_get_params, above */
-    memmove(hdr, (const char *)(&conn), sizeof(conn));
+    uint64_t reqid = hs_pirprocess_alloc_reqid(conn);
+    memmove(hdr, &reqid, sizeof(reqid));
     hdr[8] = PIRSERVER_REQUEST_LOOKUP;
     hdr[8] = PIRSERVER_REQUEST_LOOKUP;
     *(uint32_t*)(hdr+9) = htonl(body_len);
     *(uint32_t*)(hdr+9) = htonl(body_len);
     res = hs_cache_pirserver_send(hdr, PIRPROCESS_HDR_SIZE);
     res = hs_cache_pirserver_send(hdr, PIRPROCESS_HDR_SIZE);

+ 26 - 34
src/feature/hs/hs_client.c

@@ -1966,11 +1966,6 @@ pirparams_clear(void)
     digestmap_free(hsdir_pir_params, pirparams_free);
     digestmap_free(hsdir_pir_params, pirparams_free);
 }
 }
 
 
-typedef struct {
-    void (*callback)(dir_connection_t *, int, const directory_request_t *);
-    dir_connection_t *conn;
-} HSClientPIRCallback;
-
 static pir_process_t pirclient;
 static pir_process_t pirclient;
 
 
 #define PIRCLIENT_REQUEST_CREATE 0x41
 #define PIRCLIENT_REQUEST_CREATE 0x41
@@ -1993,32 +1988,34 @@ hs_client_pirclient_received(const unsigned char *hdrbuf,
     log_info(LD_REND, "PIRCLIENT response header %p type %02x body len %ld",
     log_info(LD_REND, "PIRCLIENT response header %p type %02x body len %ld",
         (void *)hdrbuf, hdrbuf[8], bodylen);
         (void *)hdrbuf, hdrbuf[8], bodylen);
     if (hdrbuf[8] == PIRCLIENT_RESPONSE_CREATE && bodylen > 8) {
     if (hdrbuf[8] == PIRCLIENT_RESPONSE_CREATE && bodylen > 8) {
-        HSClientPIRCallback *cb;
-        memmove(&cb, hdrbuf, sizeof(cb));
-        /* PIRONION TODO: As with the server, it's possible the conn
-         * contained in this structure might have closed on us.  In the
-         * future, hook the callback that closes a dir_connection_t. */
-        const node_t *node = node_get_by_id(cb->conn->identity_digest);
-        if (!node) goto createerr;
+        dir_connection_t *conn;
+        uint64_t reqid;
+        memmove(&reqid, hdrbuf, sizeof(reqid));
+        conn = hs_pirprocess_lookup_reqid(reqid);
+        if (!conn) {
+            /* The dir_connection_t has closed between the time we
+             * started the PIR query creation and now (when we are ready
+             * to send it). */
+            return;
+        }
+        const node_t *node = node_get_by_id(conn->identity_digest);
+        if (!node) return;
         const routerstatus_t *rstat = node->rs;
         const routerstatus_t *rstat = node->rs;
-        if (!rstat) goto createerr;
+        if (!rstat) return;
         directory_request_t *req =
         directory_request_t *req =
             directory_request_new(DIR_PURPOSE_FETCH_HSDESC);
             directory_request_new(DIR_PURPOSE_FETCH_HSDESC);
         directory_request_set_routerstatus(req, rstat);
         directory_request_set_routerstatus(req, rstat);
         directory_request_set_indirection(req, DIRIND_ANONYMOUS);
         directory_request_set_indirection(req, DIRIND_ANONYMOUS);
-        directory_request_set_resource(req, cb->conn->requested_resource);
+        directory_request_set_resource(req, conn->requested_resource);
         directory_request_set_payload(req, bodybuf+8, bodylen-8);
         directory_request_set_payload(req, bodybuf+8, bodylen-8);
-        directory_request_fetch_set_hs_ident(req, cb->conn->hs_ident);
-        memmove(cb->conn->pirclient_queryid, bodybuf, 8);
+        directory_request_fetch_set_hs_ident(req, conn->hs_ident);
+        memmove(conn->pirclient_queryid, bodybuf, 8);
         log_info(LD_REND, "PIRCLIENT Refetching %s from %s",
         log_info(LD_REND, "PIRCLIENT Refetching %s from %s",
-            cb->conn->requested_resource,
+            conn->requested_resource,
             safe_str_client(routerstatus_describe(rstat)));
             safe_str_client(routerstatus_describe(rstat)));
-        (cb->callback)(cb->conn, 0, req);
+        connection_dir_client_directory_send_command(conn, 0, req);
         directory_request_free(req);
         directory_request_free(req);
-
-    createerr:
-        tor_free(cb);
-        return;
+        hs_pirprocess_dealloc_reqid(conn);
     } else if (hdrbuf[8] == PIRCLIENT_RESPONSE_EXTRACT) {
     } else if (hdrbuf[8] == PIRCLIENT_RESPONSE_EXTRACT) {
         connection_dir_client_conninfo_t conninfo;
         connection_dir_client_conninfo_t conninfo;
         memmove(&conninfo, hdrbuf, sizeof(conninfo));
         memmove(&conninfo, hdrbuf, sizeof(conninfo));
@@ -2047,27 +2044,22 @@ hs_client_pirclient_send(const unsigned char *buf, size_t len)
 }
 }
 
 
 /* Initiate the creation of a PIR request by the pirclient process.
 /* Initiate the creation of a PIR request by the pirclient process.
- * When it is done, call back the callback function with the given
- * parameters, and a new req having req->payload filled in with the
- * PIR request. */
+ * When it is done, call connection_dir_client_directory_send_command
+ * with the given conn, and a new req having req->payload filled in with
+ * the PIR request. */
 void
 void
-hs_client_pir_create(
-    void (*callback)(dir_connection_t *, int, const directory_request_t *),
-    dir_connection_t *conn)
+hs_client_pir_create(dir_connection_t *conn)
 {
 {
     HSClientPIRParams *pirparams =
     HSClientPIRParams *pirparams =
         hs_client_pirparams_lookup(conn->identity_digest);
         hs_client_pirparams_lookup(conn->identity_digest);
-    HSClientPIRCallback *cb;
     unsigned char hdr[PIRPROCESS_HDR_SIZE];
     unsigned char hdr[PIRPROCESS_HDR_SIZE];
     size_t bodylen;
     size_t bodylen;
     char resource[32];
     char resource[32];
+    uint64_t reqid;
 
 
     if (pirparams == NULL) return;
     if (pirparams == NULL) return;
-    cb = tor_malloc_zero(sizeof(HSClientPIRCallback));
-    if (cb == NULL) return;
-    cb->callback = callback;
-    cb->conn = conn;
-    memmove(hdr, (const char *)(&cb), sizeof(cb));
+    reqid = hs_pirprocess_alloc_reqid(conn);
+    memmove(hdr, &reqid, sizeof(reqid));
     hdr[8] = PIRCLIENT_REQUEST_CREATE;
     hdr[8] = PIRCLIENT_REQUEST_CREATE;
     bodylen = 32 + pirparams->size;
     bodylen = 32 + pirparams->size;
     *(uint32_t*)(hdr+9) = htonl(bodylen);
     *(uint32_t*)(hdr+9) = htonl(bodylen);

+ 1 - 3
src/feature/hs/hs_client.h

@@ -95,9 +95,7 @@ void hs_client_pir_init(void);
 void hs_client_pirparams_insert(const char *digest, size_t size,
 void hs_client_pirparams_insert(const char *digest, size_t size,
     const char *params);
     const char *params);
 HSClientPIRParams* hs_client_pirparams_lookup(const char *digest);
 HSClientPIRParams* hs_client_pirparams_lookup(const char *digest);
-void hs_client_pir_create(
-    void (*callback)(dir_connection_t *, int, const directory_request_t *),
-    dir_connection_t *conn);
+void hs_client_pir_create(dir_connection_t *conn);
 void hs_client_pir_extract(connection_dir_client_conninfo_t conninfo,
 void hs_client_pir_extract(connection_dir_client_conninfo_t conninfo,
     const char queryid[8], const char *body, size_t body_len);
     const char queryid[8], const char *body, size_t body_len);
 
 

+ 3 - 0
src/feature/hs/hs_common.c

@@ -23,6 +23,7 @@
 #include "feature/hs/hs_common.h"
 #include "feature/hs/hs_common.h"
 #include "feature/hs/hs_ident.h"
 #include "feature/hs/hs_ident.h"
 #include "feature/hs/hs_service.h"
 #include "feature/hs/hs_service.h"
+#include "feature/hs/hs_pirprocess.h"
 #include "feature/hs_common/shared_random_client.h"
 #include "feature/hs_common/shared_random_client.h"
 #include "feature/nodelist/describe.h"
 #include "feature/nodelist/describe.h"
 #include "feature/nodelist/networkstatus.h"
 #include "feature/nodelist/networkstatus.h"
@@ -1781,6 +1782,7 @@ hs_init(void)
   hs_circuitmap_init();
   hs_circuitmap_init();
   hs_service_init();
   hs_service_init();
   hs_cache_init();
   hs_cache_init();
+  hs_pirprocess_init();
   hs_client_pir_init();
   hs_client_pir_init();
 }
 }
 
 
@@ -1792,6 +1794,7 @@ hs_free_all(void)
   hs_circuitmap_free_all();
   hs_circuitmap_free_all();
   hs_service_free_all();
   hs_service_free_all();
   hs_cache_free_all();
   hs_cache_free_all();
+  hs_pirprocess_free_all();
   hs_client_free_all();
   hs_client_free_all();
 }
 }
 
 

+ 68 - 0
src/feature/hs/hs_pirprocess.c

@@ -11,6 +11,7 @@
 #include "lib/process/env.h"
 #include "lib/process/env.h"
 #include "lib/process/subprocess.h"
 #include "lib/process/subprocess.h"
 #include "lib/evloop/compat_libevent.h"
 #include "lib/evloop/compat_libevent.h"
+#include "feature/dircommon/dir_connection_st.h"
 
 
 #include <event2/event.h>
 #include <event2/event.h>
 
 
@@ -313,3 +314,70 @@ hs_pirprocess_send(pir_process_t handle, const unsigned char *buf,
     }
     }
     return len;
     return len;
 }
 }
+
+/* What we really want is a map from uint64_t to dir_connection_t*, but
+ * we've got an implementation of char[20] to void*, which we'll
+ * repurpose for this use.  Note that the dir_connection_t* pointers in
+ * this map are *not* owned by this map, and this map must not free
+ * them. */
+static digestmap_t *pirprocess_reqid_map;
+
+/* The last-used request id */
+static uint64_t pirprocess_last_reqid;
+
+void
+hs_pirprocess_init(void)
+{
+  pirprocess_reqid_map = digestmap_new();
+  pirprocess_last_reqid = 0;
+}
+
+void
+hs_pirprocess_free_all(void)
+{
+  digestmap_free(pirprocess_reqid_map, NULL);
+}
+
+uint64_t
+hs_pirprocess_alloc_reqid(dir_connection_t *dir_conn)
+{
+    char mapbuf[DIGEST_LEN];
+    uint64_t reqid = dir_conn->pirprocess_reqid;
+    if (reqid > 0) {
+        return reqid;
+    }
+    ++pirprocess_last_reqid;
+    if (pirprocess_last_reqid == 0) {
+        /* We wrapped a 64-bit integer?! */
+        pirprocess_last_reqid = 1;
+    }
+    reqid = pirprocess_last_reqid;
+    memset(mapbuf, 0, DIGEST_LEN);
+    memmove(mapbuf, &reqid, sizeof(reqid));
+    digestmap_set(pirprocess_reqid_map, mapbuf, dir_conn);
+    dir_conn->pirprocess_reqid = reqid;
+    return dir_conn->pirprocess_reqid;
+}
+
+void
+hs_pirprocess_dealloc_reqid(dir_connection_t *dir_conn)
+{
+    char mapbuf[DIGEST_LEN];
+    uint64_t reqid = dir_conn->pirprocess_reqid;
+    if (reqid == 0) return;
+    memset(mapbuf, 0, DIGEST_LEN);
+    memmove(mapbuf, &reqid, sizeof(reqid));
+    digestmap_remove(pirprocess_reqid_map, mapbuf);
+    dir_conn->pirprocess_reqid = 0;
+}
+
+dir_connection_t *
+hs_pirprocess_lookup_reqid(uint64_t reqid)
+{
+    char mapbuf[DIGEST_LEN];
+    dir_connection_t *dir_conn;
+    memset(mapbuf, 0, DIGEST_LEN);
+    memmove(mapbuf, &reqid, sizeof(reqid));
+    dir_conn = digestmap_get(pirprocess_reqid_map, mapbuf);
+    return dir_conn;
+}

+ 6 - 0
src/feature/hs/hs_pirprocess.h

@@ -23,4 +23,10 @@ void hs_pirprocess_close(pir_process_t *handlep);
 int hs_pirprocess_send(pir_process_t handle, const unsigned char *buf,
 int hs_pirprocess_send(pir_process_t handle, const unsigned char *buf,
     size_t len);
     size_t len);
 
 
+void hs_pirprocess_init(void);
+void hs_pirprocess_free_all(void);
+uint64_t hs_pirprocess_alloc_reqid(dir_connection_t *dir_conn);
+void hs_pirprocess_dealloc_reqid(dir_connection_t *dir_conn);
+dir_connection_t *hs_pirprocess_lookup_reqid(uint64_t reqid);
+
 #endif /* !defined(TOR_HS_PIRPROCESS_H) */
 #endif /* !defined(TOR_HS_PIRPROCESS_H) */