From 539158f2be6001a7849b7a85e053e43d7d78195a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 4 Sep 2019 10:50:09 -0400 Subject: [PATCH 1/6] config: use ptrdiff_t for all field-offset members. Previously we used int here, but it is more correct to use ptrdiff_t. (This never actually matters for our code in practice, since the structure we are managing here never exceed INT_MAX in size.) --- src/app/config/confparse.h | 2 +- src/lib/conf/conftypes.h | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/app/config/confparse.h b/src/app/config/confparse.h index b788e2cd1b..fc07996aab 100644 --- a/src/app/config/confparse.h +++ b/src/app/config/confparse.h @@ -72,7 +72,7 @@ typedef struct config_format_t { const struct_member_t *extra; /** The position of a config_suite_t pointer within the toplevel object, * or -1 if there is no such pointer. */ - int config_suite_offset; + ptrdiff_t config_suite_offset; } config_format_t; /** diff --git a/src/lib/conf/conftypes.h b/src/lib/conf/conftypes.h index fabad97d0c..abfb53b21b 100644 --- a/src/lib/conf/conftypes.h +++ b/src/lib/conf/conftypes.h @@ -33,6 +33,8 @@ #include "lib/conf/conftesting.h" #endif +#include + /** Enumeration of types which option values can take */ typedef enum config_type_t { CONFIG_TYPE_STRING = 0, /**< An arbitrary string. */ @@ -89,7 +91,7 @@ typedef struct struct_member_t { * Offset of this field within the structure. Compute this with * offsetof(structure, fieldname). **/ - int offset; + ptrdiff_t offset; } struct_member_t; /** @@ -102,7 +104,7 @@ typedef struct struct_member_t { typedef struct struct_magic_decl_t { const char *typename; uint32_t magic_val; - int magic_offset; + ptrdiff_t magic_offset; } struct_magic_decl_t; /** From dc199f40fb03588af0bccb149a564e895e11d60d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 4 Sep 2019 10:56:14 -0400 Subject: [PATCH 2/6] smartlist_pqueue: use ptrdiff_t instead of int for offsets. This is technically correct, but should not matter in practice, since we don't use this on any structs whose size exceeds INT_MAX. --- src/lib/container/smartlist.c | 10 +++++----- src/lib/container/smartlist.h | 9 +++++---- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/lib/container/smartlist.c b/src/lib/container/smartlist.c index 3ab2797d68..2b71c11287 100644 --- a/src/lib/container/smartlist.c +++ b/src/lib/container/smartlist.c @@ -678,7 +678,7 @@ smartlist_sort_pointers(smartlist_t *sl) static inline void smartlist_heapify(smartlist_t *sl, int (*compare)(const void *a, const void *b), - int idx_field_offset, + ptrdiff_t idx_field_offset, int idx) { while (1) { @@ -725,7 +725,7 @@ smartlist_heapify(smartlist_t *sl, void smartlist_pqueue_add(smartlist_t *sl, int (*compare)(const void *a, const void *b), - int idx_field_offset, + ptrdiff_t idx_field_offset, void *item) { int idx; @@ -754,7 +754,7 @@ smartlist_pqueue_add(smartlist_t *sl, void * smartlist_pqueue_pop(smartlist_t *sl, int (*compare)(const void *a, const void *b), - int idx_field_offset) + ptrdiff_t idx_field_offset) { void *top; tor_assert(sl->num_used); @@ -778,7 +778,7 @@ smartlist_pqueue_pop(smartlist_t *sl, void smartlist_pqueue_remove(smartlist_t *sl, int (*compare)(const void *a, const void *b), - int idx_field_offset, + ptrdiff_t idx_field_offset, void *item) { int idx = IDX_OF_ITEM(item); @@ -802,7 +802,7 @@ smartlist_pqueue_remove(smartlist_t *sl, void smartlist_pqueue_assert_ok(smartlist_t *sl, int (*compare)(const void *a, const void *b), - int idx_field_offset) + ptrdiff_t idx_field_offset) { int i; for (i = sl->num_used - 1; i >= 0; --i) { diff --git a/src/lib/container/smartlist.h b/src/lib/container/smartlist.h index 81b0151433..25638e4b22 100644 --- a/src/lib/container/smartlist.h +++ b/src/lib/container/smartlist.h @@ -13,6 +13,7 @@ **/ #include +#include #include "lib/smartlist_core/smartlist_core.h" #include "lib/smartlist_core/smartlist_foreach.h" @@ -72,18 +73,18 @@ int smartlist_bsearch_idx(const smartlist_t *sl, const void *key, void smartlist_pqueue_add(smartlist_t *sl, int (*compare)(const void *a, const void *b), - int idx_field_offset, + ptrdiff_t idx_field_offset, void *item); void *smartlist_pqueue_pop(smartlist_t *sl, int (*compare)(const void *a, const void *b), - int idx_field_offset); + ptrdiff_t idx_field_offset); void smartlist_pqueue_remove(smartlist_t *sl, int (*compare)(const void *a, const void *b), - int idx_field_offset, + ptrdiff_t idx_field_offset, void *item); void smartlist_pqueue_assert_ok(smartlist_t *sl, int (*compare)(const void *a, const void *b), - int idx_field_offset); + ptrdiff_t idx_field_offset); char *smartlist_join_strings(smartlist_t *sl, const char *join, int terminate, size_t *len_out) ATTR_MALLOC; From b39ee42904d9210ff3b6872d69535d32f588f907 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 4 Sep 2019 10:56:57 -0400 Subject: [PATCH 3/6] compat_compiler: fix documentation for STRUCT_VAR_P Previously we had said that off_t was a reasonable type to hold the result of offsetof(). That isn't so: ptrdiff_t is correct. --- src/lib/cc/compat_compiler.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/cc/compat_compiler.h b/src/lib/cc/compat_compiler.h index a8d1593214..92301449e8 100644 --- a/src/lib/cc/compat_compiler.h +++ b/src/lib/cc/compat_compiler.h @@ -195,7 +195,7 @@ * structure st. Example: * 
  *   struct a { int foo; int bar; } x;
- *   off_t bar_offset = offsetof(struct a, bar);
+ *   ptrdiff_t bar_offset = offsetof(struct a, bar);
  *   int *bar_p = STRUCT_VAR_P(&x, bar_offset);
  *   *bar_p = 3;
  *
From ec724fe8c8baf245a4cada85bb8b8b4832f6e725 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 4 Sep 2019 10:59:18 -0400 Subject: [PATCH 4/6] rendclient: use ptrdiff, not off_t, for offset of DH field. The off_t type is only useful for offsets on the filesystem. For in-memory offsets, use ptrdiff_t. --- src/feature/rend/rendclient.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/feature/rend/rendclient.c b/src/feature/rend/rendclient.c index 5bdd4d453e..2540066dfc 100644 --- a/src/feature/rend/rendclient.c +++ b/src/feature/rend/rendclient.c @@ -119,7 +119,7 @@ rend_client_send_introduction(origin_circuit_t *introcirc, char tmp[RELAY_PAYLOAD_SIZE]; rend_cache_entry_t *entry = NULL; crypt_path_t *cpath; - off_t dh_offset; + ptrdiff_t dh_offset; crypto_pk_t *intro_key = NULL; int status = 0; const char *onion_address; From 3aba13f77927e669d80009b2318c6d3d3d90a922 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 4 Sep 2019 11:06:15 -0400 Subject: [PATCH 5/6] buffers: use ptrdiff_t to indicate offsets. Previously we used int in some places and off_t for others. Neither is correct: ptrdiff_t is right for differences between pointers. (off_t is only for offsets and sizes on the filesystem.) --- src/lib/buf/buffers.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/lib/buf/buffers.c b/src/lib/buf/buffers.c index 88a25b8470..4d026bd37d 100644 --- a/src/lib/buf/buffers.c +++ b/src/lib/buf/buffers.c @@ -158,7 +158,7 @@ chunk_new_with_alloc_size(size_t alloc) static inline chunk_t * chunk_grow(chunk_t *chunk, size_t sz) { - off_t offset; + ptrdiff_t offset; const size_t memlen_orig = chunk->memlen; const size_t orig_alloc = CHUNK_ALLOC_SIZE(memlen_orig); const size_t new_alloc = CHUNK_ALLOC_SIZE(sz); @@ -440,7 +440,7 @@ chunk_copy(const chunk_t *in_chunk) #endif newch->next = NULL; if (in_chunk->data) { - off_t offset = in_chunk->data - in_chunk->mem; + ptrdiff_t offset = in_chunk->data - in_chunk->mem; newch->data = newch->mem + offset; } return newch; @@ -710,7 +710,8 @@ buf_move_all(buf_t *buf_out, buf_t *buf_in) /** Internal structure: represents a position in a buffer. */ typedef struct buf_pos_t { const chunk_t *chunk; /**< Which chunk are we pointing to? */ - int pos;/**< Which character inside the chunk's data are we pointing to? */ + ptrdiff_t pos;/**< Which character inside the chunk's data are we pointing + * to? */ size_t chunk_pos; /**< Total length of all previous chunks. */ } buf_pos_t; @@ -726,15 +727,15 @@ buf_pos_init(const buf_t *buf, buf_pos_t *out) /** Advance out to the first appearance of ch at the current * position of out, or later. Return -1 if no instances are found; * otherwise returns the absolute position of the character. */ -static off_t +static ptrdiff_t buf_find_pos_of_char(char ch, buf_pos_t *out) { const chunk_t *chunk; - int pos; + ptrdiff_t pos; tor_assert(out); if (out->chunk) { if (out->chunk->datalen) { - tor_assert(out->pos < (off_t)out->chunk->datalen); + tor_assert(out->pos < (ptrdiff_t)out->chunk->datalen); } else { tor_assert(out->pos == 0); } @@ -762,7 +763,7 @@ buf_pos_inc(buf_pos_t *pos) { tor_assert(pos->pos < INT_MAX - 1); ++pos->pos; - if (pos->pos == (off_t)pos->chunk->datalen) { + if (pos->pos == (ptrdiff_t)pos->chunk->datalen) { if (!pos->chunk->next) return -1; pos->chunk_pos += pos->chunk->datalen; @@ -836,11 +837,11 @@ buf_peek_startswith(const buf_t *buf, const char *cmd) /** Return the index within buf at which ch first appears, * or -1 if ch does not appear on buf. */ -static off_t +static ptrdiff_t buf_find_offset_of_char(buf_t *buf, char ch) { chunk_t *chunk; - off_t offset = 0; + ptrdiff_t offset = 0; tor_assert(buf->datalen < INT_MAX); for (chunk = buf->head; chunk; chunk = chunk->next) { char *cp = memchr(chunk->data, ch, chunk->datalen); @@ -863,7 +864,7 @@ int buf_get_line(buf_t *buf, char *data_out, size_t *data_len) { size_t sz; - off_t offset; + ptrdiff_t offset; if (!buf->head) return 0; From 106b75aa5398f7a260a4d0f3093c244376258242 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 4 Sep 2019 11:09:01 -0400 Subject: [PATCH 6/6] changes file for 31532 --- changes/ticket31532 | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changes/ticket31532 diff --git a/changes/ticket31532 b/changes/ticket31532 new file mode 100644 index 0000000000..95bcbc517c --- /dev/null +++ b/changes/ticket31532 @@ -0,0 +1,4 @@ + o Code simplification and refactoring: + - Use the ptrdiff_t type consistently for expressing variable offsets and + pointer differences. Previously we incorrectly (but harmlessly) used + int and sometimes off_t for these cases. Closes ticket 31532.