Skip to content

Commit e904107

Browse files
indutnyindutny
committed
http: fix llhttp_finish semantics and add tests
Although implemented in a compatible way `llhttp_finish()` was never properly tested until now. Unsurprisingly, there was an incorrect behavior hidden in there. Fix: #25 PR-URL: #26 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
1 parent 125e44f commit e904107

8 files changed

Lines changed: 553 additions & 47 deletions

File tree

‎package-lock.json‎

Lines changed: 439 additions & 32 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@
3939
"@types/mocha": "^5.2.5",
4040
"@types/node": "^10.12.9",
4141
"llparse-dot": "^1.0.1",
42-
"llparse-test-fixture": "^3.1.0",
42+
"llparse-test-fixture": "^3.3.0",
4343
"mdgator": "^1.1.2",
4444
"mocha": "^5.2.0",
4545
"ts-node": "^7.0.1",

‎src/llhttp/http.ts‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -582,8 +582,8 @@ export class HTTP {
582582
span.body.end(n('message_done')))));
583583

584584
n('body_identity_eof')
585-
.otherwise(span.body.start(
586-
this.update('finish', FINISH.SAFE_WITH_CB, 'eof')));
585+
.otherwise(
586+
this.update('finish', FINISH.SAFE_WITH_CB, span.body.start(n('eof'))));
587587

588588
// Just read everything until EOF
589589
n('eof')
@@ -671,8 +671,9 @@ export class HTTP {
671671
// Check if we'd like to keep-alive
672672
if (this.mode === 'strict') {
673673
n('cleanup')
674-
.otherwise(p.invoke(callback.afterMessageComplete, this.mode === 'strict' ?
675-
{ 1: n('restart') } : {}, n('closed')));
674+
.otherwise(p.invoke(callback.afterMessageComplete, {
675+
1: n('restart'),
676+
}, this.update('finish', FINISH.SAFE, n('closed'))));
676677
} else {
677678
n('cleanup')
678679
.otherwise(p.invoke(callback.afterMessageComplete, n('restart')));
@@ -684,7 +685,7 @@ export class HTTP {
684685
'Data after `Connection: close`'));
685686

686687
n('restart')
687-
.otherwise(n('start'));
688+
.otherwise(this.update('finish', FINISH.SAFE, n('start')));
688689
}
689690

690691
private node<T extends Node>(name: string | T): T {

‎test/fixtures/extra.c‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
#include <stdlib.h>
2+
13
#include "fixture.h"
24

35
int llhttp__on_url(llparse_t* s, const char* p, const char* endp) {
@@ -55,6 +57,11 @@ void llhttp__test_init_response(llparse_t* s) {
5557
}
5658

5759

60+
void llhttp__test_finish(llparse_t* s) {
61+
llparse__print(NULL, NULL, "finish=%d", s->finish);
62+
}
63+
64+
5865
int llhttp__on_status(llparse_t* s, const char* p, const char* endp) {
5966
if (llparse__in_bench)
6067
return 0;

‎test/fixtures/index.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,8 @@ import * as path from 'path';
88

99
import * as llhttp from '../../src/llhttp';
1010

11-
export type TestType = 'request' | 'response' | 'none' | 'url';
11+
export type TestType = 'request' | 'response' | 'request-finish' |
12+
'response-finish' | 'none' | 'url';
1213

1314
export { FixtureResult };
1415

@@ -57,6 +58,13 @@ export function build(llparse: LLParse, node: any, outFile: string,
5758
const extra = options.extra === undefined ? [] : options.extra.slice();
5859
if (ty === 'request' || ty === 'response') {
5960
extra.push(`-DLLPARSE__TEST_INIT=llhttp__test_init_${ty}`);
61+
} else if (ty === 'request-finish' || ty === 'response-finish') {
62+
if (ty === 'request-finish') {
63+
extra.push('-DLLPARSE__TEST_INIT=llhttp__test_init_request');
64+
} else {
65+
extra.push('-DLLPARSE__TEST_INIT=llhttp__test_init_response');
66+
}
67+
extra.push('-DLLPARSE__TEST_FINISH=llhttp__test_finish');
6068
}
6169

6270
return fixtures.build(artifacts, outFile, Object.assign(options, {

‎test/md-test.ts‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -75,16 +75,20 @@ interface IFixtureMap {
7575

7676
const http: IFixtureMap = {
7777
loose: {
78-
none: buildMode('loose', 'none'),
79-
request: buildMode('loose', 'request'),
80-
response: buildMode('loose', 'response'),
81-
url: buildMode('loose', 'url'),
78+
'none': buildMode('loose', 'none'),
79+
'request': buildMode('loose', 'request'),
80+
'request-finish': buildMode('loose', 'request-finish'),
81+
'response': buildMode('loose', 'response'),
82+
'response-finish': buildMode('loose', 'response-finish'),
83+
'url': buildMode('loose', 'url'),
8284
},
8385
strict: {
84-
none: buildMode('strict', 'none'),
85-
request: buildMode('strict', 'request'),
86-
response: buildMode('strict', 'response'),
87-
url: buildMode('strict', 'url'),
86+
'none': buildMode('strict', 'none'),
87+
'request': buildMode('strict', 'request'),
88+
'request-finish': buildMode('strict', 'request-finish'),
89+
'response': buildMode('strict', 'response'),
90+
'response-finish': buildMode('strict', 'response-finish'),
91+
'url': buildMode('strict', 'url'),
8892
},
8993
};
9094

@@ -141,6 +145,10 @@ function run(name: string): void {
141145
types = [ 'request' ];
142146
} else if (meta.type === 'response-only') {
143147
types = [ 'response' ];
148+
} else if (meta.type === 'request-finish') {
149+
types = [ 'request-finish' ];
150+
} else if (meta.type === 'response-finish') {
151+
types = [ 'response-finish' ];
144152
} else {
145153
throw new Error(`Invalid value of \`type\` metadata: "${meta.type}"`);
146154
}
@@ -229,11 +237,13 @@ run('request/connection');
229237
run('request/content-length');
230238
run('request/transfer-encoding');
231239
run('request/invalid');
240+
run('request/finish');
232241

233242
run('response/sample');
234243
run('response/connection');
235244
run('response/content-length');
236245
run('response/transfer-encoding');
237246
run('response/invalid');
247+
run('response/finish');
238248

239249
run('url');

‎test/request/finish.md‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
Finish
2+
======
3+
4+
Those tests check the return codes and the behavior of `llhttp_finish()` C API.
5+
6+
## It should be safe to finish after GET request
7+
8+
<!-- meta={"type": "request-finish"} -->
9+
```http
10+
GET / HTTP/1.1
11+
12+
13+
```
14+
15+
```log
16+
off=0 message begin
17+
off=4 len=1 span[url]="/"
18+
off=18 headers complete method=1 v=1/1 flags=0 content_length=0
19+
off=18 message complete
20+
off=NULL finish=0
21+
```
22+
23+
## It should be unsafe to finish after incomplete PUT request
24+
25+
<!-- meta={"type": "request-finish"} -->
26+
```http
27+
PUT / HTTP/1.1
28+
Content-Length: 100
29+
30+
```
31+
32+
```log
33+
off=0 message begin
34+
off=4 len=1 span[url]="/"
35+
off=16 len=14 span[header_field]="Content-Length"
36+
off=32 len=3 span[header_value]="100"
37+
off=NULL finish=2
38+
```
39+
40+
## It should be unsafe to finish inside of the header
41+
42+
<!-- meta={"type": "request-finish"} -->
43+
```http
44+
PUT / HTTP/1.1
45+
Content-Leng
46+
```
47+
48+
```log
49+
off=0 message begin
50+
off=4 len=1 span[url]="/"
51+
off=16 len=12 span[header_field]="Content-Leng"
52+
off=NULL finish=2
53+
```

‎test/response/finish.md‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
Finish
2+
======
3+
4+
Those tests check the return codes and the behavior of `llhttp_finish()` C API.
5+
6+
## It should be safe to finish with cb after empty response
7+
8+
<!-- meta={"type": "response-finish"} -->
9+
```http
10+
HTTP/1.1 200 OK
11+
12+
13+
```
14+
15+
```log
16+
off=0 message begin
17+
off=13 len=2 span[status]="OK"
18+
off=19 headers complete status=200 v=1/1 flags=0 content_length=0
19+
off=NULL finish=1
20+
```

0 commit comments

Comments
 (0)