fix content-length and chunk-size overflow test #170

Closed
wants to merge 1 commit into from

2 participants

@orangemocha
Node.js Foundation member

The overflow check didn't work for all possible inputs.

@orangemocha orangemocha fix content-length and chunk-size overflow test
The overflow check didn't work for all possible inputs.
410283d
@indutny indutny commented on the diff Jan 27, 2014
http_parser.c
@@ -1509,8 +1509,8 @@ size_t http_parser_execute (http_parser *parser,
t *= 10;
t += ch - '0';
- /* Overflow? */
- if (t < parser->content_length || t == ULLONG_MAX) {
+ /* Overflow? Test against a conservative limit for simplicity. */
+ if ((ULLONG_MAX - 10) / 10 < parser->content_length) {
@indutny
Node.js Foundation member
indutny added a note Jan 27, 2014

May be ULLONG_MAX / 10 - 1 ?

@orangemocha
Node.js Foundation member

Well it's the same but this way it's more obvious where the number comes from: if you multiply by 10 and add a decimal digit, you cannot possibly reach ULLONG_MAX.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@indutny indutny commented on the diff Jan 27, 2014
http_parser.c
@@ -1782,8 +1782,8 @@ size_t http_parser_execute (http_parser *parser,
t *= 16;
t += unhex_val;
- /* Overflow? */
- if (t < parser->content_length || t == ULLONG_MAX) {
+ /* Overflow? Test against a conservative limit for simplicity. */
+ if ((ULLONG_MAX - 16) / 16 < parser->content_length) {
@indutny
Node.js Foundation member
indutny added a note Jan 27, 2014

Same here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@indutny
Node.js Foundation member

Few style comments, otherwise LGTM! Thank you!

@indutny
Node.js Foundation member

Ok, landed with test fixes in a252d4e

@indutny indutny closed this Jan 27, 2014
@indutny
Node.js Foundation member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment