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

Fix fs can't handle large file on 64bit platform #1199

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
5 participants

koichik commented Jun 18, 2011

fs.read() and fs.write() can't handle more than 2GB files on 64bit platform.
Also fs.truncate() can't handle more than 4GB files.

Node already has the capability to handle large files.
This patch changes only a parameter check (read/write) and
type conversion from JS's number to C types (truncate).

The testcase only works on 64bit platform, then I put it into test/disabled.

Fix fs can't handle large file on 64bit platform
fs.read() and fs.write() can't handle more than 2GB files on 64bit platform.
Also fs.truncate() can't handle more than 4GB files.

Node already has the capability to handle large files.
This patch changes only a parameter check (read/write) and
type conversion from JS's number to C types (truncate).

The testcase only works on 64bit platform, then I put it into test/disabled.

Your guard should check for _LARGEFILE_SOURCE, __USE_FILE_OFFSET64 is a GNU-ism.

Owner

koichik replied Jun 18, 2011

Thanks!

That check will fail if x is e.g. 42.000001. Could happen if x is the result of arithmetic with rounding errors.

Owner

koichik replied Jun 18, 2011

It's a way same as v8::Value::IsInt32.

Right. Maybe it's better to raise a JS exception if the argument is not a true integer? Both floating-point truncation and defaulting to -1 (i.e. write instead of pwrite) can lead to bugs that are extremely hard to track down. It's probably best to simply inform the user that something is amiss.

@ry what do you think?

Owner

koichik replied Jun 18, 2011

Basically I agree Ben.
Because I want to this pull-request for v0.4 (stable branch) rather than master (v0.5), then I did not want to change behavior except the large file support.

ry replied Jun 20, 2011

+1 to throwing on non-integer values

Owner

koichik replied Jun 20, 2011

May I change behavior on v0.4?

ry replied Jun 20, 2011

yes

Owner

koichik replied Jun 20, 2011

I added the commit.

Like this then? Drop the semi-colon, add braces to avoid operator precedence errors.

#define GET_OFFSET(a)  ((a)->IsNumber() ? (a)->IntegerValue() : -1)
Owner

koichik replied Jun 18, 2011

I wrote it in the same way as an original (32bit) code. See line 670

Sorry, Koichi. That was a 'we can do better' in the general sense, not a critique on your work.

Just a note for a future reader...

If you want to return a large integer value from .cc to JS you can't use the commonly-seen method of returning integer values,

    return scope.Close(Integer::New(offs_result));

This will only handle signed 32 bit integer values. Even the added Integer::NewFromUnsigned() will only make unsigned full 32 bit values.

You must switch to using Number::New() to create integer values more than 32 bits. I don't know how large the values can be before truncation occurs, but this is working for me now up past 43 bits:

    return scope.Close(Number::New(offs_result));

koichik commented Jun 23, 2011

Hi, tshinnic.

Because the Write()/Read()'s length is singed int32, they does not return a number bigger than signed int32.
https://github.com/joyent/node/blob/feb26d6c742980b660efc988133640cf14fc67d3/src/node_file.cc#L698
https://github.com/joyent/node/blob/feb26d6c742980b660efc988133640cf14fc67d3/src/node_file.cc#L762

Also the Buffer size is up to signed int32, may be.
https://github.com/joyent/node/blob/5e409c2f1aa94b76f42e58001f7fcd0959a958aa/src/node_buffer.cc#L178

Then I think using Number::New is not a must in this issue.
I agree that we will be necessary when Node supported large Buffer in the future.

Agreed, not needed here

I thought to add the note here because I was using your code to update another module for position value input arguments:
baudehlo/node-fs-ext@3ac09b0
and because there I had to also output position values. If people want to know how to do both, here are both answers! :-)

japj commented Jul 7, 2011

can this be closed?

koichik added a commit to koichik/node that referenced this pull request Jul 8, 2011

Fix fs can't handle large file on 64bit platform
fs.read() and fs.write() can't handle more than 2GB files on 64bit platform.
Also fs.truncate() can't handle more than 4GB files.

Fixes #1199.

koichik commented Jul 8, 2011

I am going to push this myself :-)
Because v0.5 was released, I merged it into the master (ab68f39).
Please review.

@koichik koichik closed this in a3e3ad4 Jul 13, 2011

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