[go: up one dir, main page]

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.

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

CRITICAL LibUV Bug Kills whole server before route execution #10241

Open
AjaniBilby opened this issue Nov 18, 2024 · 7 comments
Open

CRITICAL LibUV Bug Kills whole server before route execution #10241

AjaniBilby opened this issue Nov 18, 2024 · 7 comments

Comments

@AjaniBilby
Copy link
AjaniBilby commented Nov 18, 2024

Edited to summarise so far

There are potentially two issues, one which can only take down a development server, and once which can take down a production remix.js * express.js server (haven't tested on other adapters).

Both issues trigger the same libuv assertion which is not handled anywhere causing the entire node instance to exit.
The lowest JS function which ends up calling the libuv function has not yet be found and may not be part of remix.js, but it does affect remix.js servers so I'll keep this issue open until the true fault is found.

Note, this crash is causing the server to close so fast that the console.log buffers are not flushed which makes finding the culpret function quite annoying.

Reproduction

  1. Clone AjaniBilby/remix-lib-uv-assert
  2. Install deps npm i

Malformed URL

  1. Start the development server npm run dev
  2. Make the following curl request
curl -i -X POST \
   -d \
'{}' \
'http://localhost:3000/api/address/validate/%E1%9C%84%C8%BAy%F0%90%9E%B2:%F0%9E%A2%A2%F0%98%B4%87%F0%90%80%80'%C2%A53%CC%9E[%3Ci$?%F0%9C%BC%86%DF%8E%EF%BF%BDC%EF%BF%BD%E2%80%97%F0%91%84%BC%E3%81%B6%27'

Fails an assertion in what I assume is libuv, however it appears to fail so rapidly that morgan doesn't get to log the request.

App listening on http://localhost:3000
Assertion failed: w_target_len == 0, file c:\ws\deps\uv\src\idna.c, line 408

Malformed Packet

  1. Start the production server npm run start or dev `npm run dev
  2. Send the corrupted packet node kill.js
> cross-env NODE_ENV=production node ./server.js

App listening on http://localhost:3000
Assertion failed: w_target_len == 0, file c:\ws\deps\uv\src\idna.c, line 408

System Info

System:
    OS: Windows 11 10.0.22631
    CPU: (22) x64 Intel(R) Core(TM) Ultra 7 155H
    Memory: 4.30 GB / 15.74 GB
  Binaries:
    Node: 22.2.0 - C:\Program Files\nodejs\node.EXE
    npm: 10.9.0 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: Chromium (128.0.2739.42)
    Internet Explorer: 11.0.22621.3527
@AjaniBilby AjaniBilby changed the title CRITICAL LibUV Bug Kills whole server before route execution LibUV Bug Kills whole server before route execution Nov 18, 2024
@AjaniBilby
Copy link
Author
AjaniBilby commented Nov 18, 2024

I was having the issue occur before on production and dev, but the repo I linked only has it occuring on dev, which of course would be much less of a worry, and is possibly a vite problem if it's dev only.

Edit: I can get this error in production to occur via my fuzz testing, I'm just having issue isolating the test to get it to occur cleanly in the sample repo

@AjaniBilby AjaniBilby changed the title LibUV Bug Kills whole server before route execution CRITICAL LibUV Bug Kills whole server before route execution Nov 18, 2024
@AjaniBilby
Copy link
Author

Affected LibUV Source seems to be a UTF-8 to UTF-16 conversion issue

@AjaniBilby
Copy link
Author

With openapi-fuzzer installed you can kill a production remix.js server which has no routes defined. I still haven't pin-pointed the exact offending request yet, but the setup in the source will hit it eventually.

npm run build & npm run start
npm run fuzz:all

Since it is a libuv utf8 -> utf16 issue, I doubt it something directly in the remix.js code, however it does affect all remix.js express servers so I will keep this issue open until I can find the closest NodeJS dependency to this issue.
It possibly might also affect not express.js remix servers, though I haven't tested that yet.

@AjaniBilby
Copy link
Author
AjaniBilby commented Nov 19, 2024

Found an exact packet that will consistently kill a production remix.js express.js server
source

Note if you stringify the buffer then send the string instead of the raw buffer the server will not die.
It is very specific to a certain byte encoding

Edit: It seems to be a combination of the URL path + API-KEY header in that packet that creates the issue, because if you reduce the path to something that makes sense the bug goes away, but if you restore it and also remove that header it also resolves the issue, so it's clearly a conjunction of the two creating this issue

@AjaniBilby
Copy link
Author

Removing these lines stops the issue from occuring:

app.use(
  viteDevServer
    ? viteDevServer.middlewares
    : express.static("build/client")
);

However leaving app.use(morgan("tiny")); does not affect it which implies it's not express.js, and instead is an issue present in viteDevServer.middlewares and what ever express.static generates, though I don't know what express does here.

@AjaniBilby
Copy link
Author
AjaniBilby commented Nov 19, 2024

I tracked down the offending cause, and confirmed by removing this call it does fix one fuzz test case, however I believe this issue should remain open until a patch is available because it doesn't explain how the other http headers were affecting this, and there is likely another issue also occuring, but is hard to find while there isn't a patch for this case.

remix.js -> express.js -> serve-static -> send.SendStream.sendFile:L629-L634

fs.stat(p, function (err, stat) {
  if (err) return next(err)
  if (stat.isDirectory()) return next()
  self.emit('file', p, stat)
  self.send(p, stat)
})

See nodejs/node/issues#55914

@AjaniBilby
Copy link
Author

NVM, actually the API-KEY header might have been a red-herring.
It's up to the maintainers whether or not this issue should remain open as it's not an issue directly with remix.js, but does affect it quite spectacularly until patched.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant