fs: fix partial write corruption in writeFile and writeFileSync - #1063
olov wants to merge 1 commit into
Conversation
Sorry, something went wrong.
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.
There was a problem hiding this comment.
Why the null check here?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Ah, got it, makes sense.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
I don't see how position can become undefined or false in any of the two code paths.
Sorry, something went wrong.
There was a problem hiding this comment.
To elaborate, position is internal to writeFileSync and writeFile. In the latter case via an internal writeAll function that's only called from writeFile.
Sorry, something went wrong.
|
Is it possible to add a test for the bug(s) this PR addresses too? |
Sorry, something went wrong.
|
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"}); |
Sorry, something went wrong.
|
Anything else needed here? |
Sorry, something went wrong.
|
Yeah, I don't know how to test this either. |
Sorry, something went wrong.
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>
Sorry, something went wrong.
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.
drift in iteration three and onwards.
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