Closed indutny closed 3 years ago
cc @addaleax as well since you are so good at C/C++!
Does it affect 32-bit builds of Node.js too?
Does it affect 32-bit builds of Node.js too?
I believe it does.
+1 for fast-tracking this.
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.
Landed in 037dd2f. Thanks y'all!
Landed in 037dd2f. Thanks y'all!
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?
Or is it just a matter of rebuilding Node and it will use the latest version of llparse? :)
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!
On 32bit platforms
size_t
is essentiallyuint32_t
(or at times even meageruint16_t
). Loadinguint64_t
field value intosize_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.