Response body ends prematurely #803

ronag · 2021-05-11T15:27:46Z

~~Haven't had time to check but I suspect we might have a problem with body sizes larger than 31 bit.~~

undici will complete without error before reading entire response body.

ronag · 2021-05-11T15:27:57Z

@dnlup would you mind taking a look and maybe making a test?

mcollina · 2021-05-11T15:33:04Z

How does Node.js handle this case?

ronag · 2021-05-11T15:46:13Z

It just works. This might be a wasm thing.

dnlup · 2021-05-11T15:46:56Z

Are we talking about something greater than Number.MAX_SAFE_INTEGER?

ronag · 2021-05-11T15:55:09Z

Are we talking about something greater than Number.MAX_SAFE_INTEGER?

No, I have a case where I'm unable to receive anything larger than ~2^31. I'm not sure if it's undici at the moment but I think it's good idea to have a test where we transfer more than 2^32 bytes.

ronag · 2021-05-11T16:16:49Z

Could also be a problem with Node 16 http1/http2 server. I'm seeing the issue in a reverse proxy in production.

dnlup · 2021-05-11T16:47:42Z

Inspecting llhttp I see content-length is an unsigned int of 64 bits. That should exclude some issues in the parser native side.

dnlup · 2021-05-11T18:21:16Z

I think the problem is with Node. It happens only if you pass the entire buffer to res.write. Undici errors with message The other side closed.

I used Node 16 to test.

ronag · 2021-05-11T22:03:43Z

Something is weird. undici always prematurely ends response at 1605405800 bytes without error. While curl properly reads all 5 GB.

ronag · 2021-05-11T22:23:34Z

@mcollina @dnlup

This fails: #805

ronag · 2021-05-11T22:24:35Z

@mcollina we should probably fix this before release.

ronag · 2021-05-11T22:27:20Z

@indutny How come llhttp ends message without error before outputting content-length number of bytes?

ronag · 2021-05-11T22:46:02Z

going back to undici@3 resolves the issue.

ronag · 2021-05-11T23:05:01Z

Currently I'm at a loss on cause and fix for this.

indutny · 2021-05-12T03:42:43Z

Isn't wasm memory space limited to ~~2gb~~? I think you need to split up the data before passing it into llhttp.

Correction: 4gb

ronag · 2021-05-12T05:03:05Z

The data is split? See the test.

indutny · 2021-05-12T05:21:40Z

Looking. Completely unrelated, you should probably use parseInt(..., 10) instead of just parseInt() (Yes, JS is weird: parseInt('0x1f') === 0x1f)

indutny · 2021-05-12T05:40:06Z

llhttp itself appears to work fine: https://gist.github.com/indutny/818e4428eeadb17bb541a668e653e2a9 . Whenever I execute it - I get back exactly the same amount of bytes that passed to execute.

I strongly suspect that it is either an issue with wasm build or the wrapping around it.

ronag · 2021-05-12T05:45:01Z

@indutny Thanks for confirming. Much appreciated.

ronag · 2021-05-12T05:53:13Z

@indutny I ported your test into our WebASsembly and was able to reproduce the issue: https://gist.github.com/ronag/3e316e78931f8b49f1138460a77a258a

indutny · 2021-05-12T05:54:02Z

Added a bunch of logging and looks like uint64_t subtraction doesn't work:

parser.content_length=0x100028c68 on_body_len=65536 read_total=1605238784
parser.content_length=0x100018c68 on_body_len=65536 read_total=1605304320
parser.content_length=0x100008c68 on_body_len=65536 read_total=1605369856
parser.content_length=0x0 on_body_len=35944 read_total=1605405800

As you might have guessed 35944 is 0x8c68 in hex. So it subtracts 0x8c68 from 0x100008c68 and gets 0x0.

indutny · 2021-05-12T05:54:28Z

FWIW, here's the code that I've added to wasm_on_body to extract this:

      const _cl = new Uint32Array(llhttp.exports.memory.buffer, p + 8 * 4, 2);
      const _remaining = _cl[1] * 0x100000000 + _cl[0];
      console.log('parser.content_length=0x%s on_body_len=%d read_total=%d', _remaining.toString(16), len, _total += len);

indutny · 2021-05-12T05:55:47Z

What's the best way to view the contents of the wasm file?

indutny · 2021-05-12T05:57:38Z

Sounds like wasm2wat

ronag · 2021-05-12T05:57:47Z

What's the best way to view the contents of the wasm file?

Maybe https://webassembly.studio?

indutny · 2021-05-12T06:01:09Z

I know what's going on. Could you find s_n_llhttp__internal__n_consume_content_length replace size_ts with uint64_ts right after it and try recompiling and running the test?

ronag · 2021-05-12T06:07:20Z

That fixed it!

indutny · 2021-05-12T06:08:44Z

Alright, working on the real fix. Thanks for trying it.

On 32bit platforms `size_t` is essentially `uint32_t` (or at times even meager `uint16_t`). Loading `uint64_t` field value into `size_t` on these platforms would truncate the high bits and leave only the low 32 (16) bits in place. This leads to various interesting errors in downstream modules. See: - nodejs/llhttp#110 - nodejs/undici#803

indutny · 2021-05-12T06:31:11Z

Fix: nodejs/llparse#44 .

@ronag could you try llhttp.c from the attachment and see if it still works?

llhttp.c.zip

indutny · 2021-05-12T06:32:05Z

FWIW, this is the file that I intend to release once llparse fix lands. Feel free to ship the wasm version built from this file (after reviewing the diff) until we'll get it pushed upstream!

On 32bit platforms `size_t` is essentially `uint32_t` (or at times even meager `uint16_t`). Loading `uint64_t` field value into `size_t` on these platforms would truncate the high bits and leave only the low 32 (16) bits in place. This leads to various interesting errors in downstream modules. See: - nodejs/llhttp#110 - nodejs/undici#803 This patch makes all field loads go into their respective types. Truncation doesn't happen in this case because C coercion rules will cast both types to the largest necessary datatype to hold either of them.

ronag · 2021-05-12T06:35:23Z

Fix: nodejs/llparse#44 .

@ronag could you try llhttp.c from the attachment and see if it still works?

llhttp.c.zip

All good!

dnlup · 2021-05-12T06:40:56Z

Thank you for the help @indutny !

indutny · 2021-05-12T06:47:16Z

No worries. Always glad to fix my own mess ups 😂

…

On Tue, May 11, 2021 at 23:41 Daniele Belardi ***@***.***> wrote: Thank you for the help @indutny <https://github.com/indutny> ! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#803 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB2HQ632KFBEM2ZR2JGJVTTNIPI3ANCNFSM44V2Z4ZQ> .

On 32bit platforms `size_t` is essentially `uint32_t` (or at times even meager `uint16_t`). Loading `uint64_t` field value into `size_t` on these platforms would truncate the high bits and leave only the low 32 (16) bits in place. This leads to various interesting errors in downstream modules. See: - nodejs/llhttp#110 - nodejs/undici#803 This patch makes all field loads go into their respective types. Truncation doesn't happen in this case because C coercion rules will cast both types to the largest necessary datatype to hold either of them. PR-URL: #44 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Daniele Belardi <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

indutny · 2021-05-12T21:59:48Z

Created a new release: https://github.com/nodejs/llhttp/releases/tag/release%2Fv6.0.2

ronag added the bug Something isn't working label May 11, 2021

ronag added this to the 4.0 milestone May 11, 2021

dnlup mentioned this issue May 11, 2021

test: add content-length greather than 2^31 #804

Closed

ronag closed this as completed May 11, 2021

ronag reopened this May 11, 2021

ronag mentioned this issue May 11, 2021

fix: premature end large body #805

Merged

ronag changed the title ~~broken for content-length > 31 bits?~~ Response body ends prematurely May 11, 2021

ronag added the Status: help-wanted This issue/pr is open for contributions label May 11, 2021

indutny mentioned this issue May 12, 2021

download of compressed files on arm-plattforms nodejs/llhttp#110

Closed

indutny mentioned this issue May 12, 2021

c: use proper type coercion in consume nodejs/llparse#44

Closed

ronag closed this as completed May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Response body ends prematurely #803

Response body ends prematurely #803

ronag commented May 11, 2021 •

edited

Loading

ronag commented May 11, 2021

mcollina commented May 11, 2021

ronag commented May 11, 2021

dnlup commented May 11, 2021

ronag commented May 11, 2021 •

edited

Loading

ronag commented May 11, 2021 •

edited

Loading

dnlup commented May 11, 2021

dnlup commented May 11, 2021 •

edited

Loading

ronag commented May 11, 2021 •

edited

Loading

ronag commented May 11, 2021 •

edited

Loading

ronag commented May 11, 2021

ronag commented May 11, 2021 •

edited

Loading

ronag commented May 11, 2021

ronag commented May 11, 2021

indutny commented May 12, 2021 •

edited

Loading

ronag commented May 12, 2021 •

edited

Loading

indutny commented May 12, 2021

indutny commented May 12, 2021

ronag commented May 12, 2021

ronag commented May 12, 2021

indutny commented May 12, 2021

indutny commented May 12, 2021

indutny commented May 12, 2021

indutny commented May 12, 2021

ronag commented May 12, 2021

indutny commented May 12, 2021

ronag commented May 12, 2021

indutny commented May 12, 2021

indutny commented May 12, 2021

indutny commented May 12, 2021 •

edited

Loading

ronag commented May 12, 2021

dnlup commented May 12, 2021

indutny commented May 12, 2021 via email

indutny commented May 12, 2021

Response body ends prematurely #803

Response body ends prematurely #803

Comments

ronag commented May 11, 2021 • edited Loading

ronag commented May 11, 2021

mcollina commented May 11, 2021

ronag commented May 11, 2021

dnlup commented May 11, 2021

ronag commented May 11, 2021 • edited Loading

ronag commented May 11, 2021 • edited Loading

dnlup commented May 11, 2021

dnlup commented May 11, 2021 • edited Loading

ronag commented May 11, 2021 • edited Loading

ronag commented May 11, 2021 • edited Loading

ronag commented May 11, 2021

ronag commented May 11, 2021 • edited Loading

ronag commented May 11, 2021

ronag commented May 11, 2021

indutny commented May 12, 2021 • edited Loading

ronag commented May 12, 2021 • edited Loading

indutny commented May 12, 2021

indutny commented May 12, 2021

ronag commented May 12, 2021

ronag commented May 12, 2021

indutny commented May 12, 2021

indutny commented May 12, 2021

indutny commented May 12, 2021

indutny commented May 12, 2021

ronag commented May 12, 2021

indutny commented May 12, 2021

ronag commented May 12, 2021

indutny commented May 12, 2021

indutny commented May 12, 2021

indutny commented May 12, 2021 • edited Loading

ronag commented May 12, 2021

dnlup commented May 12, 2021

indutny commented May 12, 2021 via email

indutny commented May 12, 2021

ronag commented May 11, 2021 •

edited

Loading

ronag commented May 11, 2021 •

edited

Loading

ronag commented May 11, 2021 •

edited

Loading

dnlup commented May 11, 2021 •

edited

Loading

ronag commented May 11, 2021 •

edited

Loading

ronag commented May 11, 2021 •

edited

Loading

ronag commented May 11, 2021 •

edited

Loading

indutny commented May 12, 2021 •

edited

Loading

ronag commented May 12, 2021 •

edited

Loading

indutny commented May 12, 2021 •

edited

Loading