Unverified Commit 67a4ce1c authored by Jamie Davis's avatar Jamie Davis Committed by Ruben Bridgewater
Browse files

fs: partition readFile against pool exhaustion

Problem:

Node implements fs.readFile as:
- a call to stat, then
- a C++ -> libuv request to read the entire file using the stat size

Why is this bad?
The effect is to place on the libuv threadpool a potentially-large
read request, occupying the libuv thread until it completes.
While readFile certainly requires buffering the entire file contents,
it can partition the read into smaller buffers
(as is done on other read paths)
along the way to avoid threadpool exhaustion.

If the file is relatively large or stored on a slow medium, reading
the entire file in one shot seems particularly harmful,
and presents a possible DoS vector.

Solution:

Partition the read into multiple smaller requests.

Considerations:

1. Correctness

I don't think partitioning the read like this raises
any additional risk of read-write races on the FS.
If the application is concurrently readFile'ing and modifying the file,
it will already see funny behavior. Though libuv uses preadv where
available, this doesn't guarantee read atomicity in the presence of
concurrent writes.

2. Performance

Downside: Partitioning means that a single large readFile will
  require into many "out and back" requests to libuv,
  introducing overhead.
Upside: In between each "out and back", other work pending on the
  threadpool can take a turn.

In short, although partitioning will slow down a large request,
it will lead to better throughput if the threadpool is handling
more than one type of request.

Fixes: https://github.com/nodejs/node/issues/17047

PR-URL: https://github.com/nodejs/node/pull/17054


Reviewed-By: default avatarBenjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: default avatarTiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: default avatarGireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: default avatarJames M Snell <jasnell@gmail.com>
Reviewed-By: default avatarMatteo Collina <matteo.collina@gmail.com>
Reviewed-By: default avatarSakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: default avatarRuben Bridgewater <ruben@bridgewater.de>
parent 36fd25fa
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment