c: use proper type coercion in consume - #44
Conversation
Sorry, something went wrong.
|
cc @addaleax as well since you are so good at C/C++! |
Sorry, something went wrong.
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.
|
Does it affect 32-bit builds of Node.js too? |
Sorry, something went wrong.
I believe it does. |
Sorry, something went wrong.
mcollina
left a comment
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
|
+1 for fast-tracking this. |
Sorry, something went wrong.
|
FWIW, here's how the generated code looks with this patch: size_t avail;
uint64_t need;
avail = endp - p;
need = state->content_length;
if (avail >= need) {
p += need;
state->content_length = 0;
goto s_n_llhttp__internal__n_span_end_llhttp__on_body;
}
state->content_length -= avail;
return s_n_llhttp__internal__n_consume_content_length;
/* UNREACHABLE */;
abort();Note the uint64_t above. It was size_t for both variables before the patch. |
Sorry, something went wrong.
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 <matteo.collina@gmail.com> Reviewed-By: Daniele Belardi <dwon.dnl@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Hey @indutny thanks for this fix! Could you share any info on how soon this will trickle down into actual Node, and on which versions it will work? |
Sorry, something went wrong.
|
I'm still waiting for llhttp's CI run to complete, and will publish patch version releases after that. Will submit PRs to Node.js right after that. We should be able to have a discussion on the release timing there! |
Sorry, something went wrong.
Fix: #37053 See: nodejs/llparse#44 PR-URL: #38665 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Daniele Belardi <dwon.dnl@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Fix: #37053 See: nodejs/llparse#44 PR-URL: #38665 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Daniele Belardi <dwon.dnl@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
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:
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.
cc @Trott @mcollina @nodejs/http
I'd appreciate if we could fast-track this. We've been avoiding issues with this for awhile, but both wasm build in unidici and some arm users from the llhttp#110 hit this consistently on large uploads/downloads.