Skip to content

Commit

Permalink
src: fix slice of slice of file-backed Blob
Browse files Browse the repository at this point in the history
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: #53908
PR-URL: #53972
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
jleedev authored Jul 22, 2024
1 parent 1c5fe04 commit 17fb18d
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 2 deletions.
6 changes: 4 additions & 2 deletions src/dataqueue/queue.cc
Original file line number Diff line number Diff line change
Expand Up @@ -840,7 +840,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<DataQueue::Reader> get_reader() override {
return ReaderImpl::Create(this);
Expand All @@ -851,7 +853,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_);
Expand Down
10 changes: 10 additions & 0 deletions test/parallel/test-blob-file-backed.js
Original file line number Diff line number Diff line change
Expand Up @@ -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());

Expand Down

0 comments on commit 17fb18d

Please sign in to comment.