This repository has been archived by the owner. It is now read-only.

fs.readSync(0, new Buffer(0), 0, 0, 0) throws Error: Offset is out of bounds #5685

Open
eahutchins opened this Issue Jun 13, 2013 · 10 comments

Comments

Projects
None yet
5 participants

Reading 0 bytes into a 0 sized buffer should be a no-op to make it more like read(2):

If count is zero, read() returns zero and has no other results.

fs.readSync(0, new Buffer(1), 0, 0, 0) fails with Error: ESPIPE, invalid seek on stdin, but a real empty file works and returns 0.

glasser commented Jul 22, 2013

Just hit this issue as well.

glasser commented Jul 22, 2013

Some notes: fs.read has the same issue. fs.write and fs.writeSync explicitly check for length 0 and do an early return.

http://pubs.opengroup.org/onlinepubs/9699919799/functions/read.html is the POSIX read spec.

isaacs added a commit to isaacs/node-v0.x-archive that referenced this issue Jul 23, 2013

isaacs commented Jul 24, 2013

Working as intended. Please see discussion in #5888.

@isaacs isaacs closed this Jul 24, 2013

isaacs commented Jul 24, 2013

Ah, ok, so the issue in the OP is still an issue, though.

@isaacs isaacs reopened this Jul 24, 2013

glasser commented Jul 24, 2013

Yeah, so:

  • If we want to support being able to get errors out of read(0), then we can implement that (but you currently can't do that). When I first looked at the code in node_file.cc, I thought that you could just remove the (off >= buffer_length) check (and rely purely on the (off + len > buffer_length) check). But that might run into integer overflow issues, and admittedly the error message is better. Perhaps just change the check to (off > buffer_length)? The idea here would be, it's OK for the block we're trying to read to start anywhere in the buffer, including at the end.
  • If we don't want to support that, then the length > 0 check in read is fine.

Why wouldn't

offset <= buffer_length
length <= buffer_length
offset + length <= buffer_length

be sufficient?
On Jul 23, 2013 5:29 PM, "David Glasser" notifications@github.com wrote:

Yeah, so:

If we want to support being able to get errors out of read(0), then we
can implement that (but you currently can't do that). When I first looked
at the code in node_file.cc, I thought that you could just remove the (off

= buffer_length) check (and rely purely on the (off + len >
buffer_length) check). But that might run into integer overflow
issues, and admittedly the error message is better. Perhaps just change the
check to (off > buffer_length)? The idea here would be, it's OK for
the block we're trying to read to start anywhere in the buffer,
including at the end.

If we don't want to support that, then the length > 0 check in read is
fine.


Reply to this email directly or view it on GitHubhttps://github.com/joyent/node/issues/5685#issuecomment-21456240
.

glasser commented Jul 24, 2013

That sounds right to me, but I just try not to make assertions about safety in the face of overflow without thinking very carefully :)

@vStone vStone referenced this issue in rlidwka/sinopia-htpasswd Sep 30, 2014

Closed

Offset is out of bounds with an empty htpasswd file #2

Owner

jasnell commented Jun 22, 2015

@trevnorris ... any further thoughts on this one?

@jasnell If it hasn't been fixed yet then should be easy to do. Just need to change the range checks, and return early if the operation is a noop.

@robertknight robertknight referenced this issue in robertknight/xar-js Sep 4, 2015

Closed

exception on empty files #2

robertknight added a commit to robertknight/xar-js that referenced this issue Sep 4, 2015

Fix error when adding empty file to archive.
fs.readSync() reports an error under Node <= 0.12.x
when reading 0 bytes from a 0-byte length buffer.

See nodejs/node-v0.x-archive#5685

Fixes #2
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.