Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

deps: update llhttp to 6.0.2 #38665

Closed
wants to merge 1 commit into from
Closed

deps: update llhttp to 6.0.2 #38665

wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented May 13, 2021 •
edited by aduh95
Loading

Fixes: #37053
Refs: nodejs/llparse#44

Needs backports to all release branches. I've made 4.0.1 release to facilitate with that.

@indutny indutny requested review from mcollina, Trott and addaleax May 13, 2021 02:52
@indutny indutny mentioned this pull request May 13, 2021
Closed
@github-actions github-actions bot added the needs-ci PRs that need a full CI run. label May 13, 2021
@indutny indutny mentioned this pull request May 13, 2021
Closed
@mscdex
Copy link
Contributor

mscdex commented May 13, 2021 •
edited
Loading

@indutny I think that's the wrong link for the "Fixes:" part of the commit message?

@RaisinTen
Copy link
Contributor 

- Fix: #37503
+ Fixes: https://github.com/nodejs/node/issues/37053

mcollina
mcollina approved these changes May 13, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/38057/

@indutny indutny force-pushed the feature/llhttp-6.0.2 branch from 2aae81b to 49e48a1 Compare May 13, 2021 06:58
@indutny
Copy link
Member Author

indutny commented May 13, 2021

Thanks! That was a typo in "Fix". Fixed now! 😂

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/38058/

@richardlau
Copy link
Member

Needs backports to all release branches. I've made 4.0.1 release to facilitate with that.

I take this statement to mean we'd need manual backports for the LTS releases (14/12)? Unfortunately it looks like the way we applied security patches hasn't kept the llhttp files in-step -- current versions of

declare llhttp 2.1.3 and is what you get in node -p process.versions.llhttp.

@targos
Copy link
Member

targos commented May 13, 2021

On master, we jumped directly from v2.1.3 to v5.1.0 in #38146

@legendecas legendecas mentioned this pull request May 13, 2021
Closed
@indutny
Copy link
Member Author

indutny commented May 14, 2021

@richardlau asking the right question! I don't expect any problems with making 2.x release for these branches. Thankfully the fix is not in llhttp itself, but rather in its compiler. 2.x uses [email protected] and the fixed version is [email protected]. Should be as easy as bumping the dependency and making a release. If there are any security fixes - I'll happily apply them as well.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/38121/

indutny added a commit that referenced this pull request May 20, 2021
@indutny
deps: update llhttp to 6.0.2
d798de1 
Fix: #37053
See: nodejs/llparse#44
PR-URL: #38665
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daniele Belardi <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@indutny
Copy link
Member Author

indutny commented May 20, 2021

Landed in d798de1, thank you!

@indutny indutny closed this May 20, 2021
@indutny indutny deleted the feature/llhttp-6.0.2 branch May 20, 2021 19:34
danielleadams pushed a commit that referenced this pull request May 31, 2021
@indutny @danielleadams
deps: update llhttp to 6.0.2
15aaf14 
Fix: #37053
See: nodejs/llparse#44
PR-URL: #38665
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daniele Belardi <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@danielleadams danielleadams mentioned this pull request May 31, 2021
Merged
@richardlau
Copy link
Member

Coming back to this for v14.x and v12.x. I could do with some help here working out what is landable on those re. llhttp. As mentioned before (#38665 (comment)) they both claim to be llhttp 2.1.3 although e.g. on v14.x there was a security patch applied which I think corresponded to a semver major llhttp (3?)

$ git log --oneline deps/llhttp/
641f786bb1 http: unset `F_CHUNKED` on new `Transfer-Encoding`
85062b3aad deps: update llhttp to 2.1.3
...

I guess we missed a step somewhere with the security patches and synching llhttp versions.

Perhaps the best thing to do is mark this as requiring manual backports.

@Chaphasilor Chaphasilor mentioned this pull request Aug 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lioncorpo lioncorpo lioncorpo approved these changes

@dnlup dnlup dnlup approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[linux-armv7l] http.request() fails with HPE_INVALID_CONSTANT error