search

nodejs / http-parser

http request/response parser for c
MIT License
6.32k stars 1.53k forks source link

2.9.3 breaks ABI compatibility with 2.9.2 with no corresponding SONAME change #502

Closed maxcrees closed 4 years ago

maxcrees commented 4 years ago

Hello,

After spending quite some time debugging quassel-webserver breaking on my latest system update, I traced the issue back to the upgrade of http-parser from 2.9.2 to 2.9.3 without a corresponding rebuild of node itself. Node wasn't rebuilt because the SONAME of http-parser didn't change.

However, it is clear that the changes introduced in 7d5c99d09f6743b055d53fc3f642746d9801479b horribly broke the ABI of libhttp_parser.so. This is due to the changes made to enum http_errno and struct http_parser (the changes made to enum header_states are also breaking but I'm assuming this is not ABI). The abi-laboratory report gives a nice summary of this.

The problem I encountered was any request to the app would immediately return a Bad Request, but I figure this can manifest in any number of ways - for example, the Fedora people saw that it made libgit2 segfault.

Please consider cutting a new release with an updated SONAME or otherwise making the changes ABI compatible.

Thanks,

Max

bnoordhuis commented 4 years ago

Paging @indutny and also @jasnell, @mcollina and @sam-github.

It seems like a pretty obvious oversight (I caught it immediately while looking at the diff so I'm pretty disappointed no one else did...) so please figure out how to fix this.

mcollina commented 4 years ago

Sigh :(.

indutny commented 4 years ago

FWIW, I've marked relevant PR as an ABI change. Looks like the details went missing somewhere in the process. Should we cut 2.9.4 with the revert and then 2.10.0?

Sorry!

jasnell commented 4 years ago

As @indutny mentions, this was clearly an abi breaking change. Unfortunate that that was missed in the release process. A revert plus new 2.10.0 should work I think.

bnoordhuis commented 4 years ago

Should we cut 2.9.4 with the revert and then 2.10.0?

Just to be clear, 2.10.0 would contain the ABI change again? With a SONAME bump? What happens when a security vulnerability is found that affects 2.9.4?

I think the change from 7d5c99d can be done in a backwards compatible way. PTAL y'all at #503.

bnoordhuis commented 4 years ago

Fixed in 714cbb2 and released as v2.9.4.