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

Incorrect slice() from fs.openAsBlob() #53908

Closed
jleedev opened this issue Jul 17, 2024 · 6 comments · Fixed by #53972
Closed

Incorrect slice() from fs.openAsBlob() #53908

jleedev opened this issue Jul 17, 2024 · 6 comments · Fixed by #53972
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system.

Comments

@jleedev
Copy link
Contributor

jleedev commented Jul 17, 2024

Version

v22.5.0

Platform

Linux penguin 6.6.32-02873-ge0e03a627715 #1 SMP PREEMPT Mon, 1 Jul 2024 20:29:42 +0000 aarch64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

> await fs.promises.writeFile('a.txt', 'abcdefghij')
> blob = await fs.openAsBlob('a.txt')
Blob { size: 10, type: '' }
> await _.text()
'abcdefghij'
> blob.slice(3)
Blob { size: 7, type: '' }
> await _.text()
'defghij'
> blob.slice(3).slice()
Blob { size: 7, type: '' }
> await _.text()
'defg'

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior? Why is that the expected behavior?

slice() with no arguments should be equivalent to slice(0, length) and return the entire blob; both of those are incorrectly returning something else.

What do you see instead?

The returned Blob object purports to have the expected size, but when it is read it has the wrong length.

Additional information

No response

@avivkeller
Copy link
Member

avivkeller commented Jul 17, 2024

Reproducible:

import fs from 'node:fs';

// await fs.writeFile('file.txt', '123456789')

const blob = await fs.openAsBlob('file.txt');
console.log(await blob.text());

console.log(await blob.slice(3).text());

console.log(await blob.slice(3).slice().text());
└─$ node index.mjs
123456789
456789
456

@avivkeller avivkeller added the fs Issues and PRs related to the fs subsystem / file system. label Jul 17, 2024
@avivkeller
Copy link
Member

It however does not occur with non-fs blobs:

import { Blob } from 'node:buffer';

const blob = new Blob(['123456789']);

console.log(await blob.text());
console.log(await blob.slice(3).text());
console.log(await blob.slice(3).slice().text());
└─$ node index.mjs
123456789
456789
456789

@HardikGoyal2003
Copy link

Hey @redyetidev, could you assign me this issue? I'd like to work on it, if that's alright.

@avivkeller
Copy link
Member

Feel free to work on it, but issues aren't typically assigned (except when they are self assigned).

If you have an improvement, feel free to submit a PR.

@jleedev
Copy link
Contributor Author

jleedev commented Jul 20, 2024

new_end = std::min(end.value(), end_);

Should be:

 new_end = std::min(end.value() + start_, end_); 

@avivkeller
Copy link
Member

Feel free to submit a PR with suggestions to the Conway.

jleedev added a commit to jleedev/node that referenced this issue Jul 20, 2024
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: nodejs#53908
@aduh95 aduh95 added the confirmed-bug Issues with confirmed bugs. label Jul 20, 2024
nodejs-github-bot pushed a commit that referenced this issue Jul 22, 2024
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>
targos pushed a commit that referenced this issue Jul 28, 2024
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>
RafaelGSS pushed a commit that referenced this issue Aug 5, 2024
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>
marco-ippolito pushed a commit that referenced this issue Aug 19, 2024
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>
marco-ippolito pushed a commit that referenced this issue Aug 19, 2024
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>
marco-ippolito pushed a commit that referenced this issue Aug 19, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants