Skip to content

Fix HPACK dynamic table desync #2628

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

Draft
const-t wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

Fix HPACK dynamic table desync #2628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
96036e6
fix: Sending of the dynamic table size update
const-t Mar 9, 2026
6d2e600
fix: HPACK dynamic table desync
const-t Mar 11, 2026
1efdd6f
fix: couple of HPACK desyncs in trailers
const-t Mar 13, 2026
d31a086
refactor: Move skb related functions to ss_skb.h
const-t Mar 16, 2026
9695e28
refactor: get rid of code duplication
const-t Mar 16, 2026
6d94657
fix: forbid `RST_STREAM` between `HEADERS` and `CONTINUATION` frames
const-t Apr 9, 2026
5fa0db4
fix: blocked data sending even when tcp window is enough
const-t Apr 21, 2026
ca73041
fix: sending and receiving RST_STREAM breaks HPACK context
const-t Apr 21, 2026
f6e2987
fix: reset connection instead of stream
const-t Apr 22, 2026
fe67274
fix: write dynamic table size before check send window
const-t Apr 30, 2026
0076582
refactor: "overloaded" function `tfw_h2_insert_frame_header()`
const-t Apr 30, 2026
6eba21e
fix: don't return positive values
const-t May 14, 2026
0edde4e
refactor: Move adding stream to closed to the same place as cleaning
const-t May 14, 2026
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
1 change: 0 additions & 1 deletion fw/cache.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
#include <linux/tcp.h>
#include <linux/topology.h>
#include <linux/nodemask.h>
#include <linux/skbuff_ref.h>

#undef DEBUG
#if DBG_CACHE > 0
Expand Down
42 changes: 22 additions & 20 deletions fw/hpack.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3718,23 +3718,19 @@ tfw_hpack_encode(TfwHttpResp *__restrict resp, TfwStr *__restrict hdr,
* into the HTTP/2 HPACK format.
*/
int
tfw_hpack_transform(TfwHttpResp *__restrict resp, TfwStr *__restrict hdr)
tfw_hpack_transform(TfwHttpResp *__restrict resp, TfwStr *__restrict hdr,
bool dyn_indexing)
{
return __tfw_hpack_encode(resp, hdr, true, true, true);
return __tfw_hpack_encode(resp, hdr, true, dyn_indexing, true);
}

void
tfw_hpack_set_rbuf_size(TfwHPackETbl *__restrict tbl, unsigned short new_size)
tfw_hpack_set_rbuf_size(TfwHPackETbl *__restrict tbl,
unsigned int requested_size)
{
if (new_size > HPACK_ENC_TABLE_MAX_SIZE) {
T_WARN("Client requests hpack table size (%hu), which is "
"greater than HPACK_ENC_TABLE_MAX_SIZE.", new_size);
new_size = HPACK_ENC_TABLE_MAX_SIZE;
}

T_DBG3("%s: tbl->rb_len=%hu, tbl->size=%hu, tbl->window=%hu,"
" new_size=%hu\n", __func__, tbl->rb_len, tbl->size,
tbl->window, new_size);
" requested_size=%u\n", __func__, tbl->rb_len, tbl->size,
tbl->window, requested_size);

/*
* RFC7541#section-4.2:
Expand All @@ -3744,21 +3740,29 @@ tfw_hpack_set_rbuf_size(TfwHPackETbl *__restrict tbl, unsigned short new_size)
* size that occurs in that interval MUST be signaled in a dynamic
* table size update.
*/
if (tbl->window != new_size && (likely(!tbl->wnd_changed)
|| unlikely(!tbl->window) || new_size < tbl->window))
if (tbl->window != requested_size && (likely(!tbl->wnd_changed)
Copy link
Copy Markdown
Contributor

@EvgeniiMekhanik EvgeniiMekhanik May 8, 2026

Choose a reason for hiding this comment

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

I read RFC again and look for nghtt2 realization - in some cases we should apply and send two hpack dynamic changes!
For example
4096 - > 1 -> 100 we should send 1 and 100 (minimum and final)
4096 -> 1 -> 0 -> 2 -> 100 -> 3 we should send 0 and 3 (minimum and final)

Copy link
Copy Markdown
Contributor Author

@const-t const-t May 8, 2026

Choose a reason for hiding this comment

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

Good catch, another one bug. Tempesta must be able to send min and max size. RFC cite:

Multiple updates to the maximum table size can occur between the
transmission of two header blocks. In the case that this size is
changed more than once in this interval, the smallest maximum table
size that occurs in that interval MUST be signaled in a dynamic table
size update. The final maximum size is always signaled, resulting in
at most two dynamic table size updates. This ensures that the
decoder is able to perform eviction based on reductions in dynamic
table size

However maybe it makes sense to implement it in other PR, to keep this PR small.

Copy link
Copy Markdown
Contributor

@EvgeniiMekhanik EvgeniiMekhanik May 9, 2026

Choose a reason for hiding this comment

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

Yes also look nghttp2_hd_deflate_hd_bufs in nghttp2

if (deflater->ctx.hd_table_bufsize_max > min_hd_table_bufsize_max) {

      rv = emit_table_size(bufs, min_hd_table_bufsize_max);

      if (rv != 0) {
        goto fail;
      }
    }

    rv = emit_table_size(bufs, deflater->ctx.hd_table_bufsize_max);

|| unlikely(!tbl->window) || requested_size < tbl->window))
{
unsigned short new_size = min_t(unsigned int, requested_size,
HPACK_ENC_TABLE_MAX_SIZE);
BUILD_BUG_ON(HPACK_ENC_TABLE_MAX_SIZE > USHRT_MAX ||
sizeof(new_size) != sizeof(tbl->window));
if (tbl->size > new_size)
tfw_hpack_rbuf_calc(tbl, new_size, NULL,
(TfwHPackETblIter *)tbl);
WARN_ON_ONCE(tbl->rb_len > tbl->size);

tbl->window = new_size;
tbl->wnd_changed = true;

T_DBG3("%s: New hpack encoder table size has been set to %u\n",
__func__, tbl->window);
}
}

int
tfw_hpack_enc_tbl_write_sz(TfwHPackETbl *__restrict tbl, TfwStream *stream)
tfw_hpack_enc_tbl_write_sz(TfwHPackETbl *tbl, struct sk_buff *skb_head,
unsigned int offset, unsigned int *acc_len)
{
TfwHPackInt tmp = {};
TfwStr dst = {};
Expand All @@ -3769,18 +3773,16 @@ tfw_hpack_enc_tbl_write_sz(TfwHPackETbl *__restrict tbl, TfwStream *stream)
WARN_ON_ONCE(!tbl->wnd_changed);
write_int(tbl->window, 0x1F, 0x20, &tmp);

data = ss_skb_data_ptr_by_offset(stream->xmit.skb_head,
FRAME_HEADER_SIZE);
data = ss_skb_data_ptr_by_offset(skb_head,
offset + FRAME_HEADER_SIZE);
BUG_ON(!data);

r = ss_skb_get_room_w_frag(stream->xmit.skb_head,
stream->xmit.skb_head,
data, tmp.sz, &dst, &_);
r = ss_skb_get_room_w_frag(skb_head, skb_head, data, tmp.sz, &dst, &_);
if (unlikely(r))
return r;

memcpy_fast(dst.data, tmp.buf, tmp.sz);
stream->xmit.h_len += tmp.sz;
*acc_len += tmp.sz;
tbl->wnd_changed = false;

return 0;
Expand Down
9 changes: 6 additions & 3 deletions fw/hpack.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,19 +302,22 @@ void write_int(unsigned long index, unsigned short max, unsigned short mask,
int tfw_hpack_init(TfwHPack *__restrict hp, TfwClientMem *owner,
unsigned int htbl_sz);
void tfw_hpack_clean(TfwHPack *__restrict hp);
int tfw_hpack_transform(TfwHttpResp *__restrict resp, TfwStr *__restrict hdr);
int tfw_hpack_transform(TfwHttpResp *__restrict resp, TfwStr *__restrict hdr,
bool dyn_indexing);
int tfw_hpack_encode(TfwHttpResp *__restrict resp, TfwStr *__restrict hdr,
bool use_pool, bool dyn_indexing);
void tfw_hpack_set_rbuf_size(TfwHPackETbl *__restrict tbl,
unsigned short new_size);
unsigned int new_size);
int tfw_hpack_decode(TfwHPack *__restrict hp, unsigned char *__restrict src,
unsigned long n, TfwHttpReq *__restrict req,
unsigned int *__restrict parsed);
int tfw_hpack_cache_decode_expand(TfwHPack *__restrict hp,
TfwHttpResp *__restrict resp,
unsigned char *__restrict src, unsigned long n,
TfwDecodeCacheIter *__restrict cd_iter);
int tfw_hpack_enc_tbl_write_sz(TfwHPackETbl *__restrict tbl, TfwStream *stream);
int tfw_hpack_enc_tbl_write_sz(TfwHPackETbl *tbl, struct sk_buff *skb_head,
unsigned int offset,
unsigned int *acc_len);

static inline unsigned int
tfw_hpack_int_size(unsigned long index, unsigned short max)
Expand Down
42 changes: 11 additions & 31 deletions fw/http.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@
#include <linux/string.h>
#include <linux/sort.h>
#include <linux/bsearch.h>
#include <linux/skbuff_ref.h>

#undef DEBUG
#if DBG_HTTP > 0
Expand Down Expand Up @@ -1818,31 +1817,12 @@ do { \
}
}

static void
__tfw_http_free_cleanup(TfwHttpMsgCleanup *cleanup)
{
int i;
struct sk_buff *skb;

while ((skb = ss_skb_dequeue(&cleanup->skb_head)))
__ss_kfree_skb(skb);

for (i = 0; i < cleanup->pages_sz; i++)
/*
* Pass "true" even for non recyclable pages, relying on check
* pp_magic == PP_SIGNATURE in napi_pp_put_page(), which avoid
* recycling of non page_pool pages. Overhead seems the same
* as to have/maintain flag for each fragment.
*/
skb_page_unref(cleanup->pages[i], true);
}

static void
__tfw_http_req_cleanup(TfwHttpReq *req)
{
if (!req->cleanup)
return;
__tfw_http_free_cleanup(req->cleanup);
ss_skb_free_cleanup(req->cleanup);
req->cleanup = NULL;
}

Expand Down Expand Up @@ -3918,7 +3898,7 @@ tfw_h1_adjust_req(TfwHttpReq *req)
req->vhost,
TFW_VHOST_HDRMOD_REQ);

req->cleanup = tfw_pool_alloc(hm->pool, sizeof(TfwHttpMsgCleanup));
req->cleanup = tfw_pool_alloc(hm->pool, sizeof(TfwSkbCleanup));
if (unlikely(!req->cleanup))
return -ENOMEM;
req->cleanup->pages_sz = 0;
Expand Down Expand Up @@ -4272,10 +4252,10 @@ tfw_h2_adjust_req(TfwHttpReq *req)
bool need_cl = req->body.len &&
TFW_STR_EMPTY(&ht->tbl[TFW_HTTP_HDR_CONTENT_LENGTH]);

req->cleanup = tfw_pool_alloc(req->pool, sizeof(TfwHttpMsgCleanup));
req->cleanup = tfw_pool_alloc(req->pool, sizeof(TfwSkbCleanup));
if (unlikely(!req->cleanup))
return -ENOMEM;
memset(req->cleanup, 0, sizeof(TfwHttpMsgCleanup));
memset(req->cleanup, 0, sizeof(TfwSkbCleanup));

if (need_cl) {
cl_data_len = tfw_ultoa(req->body.len, cl_data, TFW_ULTOA_BUF_SIZ);
Expand Down Expand Up @@ -4615,7 +4595,7 @@ tfw_http_resp_get_conn_flags(TfwHttpResp *resp)
* headers will be avoided.
*/
static int
tfw_http_resp_set_empty_skb_head(TfwHttpResp *resp, TfwHttpMsgCleanup *cleanup)
tfw_http_resp_set_empty_skb_head(TfwHttpResp *resp, TfwSkbCleanup *cleanup)
{
void *opaque_data = TFW_SKB_CB(resp->msg.skb_head)->opaque_data;
TfwMsgIter *iter = &resp->iter;
Expand All @@ -4638,7 +4618,7 @@ tfw_http_resp_set_empty_skb_head(TfwHttpResp *resp, TfwHttpMsgCleanup *cleanup)
}

static int
tfw_h1_resp_cutoff_headers(TfwHttpResp *resp, TfwHttpMsgCleanup *cleanup)
tfw_h1_resp_cutoff_headers(TfwHttpResp *resp, TfwSkbCleanup *cleanup)
{
TfwHttpMsg *hm = (TfwHttpMsg *)resp;
TfwHttpReq *req = resp->req;
Expand Down Expand Up @@ -4736,7 +4716,7 @@ tfw_http_adjust_resp(TfwHttpResp *resp)
TfwHttpReq *req = resp->req;
TfwHttpMsg *hm = (TfwHttpMsg *)resp;
TfwMsgIter *iter = &resp->iter;
TfwHttpMsgCleanup cleanup = {};
TfwSkbCleanup cleanup = {};
const TfwHdrMods *h_mods = tfw_vhost_get_hdr_mods(req->location,
req->vhost,
TFW_VHOST_HDRMOD_RESP);
Expand Down Expand Up @@ -4789,7 +4769,7 @@ tfw_http_adjust_resp(TfwHttpResp *resp)
r = tfw_http_msg_expand_from_pool(hm, &STR_CRLF);

clean:
__tfw_http_free_cleanup(&cleanup);
ss_skb_free_cleanup(&cleanup);

return r;
}
Expand Down Expand Up @@ -5385,7 +5365,7 @@ tfw_h2_hpack_encode_headers(TfwHttpResp *resp, const TfwHdrMods *h_mods)
|| tgt->flags & TFW_STR_TRAILER_HDR)
continue;

r = tfw_hpack_transform(resp, tgt);
r = tfw_hpack_transform(resp, tgt, true);
if (unlikely(r))
return r;
}
Expand Down Expand Up @@ -5883,7 +5863,7 @@ tfw_h2_resp_encode_headers(TfwHttpResp *resp)
TfwHttpReq *req = resp->req;
TfwHttpMsg *hm = (TfwHttpMsg *)resp;
TfwHttpTransIter *mit = &resp->mit;
TfwHttpMsgCleanup cleanup = {};
TfwSkbCleanup cleanup = {};
TfwStr codings = {};
const TfwHdrMods *h_mods = tfw_vhost_get_hdr_mods(req->location,
req->vhost,
Expand Down Expand Up @@ -5985,7 +5965,7 @@ tfw_h2_resp_encode_headers(TfwHttpResp *resp)
req, resp);
SS_SKB_QUEUE_DUMP(&resp->msg.skb_head);

__tfw_http_free_cleanup(&cleanup);
ss_skb_free_cleanup(&cleanup);
return r;
}

Expand Down
16 changes: 1 addition & 15 deletions fw/http.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,19 +343,6 @@ typedef struct {
long m_date;
} TfwHttpCond;

/**
* Represents the data that should be cleaned up after message transformation.
*
* @skb_head - head of skb list that must be freed;
* @pages - pages that must be freed;
* @pages_sz - current number of @pages;
*/
typedef struct {
struct sk_buff *skb_head;
netmem_ref pages[MAX_SKB_FRAGS];
unsigned char pages_sz;
} TfwHttpMsgCleanup;

/**
* HTTP Request.
*
Expand Down Expand Up @@ -404,7 +391,7 @@ struct tfw_http_req_t {
TfwHttpSess *sess;
TfwClient *peer;
void *stale_ce;
TfwHttpMsgCleanup *cleanup;
TfwSkbCleanup *cleanup;
TfwHttpCond cond;
TfwMsgParseIter pit;
HttpTfh tfh;
Expand Down Expand Up @@ -815,5 +802,4 @@ void tfw_http_extract_request_authority(TfwHttpReq *req);
bool tfw_http_mark_is_in_whitlist(unsigned int mark);
char *tfw_http_resp_status_line(int status, size_t *len);
int tfw_h2_on_send_resp(void *conn, struct sk_buff **skb_head);

#endif /* __TFW_HTTP_H__ */
7 changes: 3 additions & 4 deletions fw/http2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,8 @@ tfw_h2_apply_settings_entry(TfwH2Ctx *ctx, unsigned short id,

switch (id) {
case HTTP2_SETTINGS_TABLE_SIZE:
dest->hdr_tbl_sz = min_t(unsigned int,
val, HPACK_ENC_TABLE_MAX_SIZE);
tfw_hpack_set_rbuf_size(&ctx->hpack.enc_tbl, dest->hdr_tbl_sz);
tfw_hpack_set_rbuf_size(&ctx->hpack.enc_tbl, val);
dest->hdr_tbl_sz = ctx->hpack.enc_tbl.window;
break;

case HTTP2_SETTINGS_ENABLE_PUSH:
Expand Down Expand Up @@ -627,7 +626,7 @@ tfw_h2_hpack_encode_trailer_headers(TfwHttpResp *resp)
T_DBG3("%s: hid=%hu, d_num=%hu, nchunks=%u\n",
__func__, hid, d_num, ht->tbl[hid].nchunks);

r = tfw_hpack_transform(resp, tgt);
r = tfw_hpack_transform(resp, tgt, false);
if (unlikely(r))
goto finish;
}
Expand Down
Loading