Skip to content

Commit 1303744

Browse files
Fix several BUGs in hpack.c
- We should check return code of `tfw_hpack_exp_hdr` in `BUFFER_VAL_OPEN` and return error if allocation fails to prevent BUG. - Save `it->next` before call `tfw_hpack_exp_hdr` in `tfw_hpack_huffman_write` and restore `it->next` after this call. There is a small chance that during `tfw_hpack_exp_hdr->tfw_str_add_compound->__str_grow_tree` new pages will be allocated in `__tfw_pool_realloc`. In this case all chunks in it->hdr will be moved and change there addresses, but it->next still points to the old chunk address! (This lead to BUG during processing header values later, when we iterate over all chunks from `it->next`. (See `__hpack_process_hdr_value`) This patch fixes this problem - now it->next not a pointer it is a number of chunk in `it->hdr`. The number of chunk is not changed during call `tfw_hpack_exp_hdr` so even if all chunks moved, the number is correct!.
1 parent b044b43 commit 1303744

File tree

2 files changed

+26
-16
lines changed

2 files changed

+26
-16
lines changed

fw/hpack.c

Lines changed: 23 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -282,7 +282,7 @@ do { \
282282
do { \
283283
(it)->hdr.data = (it)->pos; \
284284
(it)->hdr.len = length; \
285-
(it)->next = &(it)->hdr; \
285+
(it)->next = 0; \
286286
} while (0)
287287

288288
#define BUFFER_NAME_OPEN(length) \
@@ -311,11 +311,15 @@ do { \
311311
r = -ENOMEM; \
312312
goto out; \
313313
} \
314-
if (!TFW_STR_EMPTY(&it->hdr)) \
315-
it->next = tfw_hpack_exp_hdr(req->pool, \
316-
length, it); \
317-
else \
314+
if (!TFW_STR_EMPTY(&it->hdr)) { \
315+
r = tfw_hpack_exp_hdr(req->pool, length, \
316+
it); \
317+
if (unlikely(r)) \
318+
return r; \
319+
it->next = it->hdr.nchunks - 1; \
320+
} else { \
318321
BUFFER_HDR_INIT(length, it); \
322+
} \
319323
} \
320324
} while (0)
321325

@@ -324,10 +328,10 @@ __hpack_process_hdr_name(TfwHttpReq *req)
324328
{
325329
const TfwStr *c, *end;
326330
TfwMsgParseIter *it = &req->pit;
327-
const TfwStr *hdr = &it->hdr, *next = it->next;
331+
const TfwStr *hdr = &it->hdr;
332+
const TfwStr *next = TFW_STR_CHUNK(hdr, it->next);
328333
int ret = -EINVAL;
329334

330-
WARN_ON_ONCE(next != hdr);
331335
TFW_STR_FOR_EACH_CHUNK(c, next, end) {
332336
bool last = c + 1 == end;
333337

@@ -344,7 +348,8 @@ __hpack_process_hdr_value(TfwHttpReq *req)
344348
{
345349
const TfwStr *chunk, *end;
346350
TfwMsgParseIter *it = &req->pit;
347-
const TfwStr *hdr = &it->hdr, *next = it->next;
351+
const TfwStr *hdr = &it->hdr;
352+
const TfwStr *next = TFW_STR_CHUNK(hdr, it->next);
348353
int ret = -EINVAL;
349354

350355
BUG_ON(TFW_STR_DUP(hdr));
@@ -431,27 +436,28 @@ write_int(unsigned long index, unsigned short max, unsigned short mask,
431436
res_idx->sz = size;
432437
}
433438

434-
static inline TfwStr *
439+
static int
435440
tfw_hpack_exp_hdr(TfwPool *__restrict pool, unsigned long len,
436441
TfwMsgParseIter *__restrict it)
437442
{
438443
TfwStr *new;
439444

440445
if (!(new = tfw_str_add_compound(pool, &it->hdr)))
441-
return NULL;
446+
return -ENOMEM;
442447

443448
new->data = it->pos;
444449
new->len = len;
445450
it->hdr.len += len;
446451

447-
return new;
452+
return 0;
448453
}
449454

450455
static inline int
451456
tfw_hpack_huffman_write(char sym, TfwHttpReq *__restrict req)
452457
{
453-
bool np;
454458
TfwMsgParseIter *it = &req->pit;
459+
bool np;
460+
int r;
455461

456462
if (it->rspace) {
457463
--it->rspace;
@@ -481,7 +487,11 @@ tfw_hpack_huffman_write(char sym, TfwHttpReq *__restrict req)
481487
return 0;
482488
}
483489

484-
return tfw_hpack_exp_hdr(req->pool, 1, it) ? 0 : -ENOMEM;
490+
r = tfw_hpack_exp_hdr(req->pool, 1, it);
491+
if (unlikely(r))
492+
return r;
493+
494+
return 0;
485495
}
486496

487497
static int

fw/msg.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,11 +74,11 @@ typedef struct {
7474
* @pos - pointer to the currently allocated chunk of decoded headers'
7575
* buffer;
7676
* @rspace - space remained in the allocated chunk;
77-
* @next - pointer to the decoded header part (name/value) to be
78-
* - parsed next;
7977
* @nm_len - length of the decoded header's name;
8078
* @nm_num - chunks number of the decoded header's name;
8179
* @tag - tag of currently processed decoded header.
80+
* @next - number of chunk which points to the decoded header part
81+
* (name/value) to be parsed next;
8282
*/
8383
typedef struct {
8484
TfwPool *pool;
@@ -89,10 +89,10 @@ typedef struct {
8989
TfwStr hdr;
9090
char *pos;
9191
unsigned long rspace;
92-
TfwStr *next;
9392
unsigned long nm_len;
9493
unsigned int nm_num;
9594
unsigned int tag;
95+
unsigned int next;
9696
} TfwMsgParseIter;
9797

9898
int tfw_msg_write(TfwMsgIter *it, const TfwStr *data);

0 commit comments

Comments
 (0)