Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix crash #1450

Merged
merged 9 commits into from
Oct 9, 2020
Merged

Fix crash #1450

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c415318
copying each header to the cache, we must leave space for the string
avbelov23 Sep 18, 2020
036420c
eliminated the progressive accumulation of duplicate lengths
avbelov23 Sep 25, 2020
f2130d5
change `new` according to `parent`
avbelov23 Oct 1, 2020
43d108e
change `parent` according to `nchild`
avbelov23 Oct 1, 2020
8531145
the function moves up the tree, and at each iteration, the node side
avbelov23 Oct 1, 2020
ae3e5f8
if there was a deletion, then we look for a new place to add a node,
avbelov23 Oct 1, 2020
9f0f753
adding an insert rebalance test
avbelov23 Oct 7, 2020
49598e2
adding a removal rebalance test
avbelov23 Oct 8, 2020
879b7c1
refactoring
avbelov23 Oct 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions tempesta_fw/cache.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -723,8 +723,8 @@ tfw_cache_build_resp_hdr(TDB *db, TfwHttpResp *resp, TfwHdrMods *hmods,
r = write_actor(db, trec, resp, p, s->len, &dc_iter);
if (unlikely(r))
break;
*acc_len += dc_iter.acc_len;;
}
*acc_len += dc_iter.acc_len;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!


return r;
}
Expand Down Expand Up @@ -1152,23 +1152,22 @@ tfw_cache_h2_copy_hdr(TfwCacheEntry *ce, char **p, TdbVRec **trec, TfwStr *hdr,
return -ENOMEM;
}

CSTR_MOVE_HDR();

if (TFW_STR_DUP(hdr)) {
CSTR_WRITE_HDR(TFW_STR_DUPLICATE, hdr->nchunks);
CSTR_MOVE_HDR();
CSTR_WRITE_HDR(TFW_STR_DUPLICATE, hdr->nchunks);
}

TFW_STR_FOR_EACH_DUP(dup, hdr, dup_end) {
unsigned int prev_len = ce->hdr_len;
unsigned int prev_len;

if (dupl) {
TFW_STR_INIT(&s_nm);
TFW_STR_INIT(&s_val);
tfw_http_hdr_split(dup, &s_nm, &s_val, true);
st_index = dup->hpack_idx;
CSTR_MOVE_HDR();
}
CSTR_MOVE_HDR();
prev_len = ce->hdr_len;

if (st_index) {
if (tfw_cache_h2_copy_int(&ce->hdr_len, st_index, 0xF,
Expand Down
52 changes: 38 additions & 14 deletions tempesta_fw/hpack.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2538,13 +2538,19 @@ tfw_hpack_rbtree_ins_rebalance(TfwHPackETbl *__restrict tbl,
HPACK_RB_SET_BLACK(uncle);
HPACK_RB_SET_RED(gparent);
parent = HPACK_NODE_COND(tbl, gparent->parent);
new = gparent;
}
else {
if (!HPACK_NODE_EMPTY(parent->right)
&& new == HPACK_NODE(tbl, parent->right))
{
tfw_hpack_rbtree_left_rt(tbl, parent);
parent = new;
/*
* Don't need `new = HPACK_NODE_COND(tbl, parent->left);`.
* This is the last iteration, because the parent will turn
* black below
*/
}
HPACK_RB_SET_BLACK(parent);
HPACK_RB_SET_RED(gparent);
Expand All @@ -2558,13 +2564,19 @@ tfw_hpack_rbtree_ins_rebalance(TfwHPackETbl *__restrict tbl,
HPACK_RB_SET_BLACK(uncle);
HPACK_RB_SET_RED(gparent);
parent = HPACK_NODE_COND(tbl, gparent->parent);
new = gparent;
}
else {
if (!HPACK_NODE_EMPTY(parent->left)
&& new == HPACK_NODE(tbl, parent->left))
{
tfw_hpack_rbtree_right_rt(tbl, parent);
parent = new;
/*
* Don't need `new = HPACK_NODE_COND(tbl, parent->right);`.
* This is the last iteration, because the parent will turn
* black below
*/
}
HPACK_RB_SET_BLACK(parent);
HPACK_RB_SET_RED(gparent);
Expand All @@ -2582,15 +2594,14 @@ tfw_hpack_rbtree_ins_rebalance(TfwHPackETbl *__restrict tbl,
static void
tfw_hpack_rbtree_del_rebalance(TfwHPackETbl *__restrict tbl,
TfwHPackNode *__restrict nchild,
TfwHPackNode *__restrict parent,
bool left)
TfwHPackNode *__restrict parent)
{
BUG_ON(!tbl->root);

while (!nchild || (nchild != tbl->root && HPACK_RB_IS_BLACK(nchild))) {
TfwHPackNode *brother, *l_neph, *r_neph;

if (left) {
if (nchild == HPACK_NODE_COND(tbl, parent->left)) {
BUG_ON(HPACK_NODE_EMPTY(parent->right));
brother = HPACK_NODE(tbl, parent->right);
if (HPACK_RB_IS_RED(brother)) {
Expand All @@ -2616,6 +2627,7 @@ tfw_hpack_rbtree_del_rebalance(TfwHPackETbl *__restrict tbl,
{
HPACK_RB_SET_RED(brother);
nchild = parent;
parent = HPACK_NODE_COND(tbl, nchild->parent);
}
else
{
Expand Down Expand Up @@ -2653,6 +2665,7 @@ tfw_hpack_rbtree_del_rebalance(TfwHPackETbl *__restrict tbl,
{
HPACK_RB_SET_RED(brother);
nchild = parent;
parent = HPACK_NODE_COND(tbl, nchild->parent);
}
else
{
Expand Down Expand Up @@ -2692,12 +2705,11 @@ tfw_hpack_rbtree_min(TfwHPackETbl *__restrict tbl,
/*
* Procedure for branches replacement in red-black tree.
*/
static inline bool
static inline void
tfw_hpack_rbtree_replace(TfwHPackETbl *__restrict tbl,
TfwHPackNode *__restrict old,
TfwHPackNode *__restrict new)
{
bool left = true;
TfwHPackNode *parent = HPACK_NODE_COND(tbl, old->parent);

BUG_ON(!old);
Expand All @@ -2716,13 +2728,10 @@ tfw_hpack_rbtree_replace(TfwHPackETbl *__restrict tbl,
WARN_ON_ONCE(HPACK_NODE_EMPTY(parent->right)
|| old != HPACK_NODE(tbl, parent->right));
parent->right = HPACK_NODE_COND_OFF(tbl, new);
left = false;
}

if (new)
new->parent = HPACK_NODE_COND_OFF(tbl, parent);

return left;
}

/*
Expand Down Expand Up @@ -2820,15 +2829,15 @@ tfw_hpack_rbtree_erase(TfwHPackETbl *__restrict tbl,
{
TfwHPackNode *nchild, *sv = node;
TfwHPackNode *parent = HPACK_NODE_COND(tbl, node->parent);
bool left_child = false, sv_black = HPACK_RB_IS_BLACK(sv);
bool sv_black = HPACK_RB_IS_BLACK(sv);

if (HPACK_NODE_EMPTY(node->left)) {
nchild = HPACK_NODE_COND(tbl, node->right);
left_child = tfw_hpack_rbtree_replace(tbl, node, nchild);
tfw_hpack_rbtree_replace(tbl, node, nchild);
}
else if (HPACK_NODE_EMPTY(node->right)) {
nchild = HPACK_NODE_COND(tbl, node->left);
left_child = tfw_hpack_rbtree_replace(tbl, node, nchild);
tfw_hpack_rbtree_replace(tbl, node, nchild);
}
else {
TfwHPackNode *n_left;
Expand All @@ -2842,7 +2851,7 @@ tfw_hpack_rbtree_erase(TfwHPackETbl *__restrict tbl,
TfwHPackNode *n_right;

parent = HPACK_NODE(tbl, sv->parent);
left_child = tfw_hpack_rbtree_replace(tbl, sv, nchild);
tfw_hpack_rbtree_replace(tbl, sv, nchild);

n_right = HPACK_NODE(tbl, node->right);
n_right->parent = HPACK_NODE_OFF(tbl, sv);
Expand All @@ -2867,7 +2876,7 @@ tfw_hpack_rbtree_erase(TfwHPackETbl *__restrict tbl,
* the last).
*/
if (sv_black && tbl->root)
tfw_hpack_rbtree_del_rebalance(tbl, nchild, parent, left_child);
tfw_hpack_rbtree_del_rebalance(tbl, nchild, parent);
}

static inline void
Expand Down Expand Up @@ -2945,18 +2954,33 @@ tfw_hpack_rbuf_calc(TfwHPackETbl *__restrict tbl, unsigned short new_size,

static inline void
tfw_hpack_rbuf_commit(TfwHPackETbl *__restrict tbl,
TfwStr *__restrict hdr,
TfwHPackNode *__restrict del_list[],
TfwHPackNodeIter *__restrict place,
TfwHPackETblIter *__restrict iter)
{
int i;
bool was_del = false;
const TfwHPackNode *node = NULL;
TfwHPackETblRes res = HPACK_IDX_ST_NOT_FOUND;

for (i = 0; i < HPACK_MAX_ENC_EVICTION; ++i) {
TfwHPackNode *del_node = del_list[i];

if (!del_node)
break;
tfw_hpack_rbtree_erase(tbl, del_node);
was_del = true;
}

/*
* If there was a deletion, the place may turn out to be invalid as a result
* of reducing the procedure for deleting a node with two children to
* deleting a node with less than two children.
*/
if (was_del) {
res = tfw_hpack_rbtree_find(tbl, hdr, &node, place);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, there is a case, when place can become invalid, but the proposed change is expensive: it possibly involves a lot of string compare operations. It seems that tfw_hpack_rbtree_erase() should be updated to keep place valid.

WARN_ON_ONCE(res == HPACK_IDX_ST_FOUND);
}

tfw_hpack_rbtree_add(tbl, iter->last, place);
Expand Down Expand Up @@ -3084,7 +3108,7 @@ tfw_hpack_add_node(TfwHPackETbl *__restrict tbl, TfwStr *__restrict hdr,
ptr = tfw_hpack_write(&s_nm, it.last->hdr);
tfw_hpack_write(&s_val, ptr);

tfw_hpack_rbuf_commit(tbl, del_list, place, &it);
tfw_hpack_rbuf_commit(tbl, hdr, del_list, place, &it);

WARN_ON_ONCE(tbl->rb_len > tbl->size);

Expand Down
Loading