tschaub / mock-fs

Configurable mock for the fs module
https://npmjs.org/package/mock-fs
Other
906 stars 85 forks source link

EBADF from fs.promises.readFile() and O_CREAT #288

Closed CynicalBusiness closed 4 years ago

CynicalBusiness commented 4 years ago

Had a bunch of tests failing with EBADF even after updating to ^4.10 (as mentioned in #245), finally managed to track it down.

When trying to mock fs.promises.readFile() with the fs.constants.O_CREAT flag, an EBADF is raised if the file does not previously exist. If the file is created manually or the flag is not used, it reads normally or raises ENOENT as it should.

Reproducable:

import mock from "mock-fs";
import { promises, constants } from "fs";

mock({ "foo.txt": "bar" });
const str1 = await promises.readFile("foo.txt"); // works fine
const str2 = await promises.readFile("bar.txt"); // ENOENT, as expected
const str3 = await promises.readFile("foo.txt", constants.O_CREAT | cosntants.O_RDONLY); // works
const str4 = await promises.readFile("bar.txt", constants.O_CREAT | constants.O_RDONLY); // EBADF, unexpected behavior.

node: 12.6.0 mock-fs: 4.10.4

3cp commented 4 years ago

O_RDONLY is 0, so constants.O_CREAT | constants.O_RDONLY is literally just constants.O_CREAT.

mockfs got confused about the read mode.

https://github.com/tschaub/mock-fs/blob/ad93ac551d54a9a346a7f4cccfcb89ca67fe9463/lib/descriptor.js#L82-L89

The implementation should be fixed and simplified as

FileDescriptor.prototype.isRead = function() {
  return (this._flags & constants.O_WRONLY) !== constants.O_WRONLY;
};

If I understand open(2) man page correctly. http://man7.org/linux/man-pages/man2/open.2.html

The argument flags must include one of the following access modes:
O_RDONLY, O_WRONLY, or O_RDWR.  These request opening the file read-
only, write-only, or read/write, respectively.

O_RDONLY is 0, O_WRONLY is 1, O_RDWR is 2, the flag uses the 2 bits for 3 states: 0 (0b00) / 1 (0b01) / 2 (0b10), the 2 bits will never be 3 (0b11).

@tschaub does this fix make sense?

3cp commented 4 years ago

Note this is not a new bug on promises api, it happens on fs.readFile and fs.readFileSync too.

tschaub commented 4 years ago

Thanks for the report @CynicalBusiness and for the proposed fix @3cp.

Let's come up with a valid test case first. The examples given in the description above (promises.readFile("foo.txt", constants.O_CREAT | cosntants.O_RDONLY)) aren't valid because they provide a number as the second argument to readFile instead of an object or a string. This would throw TypeError [ERR_INVALID_ARG_TYPE]: The "options" argument must be one of type string or Object. Received type number.

3cp commented 4 years ago

Based on what I tested, although nodejs doc did not say, the readFile/sync/promise APIs support numeric option as if it's {flag: number}.

tschaub commented 4 years ago

Fix published in mock-fs@4.12.0. Thanks, @3cp.