From 4c36d6c47ab6618cb1f3b07bd59a4b12528ea6e7 Mon Sep 17 00:00:00 2001 From: Josh Lee Date: Mon, 22 Jul 2024 14:25:30 -0400 Subject: [PATCH] src: fix slice of slice of file-backed Blob The value for `new_end` was wrong: While the members `start_` and `end_` refer to the entire length of the file, the parameters `start` and `end` are relative to the current slice. The new end would apparently have the current start_ subtracted from it, and the length would possibly overflow when the FdEntry is asked for its size or when get_reader is called, resulting in a subslice which extends past the current slice, which shouldn't be possible. Add a CHECK if this happens, rather than returning data outside the current slice. There aren't any C++ tests for FdEntry, and on the javascript side there isn't a way to ask the blob handle for its nominal size. That size could be a large uint64, which gets converted to int64 to when FileHandle::new is called, which interprets a negative length as unlimited. Fixes: https://github.com/nodejs/node/issues/53908 PR-URL: https://github.com/nodejs/node/pull/53972 Reviewed-By: Luigi Pinca Reviewed-By: Antoine du Hamel Reviewed-By: James M Snell --- src/dataqueue/queue.cc | 6 ++++-- test/parallel/test-blob-file-backed.js | 10 ++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/dataqueue/queue.cc b/src/dataqueue/queue.cc index 994b82a8751f6e..f9e2be028c123f 100644 --- a/src/dataqueue/queue.cc +++ b/src/dataqueue/queue.cc @@ -806,7 +806,9 @@ class FdEntry final : public EntryImpl { path_(std::move(path_)), stat_(stat), start_(start), - end_(end) {} + end_(end) { + CHECK_LE(start, end); + } std::shared_ptr get_reader() override { return ReaderImpl::Create(this); @@ -817,7 +819,7 @@ class FdEntry final : public EntryImpl { uint64_t new_start = start_ + start; uint64_t new_end = end_; if (end.has_value()) { - new_end = std::min(end.value(), end_); + new_end = std::min(end.value() + start_, end_); } CHECK(new_start >= start_); diff --git a/test/parallel/test-blob-file-backed.js b/test/parallel/test-blob-file-backed.js index 24560d9215178c..2e143e6936f748 100644 --- a/test/parallel/test-blob-file-backed.js +++ b/test/parallel/test-blob-file-backed.js @@ -86,6 +86,16 @@ writeFileSync(testfile5, ''); const res1 = blob.slice(995, 1005); strictEqual(await res1.text(), data.slice(995, 1005)); + + // Refs: https://github.com/nodejs/node/issues/53908 + for (const res2 of [ + blob.slice(995, 1005).slice(), + blob.slice(995).slice(0, 10), + blob.slice(0, 1005).slice(995), + ]) { + strictEqual(await res2.text(), data.slice(995, 1005)); + } + await unlink(testfile2); })().then(common.mustCall());