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

Fix appendFile bug shown to fail in PR-494 #546

Open
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@MuchtarSalimov

MuchtarSalimov commented Oct 9, 2018

As seen in PR #494, the current implementation of appendFile fails when passing a parameter that looks like { flag: 'a' } (which should succeed, since 'a' is a default value). I have included a fix and the accompanying test to verify that it works (that previous failed with the current implementation).

The reason it failed on this type of input is that validate_file_options in implementation.js only checks if the parameter is null, a function, or a string. If it is something else (which in this case in an object), it just returns it as-is. But in the case of having an object but not having an encoding specified (as in { flags: 'a' }), options.encoding is not set to the default value.

Fixes #463

@humphd

This is great that you've also gone ahead and fixed the issue you found with the tests, nice work!

Because you've built on the same set of tests, and included them here too, I'm going to close #494 and use this PR instead. I'll obviously still count that for your 0.1 release, but it will make landing this fix easier.

@@ -1875,7 +1875,7 @@ function appendFile(fs, context, path, data, options, callback) {
if(typeof data === 'number') {
data = '' + data;
}
if(typeof data === 'string' && options.encoding === 'utf8') {
if(typeof data === 'string' && (options.encoding == null || options.encoding === 'utf8')) {

This comment has been minimized.

@humphd

humphd Oct 9, 2018

Contributor

Let's change options.encoding == null to use === so we don't also allow undefined here.

@humphd

This comment has been minimized.

Contributor

humphd commented Oct 9, 2018

@MuchtarSalimov that broke your test: ✖ should append without error when specfifying flag option (default value)

Potential rewrite of same change. seeking review
@humphd the change you asked for said to rule out undefined, but now that I think about it, isn't the fact that it is undefined the problem? The object has no such attribute, and thats why it breaks. I have written a new way of checking this and would like your opinion on it.

I could just undone the == -> === change but I expect that there might be a good reason not to do that.
@humphd

This comment has been minimized.

humphd commented on 7ad1312 Oct 10, 2018

Yeah, this makes sense to me. While you're in there, why don't you deal with all the optional properties and defaults on options:

    encoding <string> | <null> Default: 'utf8'
    mode <integer> Default: 0o666
    flag <string> See support of file system flags. Default: 'a'.

MuchtarSalimov added some commits Oct 16, 2018

@humphd

Looks good, just switch those if checks to not use `undefined.

Show resolved Hide resolved src/filesystem/implementation.js Outdated

humphd and others added some commits Oct 17, 2018

@@ -1870,11 +1870,24 @@ function appendFile(fs, context, path, data, options, callback) {
if(!flags) {
return callback(new Errors.EINVAL('flags is not valid', path));
}
if (typeof options === 'object') {

This comment has been minimized.

@humphd

humphd Oct 17, 2018

Contributor

I'm confused about what you're doing here, because validate_file_options above on line 1865 does the same thing, see

function validate_file_options(options, enc, fileMode){
if(!options) {
options = { encoding: enc, flag: fileMode };
} else if(typeof options === 'function') {
options = { encoding: enc, flag: fileMode };
} else if(typeof options === 'string') {
options = { encoding: options, flag: fileMode };
}
return options;
}

The only thing it's missing is the mode. By the time we get to your code here, options should always already be an Object with encoding and flag, so your code here is essentially not doing anything.

Should we just move your options.mode up to 1866 like so:

options.mode = options.mode || 0o666;

This comment has been minimized.

@MuchtarSalimov

MuchtarSalimov Oct 17, 2018

I think you're right about setting options.flag and options.mode not doing anything, since not only does it come after line 1869, that line also takes care of it being undefined as well.
var flags = validate_flags(options.flag || 'a');
As for mode, I don't see it being called at all.

However,

  1. options needs not be an object by this point. If it is an integer, it never gets replaced with an object, and checking options.encoding then seems wrong to me. That's why I check for the object type.

  2. The options object needs not be fully formed. If the object passed is empty:
    {}
    or missing options.encoding:
    { flag = 'a' }
    it breaks because validate_file_options assumes that it is fine and leaves it alone.

If you would like, I can:

  • remove the extra lines related to options.flag
  • remove the extra lines related to options.mode (or change it to 1866, but I don't see it being used anywhere)

This comment has been minimized.

@humphd

humphd Oct 17, 2018

Contributor

According to https://nodejs.org/api/fs.html#fs_fs_appendfile_path_data_options_callback, options has to be one of a) an Object; b) a String and "If options is a string, then it specifies the encoding"

This is why validate_file_options is checking for the cases:

  1. no options arg given at all, in which case it creates a default Object that's only missing the mode default value
  2. options is a function, which is a special case of 1., where not even args were passed to the appendFile, and we didn't get an optionsObject, only the callback`.
  3. options is a String, which means we've been passed the encoding value, and we package that up as an `Object.

All of that to say, it can't be an Integer at this point, it has to be an Object. If you try and run this in node (i.e., with options as a Number), here is what happens:

> require('fs').appendFile('./myfile', 'data', 1, err => console.log(err));
TypeError: "options" must be a string or an object, got number instead.
    at getOptions (fs.js:75:11)
    at Object.fs.appendFile (fs.js:1323:13)

So maybe we should also add a test to make sure that if you pass a Number, it will fail the way we expect.

However, I agree that it never checks that options as an Object has the correct shape (properties we expect). Out of interest, here is what node does:

https://github.com/nodejs/node/blob/eef072fa083f05f84fa6ca1908472eb228095a38/lib/fs.js#L1227-L1239

Which is calling getOptions:

https://github.com/nodejs/node/blob/eef072fa083f05f84fa6ca1908472eb228095a38/lib/internal/fs/utils.js#L165-L182

I think you should do the following:

  1. Call validate_file_options to try and get options in the right form
  2. Do a check similar to what node does with what you get back (i.e., make sure it's an Object:
    if (typeof options !== 'object') {
        throw new ERR_INVALID_ARG_TYPE('options', ['string', 'Object'], options);
    }
  3. Make sure that options has the correct shape (all properties are there, or set defaults).

This is pretty close to what you have now, with the exception that you're silently allowing non-Object types to get through.

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