Skip to content

Commit

Permalink
ccan: update to latest htable fixes, and update gossmap to meet new a…
Browse files Browse the repository at this point in the history
…ssertions.

Updating ccan to stricter htable revealed we were trying to put
(void *)1 in the htable, which is forbidden:

```
topology: ccan/ccan/htable/htable.c:382: htable_add_: Assertion `entry_is_valid((uintptr_t)p)' failed.
topology: FATAL SIGNAL 6 (version 1358d7f)
0x55f30c689c34 send_backtrace
	common/daemon.c:33
0x55f30c689ce0 crashdump
	common/daemon.c:46
0x7f5d150fe51f ???
	./signal/../sysdeps/unix/sysv/linux/x86_64/libc_sigaction.c:0
0x7f5d15152828 __pthread_kill_implementation
	./nptl/pthread_kill.c:44
0x7f5d15152828 __pthread_kill_internal
	./nptl/pthread_kill.c:80
0x7f5d15152828 __GI___pthread_kill
	./nptl/pthread_kill.c:91
0x7f5d150fe475 __GI_raise
	../sysdeps/posix/raise.c:26
0x7f5d150e47b6 __GI_abort
	./stdlib/abort.c:79
0x7f5d150e46da __assert_fail_base
	./assert/assert.c:92
0x7f5d150f5e25 __GI___assert_fail
	./assert/assert.c:101
0x55f30c6adbe4 htable_add_
	ccan/ccan/htable/htable.c:382
0x55f30c65f303 chanidx_htable_add
	common/gossmap.c:35
0x55f30c6605ed new_channel
	common/gossmap.c:337
0x55f30c6609cf add_channel
	common/gossmap.c:425
0x55f30c661101 map_catchup
	common/gossmap.c:607
0x55f30c66221e gossmap_refresh
	common/gossmap.c:927
0x55f30c66e3e9 get_gossmap
	plugins/topology.c:27
0x55f30c66f939 listpeers_done
	plugins/topology.c:369
0x55f30c671f46 handle_rpc_reply
	plugins/libplugin.c:558
0x55f30c672a19 rpc_read_response_one
	plugins/libplugin.c:726
0x55f30c672b4f rpc_conn_read_response
	plugins/libplugin.c:746
0x55f30c6ae35e next_plan
	ccan/ccan/io/io.c:59
0x55f30c6aef93 do_plan
	ccan/ccan/io/io.c:407
0x55f30c6aefd5 io_ready
	ccan/ccan/io/io.c:417
0x55f30c6b1371 io_loop
	ccan/ccan/io/poll.c:453
0x55f30c67587c plugin_main
	plugins/libplugin.c:1559
0x55f30c6708eb main
	plugins/topology.c:701
0x7f5d150e5fcf __libc_start_call_main
	../sysdeps/nptl/libc_start_call_main.h:58
0x7f5d150e607c __libc_start_main_impl
	../csu/libc-start.c:409
0x55f30c65d894 ???
	???:0
0xffffffffffffffff ???
	???:0
```

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
  • Loading branch information
rustyrussell committed Jul 6, 2022
1 parent bad943d commit 3efc924
Show file tree
Hide file tree
Showing 17 changed files with 499 additions and 76 deletions.
2 changes: 1 addition & 1 deletion ccan/README
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
CCAN imported from http://ccodearchive.net.

CCAN version: init-2524-g609670cc
CCAN version: init-2540-g8448fd28
141 changes: 101 additions & 40 deletions ccan/ccan/htable/htable.c
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,16 @@ void htable_init(struct htable *ht,
ht->table = &ht->common_bits;
}

/* Fill to 87.5% */
static inline size_t ht_max(const struct htable *ht)
{
return ((size_t)3 << ht->bits) / 4;
return ((size_t)7 << ht->bits) / 8;
}

static inline size_t ht_max_with_deleted(const struct htable *ht)
/* Clean deleted if we're full, and more than 12.5% deleted */
static inline size_t ht_max_deleted(const struct htable *ht)
{
return ((size_t)9 << ht->bits) / 10;
return ((size_t)1 << ht->bits) / 8;
}

bool htable_init_sized(struct htable *ht,
Expand All @@ -103,7 +105,7 @@ bool htable_init_sized(struct htable *ht,
htable_init(ht, rehash, priv);

/* Don't go insane with sizing. */
for (ht->bits = 1; ((size_t)3 << ht->bits) / 4 < expect; ht->bits++) {
for (ht->bits = 1; ht_max(ht) < expect; ht->bits++) {
if (ht->bits == 30)
break;
}
Expand Down Expand Up @@ -195,12 +197,83 @@ void *htable_prev_(const struct htable *ht, struct htable_iter *i)
for (;;) {
if (!i->off)
return NULL;
i->off --;
i->off--;
if (entry_is_valid(ht->table[i->off]))
return get_raw_ptr(ht, ht->table[i->off]);
}
}

/* Another bit currently in mask needs to be exposed, so that a bucket with p in
* it won't appear invalid */
static COLD void unset_another_common_bit(struct htable *ht,
uintptr_t *maskdiff,
const void *p)
{
size_t i;

for (i = sizeof(uintptr_t) * CHAR_BIT - 1; i > 0; i--) {
if (((uintptr_t)p & ((uintptr_t)1 << i))
&& ht->common_mask & ~*maskdiff & ((uintptr_t)1 << i))
break;
}
/* There must have been one, right? */
assert(i > 0);

*maskdiff |= ((uintptr_t)1 << i);
}

/* We want to change the common mask: this fixes up the table */
static COLD void fixup_table_common(struct htable *ht, uintptr_t maskdiff)
{
size_t i;
uintptr_t bitsdiff;

again:
bitsdiff = ht->common_bits & maskdiff;

for (i = 0; i < (size_t)1 << ht->bits; i++) {
uintptr_t e;
if (!entry_is_valid(e = ht->table[i]))
continue;

/* Clear the bits no longer in the mask, set them as
* expected. */
e &= ~maskdiff;
e |= bitsdiff;
/* If this made it invalid, restart with more exposed */
if (!entry_is_valid(e)) {
unset_another_common_bit(ht, &maskdiff, get_raw_ptr(ht, e));
goto again;
}
ht->table[i] = e;
}

/* Take away those bits from our mask, bits and perfect bit. */
ht->common_mask &= ~maskdiff;
ht->common_bits &= ~maskdiff;
if (ht_perfect_mask(ht) & maskdiff)
ht->perfect_bitnum = NO_PERFECT_BIT;
}

/* Limited recursion */
static void ht_add(struct htable *ht, const void *new, size_t h);

/* We tried to add this entry, but it looked invalid! We need to
* let another pointer bit through mask */
static COLD void update_common_fix_invalid(struct htable *ht, const void *p, size_t h)
{
uintptr_t maskdiff;

assert(ht->elems != 0);

maskdiff = 0;
unset_another_common_bit(ht, &maskdiff, p);
fixup_table_common(ht, maskdiff);

/* Now won't recurse */
ht_add(ht, p, h);
}

/* This does not expand the hash table, that's up to caller. */
static void ht_add(struct htable *ht, const void *new, size_t h)
{
Expand All @@ -214,6 +287,8 @@ static void ht_add(struct htable *ht, const void *new, size_t h)
i = (i + 1) & ((1 << ht->bits)-1);
}
ht->table[i] = make_hval(ht, new, get_hash_ptr_bits(ht, h)|perfect);
if (!entry_is_valid(ht->table[i]))
update_common_fix_invalid(ht, new, h);
}

static COLD bool double_table(struct htable *ht)
Expand Down Expand Up @@ -283,20 +358,10 @@ static COLD void rehash_table(struct htable *ht)
/* We stole some bits, now we need to put them back... */
static COLD void update_common(struct htable *ht, const void *p)
{
unsigned int i;
uintptr_t maskdiff, bitsdiff;
uintptr_t maskdiff;

if (ht->elems == 0) {
/* Always reveal one bit of the pointer in the bucket,
* so it's not zero or HTABLE_DELETED (1), even if
* hash happens to be 0. Assumes (void *)1 is not a
* valid pointer. */
for (i = sizeof(uintptr_t)*CHAR_BIT - 1; i > 0; i--) {
if ((uintptr_t)p & ((uintptr_t)1 << i))
break;
}

ht->common_mask = ~((uintptr_t)1 << i);
ht->common_mask = -1;
ht->common_bits = ((uintptr_t)p & ht->common_mask);
ht->perfect_bitnum = 0;
(void)htable_debug(ht, HTABLE_LOC);
Expand All @@ -306,33 +371,25 @@ static COLD void update_common(struct htable *ht, const void *p)
/* Find bits which are unequal to old common set. */
maskdiff = ht->common_bits ^ ((uintptr_t)p & ht->common_mask);

/* These are the bits which go there in existing entries. */
bitsdiff = ht->common_bits & maskdiff;

for (i = 0; i < (size_t)1 << ht->bits; i++) {
if (!entry_is_valid(ht->table[i]))
continue;
/* Clear the bits no longer in the mask, set them as
* expected. */
ht->table[i] &= ~maskdiff;
ht->table[i] |= bitsdiff;
}

/* Take away those bits from our mask, bits and perfect bit. */
ht->common_mask &= ~maskdiff;
ht->common_bits &= ~maskdiff;
if (ht_perfect_mask(ht) & maskdiff)
ht->perfect_bitnum = NO_PERFECT_BIT;
fixup_table_common(ht, maskdiff);
(void)htable_debug(ht, HTABLE_LOC);
}

bool htable_add_(struct htable *ht, size_t hash, const void *p)
{
if (ht->elems+1 > ht_max(ht) && !double_table(ht))
return false;
if (ht->elems+1 + ht->deleted > ht_max_with_deleted(ht))
rehash_table(ht);
/* Cannot insert NULL, or (void *)1. */
assert(p);
assert(entry_is_valid((uintptr_t)p));

/* Getting too full? */
if (ht->elems+1 + ht->deleted > ht_max(ht)) {
/* If we're more than 1/8 deleted, clean those,
* otherwise double table size. */
if (ht->deleted > ht_max_deleted(ht))
rehash_table(ht);
else if (!double_table(ht))
return false;
}
if (((uintptr_t)p & ht->common_mask) != ht->common_bits)
update_common(ht, p);

Expand Down Expand Up @@ -361,8 +418,12 @@ void htable_delval_(struct htable *ht, struct htable_iter *i)
assert(entry_is_valid(ht->table[i->off]));

ht->elems--;
ht->table[i->off] = HTABLE_DELETED;
ht->deleted++;
/* Cheap test: if the next bucket is empty, don't need delete marker */
if (ht->table[hash_bucket(ht, i->off+1)] != 0) {
ht->table[i->off] = HTABLE_DELETED;
ht->deleted++;
} else
ht->table[i->off] = 0;
}

void *htable_pick_(const struct htable *ht, size_t seed, struct htable_iter *i)
Expand Down
2 changes: 1 addition & 1 deletion ccan/ccan/htable/htable.h
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ bool htable_copy_(struct htable *dst, const struct htable *src);
* htable_add - add a pointer into a hash table.
* @ht: the htable
* @hash: the hash value of the object
* @p: the non-NULL pointer
* @p: the non-NULL pointer (also cannot be (void *)1).
*
* Also note that this can only fail due to allocation failure. Otherwise, it
* returns true.
Expand Down
31 changes: 31 additions & 0 deletions ccan/ccan/htable/test/run-clash.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#include <ccan/htable/htable.h>
#include <ccan/htable/htable.c>
#include <ccan/tap/tap.h>
#include <stdbool.h>
#include <string.h>

/* Clashy hash */
static size_t hash(const void *elem, void *unused UNNEEDED)
{
return 0;
}

int main(void)
{
struct htable ht;

plan_tests(254 * 253);
/* We try to get two elements which clash */
for (size_t i = 2; i < 256; i++) {
for (size_t j = 2; j < 256; j++) {
if (i == j)
continue;
htable_init(&ht, hash, NULL);
htable_add(&ht, hash((void *)i, NULL), (void *)i);
htable_add(&ht, hash((void *)j, NULL), (void *)j);
ok1(htable_check(&ht, "test"));
htable_clear(&ht);
}
}
return exit_status();
}
11 changes: 2 additions & 9 deletions ccan/ccan/htable/test/run-debug.c
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ static bool check_mask(struct htable *ht, uint64_t val[], unsigned num)

int main(void)
{
unsigned int i, weight;
unsigned int i;
uintptr_t perfect_bit;
struct htable ht;
uint64_t val[NUM_VALS];
Expand All @@ -131,14 +131,7 @@ int main(void)
add_vals(&ht, val, 0, 1);
ok1(ht.bits == 1);
ok1(ht_max(&ht) == 1);
weight = 0;
for (i = 0; i < sizeof(ht.common_mask) * CHAR_BIT; i++) {
if (ht.common_mask & ((uintptr_t)1 << i)) {
weight++;
}
}
/* Only one bit should be clear. */
ok1(weight == i-1);
ok1(ht.common_mask == -1);

/* Mask should be set. */
ok1(check_mask(&ht, val, 1));
Expand Down
11 changes: 2 additions & 9 deletions ccan/ccan/htable/test/run.c
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ static bool check_mask(struct htable *ht, uint64_t val[], unsigned num)

int main(void)
{
unsigned int i, weight;
unsigned int i;
uintptr_t perfect_bit;
struct htable ht;
uint64_t val[NUM_VALS];
Expand All @@ -122,14 +122,7 @@ int main(void)
add_vals(&ht, val, 0, 1);
ok1(ht.bits == 1);
ok1(ht_max(&ht) == 1);
weight = 0;
for (i = 0; i < sizeof(ht.common_mask) * CHAR_BIT; i++) {
if (ht.common_mask & ((uintptr_t)1 << i)) {
weight++;
}
}
/* Only one bit should be clear. */
ok1(weight == i-1);
ok1(ht.common_mask == -1);

/* Mask should be set. */
ok1(check_mask(&ht, val, 1));
Expand Down
3 changes: 2 additions & 1 deletion ccan/ccan/htable/tools/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ CFLAGS=-Wall -Werror -O3 -I$(CCANDIR)

CCAN_OBJS:=ccan-tal.o ccan-tal-str.o ccan-tal-grab_file.o ccan-take.o ccan-time.o ccan-str.o ccan-noerr.o ccan-list.o

all: speed stringspeed hsearchspeed
all: speed stringspeed hsearchspeed density

speed: speed.o hash.o $(CCAN_OBJS)
density: density.o hash.o $(CCAN_OBJS)

speed.o: speed.c ../htable.h ../htable.c

Expand Down
Loading

0 comments on commit 3efc924

Please sign in to comment.