Skip to content
This repository has been archived by the owner on Nov 6, 2022. It is now read-only.

Bad pointer arithmetic in header field parsing #473

Closed
bnoordhuis opened this issue Apr 15, 2019 · 2 comments · Fixed by #474
Closed

Bad pointer arithmetic in header field parsing #473

bnoordhuis opened this issue Apr 15, 2019 · 2 comments · Fixed by #474

Comments

@bnoordhuis
Copy link
Member

http-parser/http_parser.c

Lines 1259 to 1266 in c5c4563

case h_general: {
size_t limit = data + len - p;
limit = MIN(limit, max_header_size);
while (p+1 < data + limit && TOKEN(p[1])) {
p++;
}
break;
}

What that code calls limit is actually the number of remaining characters.

p+1 < data+limit must be wrong because data+limit is not fixed, it gets smaller as the p on line 1260 gets bigger.

Say, len == 8 and p == data + 5, then data + len - p == 3. Ergo, p+1 < data+limit is always false in that case.

bnoordhuis added a commit to bnoordhuis/http-parser that referenced this issue Apr 15, 2019
The bad arithmetic didn't result in wrong or insecure behavior
(there were no out-of-bound accesses and forward progress was
still being made because of the outer loop) but it did regress
the performance of header field parsing rather massively.

Taking the test suite as an example: the inner fast path loop
was skipped over 4.4 million times, falling back to the outer
slow path loop. After this commit that only happens about
2,000 times - a 2,200-fold reduction.

Fixes: nodejs#473
@indutny
Copy link
Member

indutny commented Apr 15, 2019

Let's do some math:

len = 16000
p - data = 100
max_header_size = 8000
limit = 7900

It seems that there would be some p for which the condition is going to be false?

@bnoordhuis
Copy link
Member Author

IIUC your example, limit gets clamped to 8000 because MIN(16000 - 100, 8000).

bnoordhuis added a commit to bnoordhuis/http-parser that referenced this issue Apr 16, 2019
The bad arithmetic didn't result in wrong or insecure behavior
(there were no out-of-bound accesses and forward progress was
still being made because of the outer loop) but it did regress
the performance of header field parsing rather massively.

Taking the test suite as an example: the inner fast path loop
was skipped over 4.4 million times, falling back to the outer
slow path loop. After this commit that only happens about
2,000 times - a 2,200-fold reduction.

Fixes: nodejs#473
PR-URL: nodejs#474
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
ploxiln pushed a commit to ploxiln/http-parser that referenced this issue Apr 16, 2019
The bad arithmetic didn't result in wrong or insecure behavior,
but it did regress the performance of header field parsing massively.

Taking the test suite as an example: the inner fast path loop
was skipped over 4.4 million times, falling back to the outer
slow path loop. After this commit that only happens about
2,000 times - a 2,200-fold reduction.

Fixes: nodejs#473
PR-URL: nodejs#474

also fix -Wsign-compare warning that manifests itself with gcc 7.4.0+

Fixes: nodejs#471
PR-URL: nodejs#472

Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants