Test case and fix for lsquic rechist list problem (#410)

* Test case and debug for lsquic rechist list problem

Add:
- test case
- debug rechist print function (not called)
Modify:
- rechist_test_sanity to be more robust

Test case:
Added test case which produces a corrupt rechist.  The rechist
grows and then is emptied, then a rechist is produced that is nominally
empty (rh_n_used == 0) but has a circular list in rh_elems.  E.g.  (using
debug print at the problem site):

lsquic_rechist_received: insert before done
0x7ffe65ce75a0: cutoff 9 l. acked 12087061905995 masks 1 alloced 4 used 1 max ranges 0 head 0
  (0)[9-9] (0)[9-9] (0)[9-9] (0)[9-9] (0)[9-9] (0)[9-9] (0)[9-9]
test_rechist: /tmp/lsquic/src/liblsquic/lsquic_rechist.c:272: rechist_test_sanity: Assertion `rechist->rh_n_used == n_elems' failed.

Debug print:
Added 'rechist_dump' for ease of debugging, marked as unused.
Useful for calling from gdb or adding to code temporarily.

rechist_test_sanity:
Make it more robust to bad rechists by limiting
how many list items it will follow, to prevent infinite loops or walking off
the end of the list.

* rechist: Fix bug in lsquic_rechist_received causing circular list to be created

Fix bug in lsquic_rechist_received demonstrated by the test6 test-case.

When the list is empty, i.e.  rh_n_used == 0, the 'first_elem' path should
be used.  However, the test for this was using rh_n_alloced, which would
cause the code to continue on incorrectly.  One possibility is that it goes
to insert_before and creates a circular list in the rechist with the head at
index 0 having a next of 0.

This causes history to be lost I think.

The list at this point is still 'empty', however a following call could go
to the insert_before path and then insert an elem pointing to the circular
list, and bump up rh_n_used. Now you have a circular list.

Weird things can happen now.  Notably, ACK generation will exhaust the
packet buffer and generate an error, and so cause connections to be
prematurely aborted.

Fix lsquic_rechist_received to use rh_n_used. Also modify
rechist_alloc_elem to NULL the next pointer for robustness.

It would be nice for rechist_free_elem to also invalidate the elem, but it
is meant for relinking elems in the list and should preserve the next
pointer.
This commit is contained in:
Paul Jakma 2022-08-26 14:15:35 +01:00 committed by GitHub
parent 0aa00f2369
commit 188c055a9c
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 92 additions and 6 deletions

View file

@ -9,6 +9,8 @@
#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include <stdio.h>
#include <inttypes.h>
#include "lsquic_int_types.h"
#include "lsquic_rechist.h"
@ -57,6 +59,49 @@ rechist_free_elem (struct lsquic_rechist *rechist, unsigned idx)
#define RE_HIGH(el_) ((el_)->re_low + (el_)->re_count - 1)
#if LSQUIC_TEST
static void
rechist_dump_elem(struct rechist_elem *el)
{
fprintf(stderr,"[%"PRIu64"-%"PRIu64"]", RE_HIGH(el), el->re_low);
}
#ifdef __GNUC__
__attribute__((unused))
#endif
static void
rechist_dump(struct lsquic_rechist *rechist)
{
fprintf(stderr,
"%p: cutoff %"PRIu64" l. acked %"PRIu64" masks %u alloced %u used %u max ranges %u head %u\n",
rechist,
rechist->rh_cutoff,
rechist->rh_largest_acked_received,
rechist->rh_n_masks,
rechist->rh_n_alloced,
rechist->rh_n_used,
rechist->rh_max_ranges,
rechist->rh_head);
if (rechist->rh_n_used)
{
int idx = rechist->rh_head;
fprintf(stderr, " ");
unsigned n_elems = 0;
while (n_elems < rechist->rh_n_used)
{
++n_elems;
struct rechist_elem *el = &rechist->rh_elems[idx];
fprintf(stderr, " (%u)", idx);
rechist_dump_elem(el);
if (el->re_next == UINT_MAX)
break;
idx = el->re_next;
}
fprintf(stderr, "\n");
}
}
#endif /* LSQUIC_TEST */
static unsigned
find_free_slot (uintptr_t slots)
@ -174,6 +219,8 @@ rechist_alloc_elem (struct lsquic_rechist *rechist)
idx = find_free_slot(*mask);
*mask |= 1ull << idx;
++rechist->rh_n_used;
/*Note that re_next is invalid at this point, caller must set it */
return idx + i * BITS_PER_MASK;
}
@ -201,6 +248,9 @@ rechist_test_sanity (const struct lsquic_rechist *rechist)
{
++n_elems;
idx = el - rechist->rh_elems;
if (n_elems > rechist->rh_n_alloced
|| idx >= rechist->rh_n_alloced)
break;
masks[idx >> LOG2_BITS] |= 1ull << (idx & ((1u << LOG2_BITS) - 1));
if (el->re_next != UINT_MAX)
el = &rechist->rh_elems[el->re_next];
@ -215,8 +265,7 @@ rechist_test_sanity (const struct lsquic_rechist *rechist)
free(masks);
}
#define rechist_sanity_check(rechist_) do { \
if (0 == ++(rechist_)->rh_n_ops % 127) \
rechist_test_sanity(rechist_); \
rechist_test_sanity(rechist_); \
} while (0)
#else
#define rechist_sanity_check(rechist)
@ -231,7 +280,7 @@ lsquic_rechist_received (lsquic_rechist_t *rechist, lsquic_packno_t packno,
ptrdiff_t next_idx, prev_idx;
int idx;
if (rechist->rh_n_alloced == 0)
if (rechist->rh_n_used == 0)
goto first_elem;
if (packno < rechist->rh_cutoff)

View file

@ -39,9 +39,6 @@ struct lsquic_rechist {
struct lsquic_packno_range range;
unsigned next;
} rh_iter;
#if LSQUIC_TEST
unsigned rh_n_ops;
#endif
};
typedef struct lsquic_rechist lsquic_rechist_t;

View file

@ -195,6 +195,44 @@ test5 (void)
lsquic_rechist_cleanup(&rechist);
}
/* Regression test for bug where rechist ends up empty (rh_used == 0)
* with invalid head entry with a self-referential next, and
* lsquic_rechist_received would follow it because it didn't check rh_used.
*/
static void
test6 (void)
{
lsquic_rechist_t rechist;
char buf[256];
long int time = 12087061905875;
lsquic_rechist_init(&rechist, 0, 0);
for (int i = 0; i <= 3; i++)
lsquic_rechist_received(&rechist, i, (time += (i*10)));
lsquic_rechist_stop_wait(&rechist, 2);
lsquic_rechist_received(&rechist, 4, (time += 10));
lsquic_rechist_stop_wait(&rechist, 3);
lsquic_rechist_received(&rechist, 5, (time += 10));
lsquic_rechist_stop_wait(&rechist, 3);
lsquic_rechist_received(&rechist, 6, (time += 10));
lsquic_rechist_stop_wait(&rechist, 9);
lsquic_rechist_received(&rechist, 7, (time += 10));
lsquic_rechist_received(&rechist, 8, (time += 10));
lsquic_rechist_received(&rechist, 9, (time += 10));
rechist2str(&rechist, buf, sizeof(buf));
lsquic_rechist_cleanup(&rechist);
}
static void
test_rand_sequence (unsigned seed, unsigned max)
@ -393,6 +431,8 @@ main (void)
test4();
test5();
test6();
for (i = 0; i < 10; ++i)
test_rand_sequence(i, 0);