From 7511fbf993271312875d0648166694cc58117060 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 18 May 2004 15:35:21 +0000 Subject: [PATCH] Resolve some XXXs svn:r1889 --- src/common/tortls.c | 8 ++++++-- src/common/util.c | 23 +++++++++++++++++++++- src/common/util.h | 8 +++++--- src/or/buffers.c | 46 +++++++++++++++----------------------------- src/or/circuitlist.c | 25 +++++++++++------------- src/or/dirserv.c | 9 ++++++--- src/or/main.c | 3 --- src/or/rendcommon.c | 2 +- src/or/rendservice.c | 2 +- src/or/rephist.c | 5 ++--- src/or/routerparse.c | 7 +++---- 11 files changed, 73 insertions(+), 65 deletions(-) diff --git a/src/common/tortls.c b/src/common/tortls.c index 8dc160702c..cfca993caf 100644 --- a/src/common/tortls.c +++ b/src/common/tortls.c @@ -375,7 +375,10 @@ tor_tls_context_new(crypto_pk_env_t *identity, SSL_CTX_free(result->ctx); if (result) free(result); - /* leak certs XXXX ? */ + if (cert) + X509_free(cert); + if (idcert) + X509_free(cert); return -1; } @@ -641,7 +644,8 @@ tor_tls_verify(tor_tls *tls, crypto_pk_env_t *identity_key) if (id_pkey) EVP_PKEY_free(id_pkey); - /* XXXX This should never get invoked, but let's make sure for now. */ + /* This should never get invoked, but let's make sure in case OpenSSL + * acts unexpectedly. */ tls_log_errors(LOG_WARN, "finishing tor_tls_verify"); return r; diff --git a/src/common/util.c b/src/common/util.c index d41b0e2728..727af645b0 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -1323,7 +1323,28 @@ write_str_to_file(const char *fname, const char *str) return -1; } fclose(file); - /* XXXX This won't work on windows: you can't use rename to replace a file.*/ + +#ifdef MS_WINDOWS + /* On Windows, rename doesn't replace. We could call ReplaceFile, but + * that's hard, and we can probably sneak by without atomicity. */ + switch (file_status(fname)) { + case FN_ERROR: + log(LOG_WARN, "Error replacing %s: %s", fname, strerror(errno)); + return -1; + case FN_DIR: + log(LOG_WARN, "Error replacing %s: is directory", fname); + return -1; + case FN_FILE: + if (unlink(fname)) { + log(LOG_WARN, "Error replacing %s while removing old copy: %s", + fname, strerror(errno)); + return -1; + } + break; + case FN_NOENT: + ; + } +#endif if (rename(tempname, fname)) { log(LOG_WARN, "Error replacing %s: %s", fname, strerror(errno)); return -1; diff --git a/src/common/util.h b/src/common/util.h index 04811abfa1..6f707ca729 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -77,10 +77,12 @@ #define tor_close_socket(s) close(s) #endif - -/* XXXX This isn't so on windows. */ /** Legal characters in a filename */ -#define CONFIG_LEGAL_FILENAME_CHARACTERS "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_/" +#ifdef MS_WINDOWS +#define CONFIG_LEGAL_FILENAME_CHARACTERS "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_/\\ " +#else +#define CONFIG_LEGAL_FILENAME_CHARACTERS "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_/ " +#endif size_t strlcat(char *dst, const char *src, size_t siz); size_t strlcpy(char *dst, const char *src, size_t siz); diff --git a/src/or/buffers.c b/src/or/buffers.c index b15e43028e..8acab16a6e 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -91,29 +91,13 @@ static INLINE void buf_remove_from_front(buf_t *buf, size_t n) { buf_shrink_if_underfull(buf); } -/** Find the first instance of the str_len byte string str on the - * buf_len byte string bufstr. Strings are not necessary - * NUL-terminated. If none exists, return -1. Otherwise, return index - * of the first character in bufstr _after_ the first instance of str. - */ -/* XXXX The way this function is used, we could always get away with - * XXXX assuming that str is NUL terminated, and use strstr instead. */ -static int find_mem_in_mem(const char *str, int str_len, - const char *bufstr, int buf_len) +/** Make sure that the memory in buf ends with a zero byte. */ +static INLINE int buf_nul_terminate(buf_t *buf) { - const char *location; - const char *last_possible = bufstr + buf_len - str_len; - - tor_assert(str && str_len > 0 && bufstr); - - if(buf_len < str_len) + if (buf_ensure_capacity(buf,buf->len+1)<0) return -1; - - for(location = bufstr; location <= last_possible; location++) - if((*location == *str) && !memcmp(location+1, str+1, str_len-1)) - return location-bufstr+str_len; - - return -1; + buf->mem[buf->len] = '\0'; + return 0; } /** Create and return a new buf with capacity size. @@ -366,19 +350,22 @@ int fetch_from_buf(char *string, size_t string_len, buf_t *buf) { int fetch_from_buf_http(buf_t *buf, char **headers_out, int max_headerlen, char **body_out, int *body_used, int max_bodylen) { - char *headers, *body; - int i; + char *headers, *body, *p; int headerlen, bodylen, contentlen; assert_buf_ok(buf); headers = buf->mem; - i = find_mem_in_mem("\r\n\r\n", 4, buf->mem, buf->datalen); - if(i < 0) { + if (buf_nul_terminate(buf)<0) { + log_fn(LOG_WARN,"Couldn't nul-terminate buffer"); + return -1; + } + body = strstr(headers,"\r\n\r\n"); + if (!body) { log_fn(LOG_DEBUG,"headers not all here yet."); return 0; } - body = buf->mem+i; + body += 4; /* Skip the the CRLFCRLF */ headerlen = body-headers; /* includes the CRLFCRLF */ bodylen = buf->datalen - headerlen; log_fn(LOG_DEBUG,"headerlen %d, bodylen %d.", headerlen, bodylen); @@ -393,10 +380,9 @@ int fetch_from_buf_http(buf_t *buf, } #define CONTENT_LENGTH "\r\nContent-Length: " - i = find_mem_in_mem(CONTENT_LENGTH, strlen(CONTENT_LENGTH), - headers, headerlen); - if(i > 0) { - contentlen = atoi(headers+i); + p = strstr(headers, CONTENT_LENGTH); + if (p) { + contentlen = atoi(p+strlen(CONTENT_LENGTH)); /* if content-length is malformed, then our body length is 0. fine. */ log_fn(LOG_DEBUG,"Got a contentlen of %d.",contentlen); if(bodylen < contentlen) { diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 81926e8e36..4f611b2196 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -419,25 +419,23 @@ void assert_cpath_layer_ok(const crypt_path_t *cp) * correct. Trigger an assert if anything is invalid. */ static void - assert_cpath_ok(const crypt_path_t *cp) +assert_cpath_ok(const crypt_path_t *cp) { - while(cp->prev) - cp = cp->prev; - //XXX this is broken. cp is a doubly linked list. + const crypt_path_t *start = cp; - while(cp->next) { + do { assert_cpath_layer_ok(cp); /* layers must be in sequence of: "open* awaiting? closed*" */ - if (cp->prev) { - if (cp->prev->state == CPATH_STATE_OPEN) { - tor_assert(cp->state == CPATH_STATE_CLOSED || - cp->state == CPATH_STATE_AWAITING_KEYS); - } else { - tor_assert(cp->state == CPATH_STATE_CLOSED); + if (cp != start) { + if (cp->state == CPATH_STATE_AWAITING_KEYS) { + tor_assert(cp->prev->state == CPATH_STATE_OPEN); + } else if (cp->state == CPATH_STATE_OPEN) { + tor_assert(cp->prev->state == CPATH_STATE_OPEN); } } cp = cp->next; - } + tor_assert(cp); + } while (cp != start); } /** Verify that circuit c has all of its invariants @@ -479,8 +477,7 @@ void assert_circuit_ok(const circuit_t *c) } } if (c->cpath) { -// assert_cpath_ok(c->cpath); -// XXX the above call causes an infinite loop. + assert_cpath_ok(c->cpath); } if (c->purpose == CIRCUIT_PURPOSE_REND_ESTABLISHED) { if (!c->marked_for_close) { diff --git a/src/or/dirserv.c b/src/or/dirserv.c index c4b76406aa..fc21a3ebf7 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -331,9 +331,12 @@ dirserv_add_descriptor(const char **desc) free_descriptor_entry(*desc_ent_ptr); } else { /* Add this at the end. */ - log_fn(LOG_INFO,"Dirserv adding desc for nickname %s",ri->nickname); - desc_ent_ptr = &descriptor_list[n_descriptors++]; - /* XXX check if n_descriptors is too big */ + if (n_descriptors >= MAX_ROUTERS_IN_DIR) { + log_fn(LOG_WARN,"Too many descriptors in directory; can't add another."); + } else { + log_fn(LOG_INFO,"Dirserv adding desc for nickname %s",ri->nickname); + desc_ent_ptr = &descriptor_list[n_descriptors++]; + } } (*desc_ent_ptr) = tor_malloc(sizeof(descriptor_entry_t)); diff --git a/src/or/main.c b/src/or/main.c index 20da0c1a3f..c494908aa8 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -215,8 +215,6 @@ static void conn_read(int i) { connection_handle_read(conn) < 0) { if (!conn->marked_for_close) { /* this connection is broken. remove it */ - /* XXX This shouldn't ever happen anymore. */ - /* XXX but it'll clearly happen on MS_WINDOWS from POLLERR, right? */ log_fn(LOG_ERR,"Unhandled error on read for %s connection (fd %d); removing", CONN_TYPE_TO_STRING(conn->type), conn->s); connection_mark_for_close(conn); @@ -289,7 +287,6 @@ static void conn_close_if_marked(int i) { if(connection_speaks_cells(conn)) { if(conn->state == OR_CONN_STATE_OPEN) { retval = flush_buf_tls(conn->tls, conn->outbuf, &conn->outbuf_flushlen); - /* XXX actually, some non-zero results are maybe ok. which ones? */ } else retval = -1; /* never flush non-open broken tls connections */ } else { diff --git a/src/or/rendcommon.c b/src/or/rendcommon.c index 3260b9ff9a..38e8d7b013 100644 --- a/src/or/rendcommon.c +++ b/src/or/rendcommon.c @@ -44,7 +44,7 @@ rend_encode_service_descriptor(rend_service_descriptor_t *desc, char *buf, *cp, *ipoint; int i, keylen, asn1len; keylen = crypto_pk_keysize(desc->pk); - buf = tor_malloc(keylen*2); /* XXXX */ + buf = tor_malloc(keylen*2); /* Too long, but that's okay. */ asn1len = crypto_pk_asn1_encode(desc->pk, buf, keylen*2); if (asn1len<0) { tor_free(buf); diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 849a79e349..b5e86a4d89 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -482,7 +482,7 @@ rend_service_relaunch_rendezvous(circuit_t *oldcirc) circuit_t *newcirc; cpath_build_state_t *newstate, *oldstate; - /* XXXX assert type and build_state */ + tor_assert(oldcirc->purpose == CIRCUIT_PURPOSE_S_CONNECT_REND); if (!oldcirc->build_state || oldcirc->build_state->failure_count > MAX_REND_FAILURES) { diff --git a/src/or/rephist.c b/src/or/rephist.c index df737a65a6..cd71eabc4c 100644 --- a/src/or/rephist.c +++ b/src/or/rephist.c @@ -154,10 +154,9 @@ void rep_hist_note_connection_died(const char* nickname, time_t when) { or_history_t *hist; if(!nickname) { - /* XXX - * If conn has no nickname, it's either an OP, or it is an OR + /* If conn has no nickname, it's either an OP, or it is an OR * which didn't complete its handshake (or did and was unapproved). - * Ignore it. Is there anything better we could do? + * Ignore it. */ return; } diff --git a/src/or/routerparse.c b/src/or/routerparse.c index 48bb723885..ef6c601ec4 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -8,10 +8,9 @@ * \brief Code to parse and validate router descriptors and directories. **/ -#define _GNU_SOURCE -/* XXX this is required on rh7 to make strptime not complain. how bad - * is this for portability? +/* This is required on rh7 to make strptime not complain. */ +#define _GNU_SOURCE #include "or.h" @@ -32,7 +31,7 @@ typedef enum { K_SIGNED_DIRECTORY, K_SIGNING_KEY, K_ONION_KEY, - K_LINK_KEY, /* XXXX obsolete */ + K_LINK_KEY, /* XXXX obsolete; remove in June. */ K_ROUTER_SIGNATURE, K_PUBLISHED, K_RUNNING_ROUTERS,