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

fs: fix partial write corruption in writeFile and writeFileSync #1063

Closed
wants to merge 1 commit into from

Conversation

olov
Copy link
Contributor

@olov olov commented Mar 5, 2015

  1. writeFileSync bumps position incorrectly, causing it to
    drift in iteration three and onwards.
  2. Append mode files will get corrupted in the middle if
    writeFile or writeFileSync iterates multiple times, unless
    running on Linux. position starts out as null so first write is
    OK, but then position will refer to a location inside an
    existing file, corrupting that data. Linux ignores position for
    append mode files so it doesn't happen there.

This commit fixes these two related issues by bumping position
correctly and by always using null as the position argument
to write/writeSync for append mode files.

fixes #1058
cc @piscisaureus

1. writeFileSync bumps position incorrectly, causing it to
drift in iteration three and onwards.

2. Append mode files will get corrupted in the middle if
writeFile or writeFileSync iterates multiple times, unless
running on Linux. position starts out as null so first write is
OK, but then position will refer to a location inside an
existing file, corrupting that data. Linux ignores position for
append mode files so it doesn't happen there.

This commit fixes these two related issues by bumping position
correctly and by always using null as the position argument
to write/writeSync for append mode files.
@@ -1097,7 +1097,9 @@ function writeAll(fd, buffer, offset, length, position, callback) {
} else {
offset += written;
length -= written;
position += written;
if (position !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the null check here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to fix 2. position may be null (see fs.writeFile caller) and without the check position will become a number instead, pointing to the middle of an existing file => corrupting data.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, got it, makes sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the check needs to be if (typeof position === 'number') {, else undefined or false will still trigger the bad behavior. It's also the condition that the documentation for fs.write() mentions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how position can become undefined or false in any of the two code paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To elaborate, position is internal to writeFileSync and writeFile. In the latter case via an internal writeAll function that's only called from writeFile.

@piscisaureus
Copy link
Contributor

Is it possible to add a test for the bug(s) this PR addresses too?

@olov
Copy link
Contributor Author

olov commented Mar 5, 2015

I found it tricky and wasn't able to create anything suitable for inclusion in the PR. Perhaps others know more about how to deterministically test that "write-all" functions really work in the case of partial writes. Do you have any such tests already?

Anyways, this is a variant of the test I used to show issue 2. Do npm install posix; cp /usr/share/dict/words OUT and then sudo-run the script a number of times (between 1 and 10 usually) and you should see corrupted XXXXX showing up 40 bytes into OUT. Remember that issue 2 doesn't happen on Linux-systems (I tested on a Mac).

"use strict";

const fs = require("fs");
const posix = require("posix");
let limit = 40;

posix.setrlimit("fsize", {soft: limit, hard: limit});

const data = new Buffer(new Array(100).join("X"))

setTimeout(function() {
    console.log("upping limit");
    limit += 40;
    posix.setrlimit("fsize", {soft: limit, hard: limit});
}, 2);

fs.writeFile("OUT", data, {flag: "a"});

@olov
Copy link
Contributor Author

olov commented Mar 19, 2015

Anything else needed here?

@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Mar 22, 2015
@piscisaureus
Copy link
Contributor

Yeah, I don't know how to test this either.
Let's at least land this since it addresses a real bug.
LGTM

piscisaureus pushed a commit that referenced this pull request Mar 25, 2015
1. writeFileSync bumps position incorrectly, causing it to drift in
iteration three and onwards.

2. Append mode files will get corrupted in the middle if writeFile or
writeFileSync iterates multiple times, unless running on Linux. position
starts out as null so first write is OK, but then position will refer to
a location inside an existing file, corrupting that data. Linux ignores
position for append mode files so it doesn't happen there.

This commit fixes these two related issues by bumping position correctly
and by always using null as the position argument to write/writeSync for
append mode files.

PR-URL: #1063
Reviewed-By: Bert Belder <bertbelder@gmail.com>
@piscisaureus
Copy link
Contributor

Thanks @olov, landed in c9207f7.
Sorry it took a while to get back to you.

@olov
Copy link
Contributor Author

olov commented Mar 25, 2015

Thanks @piscisaureus!

@rvagg rvagg mentioned this pull request Mar 28, 2015
rvagg added a commit that referenced this pull request Mar 31, 2015
Notable changes:

 * fs: corruption can be caused by fs.writeFileSync() and append-mode
   fs.writeFile() and fs.writeFileSync() under certain circumstances,
   reported in #1058, fixed in #1063 (Olov Lassus).
 * iojs: an "internal modules" API has been introduced to allow core
   code to share JavaScript modules internally only without having to
   expose them as a public API, this feature is for core-only #848
   (Vladimir Kurchatkin).
 * timers: two minor problems with timers have been fixed:
   - Timer#close() is now properly idempotent #1288 (Petka Antonov).
   - setTimeout() will only run the callback once now after an
     unref() during the callback #1231 (Roman Reiss).
 * Windows: a "delay-load hook" has been added for compiled add-ons
   on Windows that should alleviate some of the problems that Windows
   users may be experiencing with add-ons in io.js #1251
   (Bert Belder).
 * V8: minor bug-fix upgrade for V8 to 4.1.0.27.
 * npm: upgrade npm to 2.7.4. See npm CHANGELOG.md for details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants