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.write does not work on external two-byte strings #18146

Closed
joyeecheung opened this issue Jan 14, 2018 · 3 comments
Closed

fs.write does not work on external two-byte strings #18146

joyeecheung opened this issue Jan 14, 2018 · 3 comments
Labels
fs Issues and PRs related to the fs subsystem / file system.

Comments

@joyeecheung
Copy link
Member

joyeecheung commented Jan 14, 2018

  • Version: master
  • Platform: all
  • Subsystem: fs

Discovered during #18144

// Flags: --expose_externalize_string

'use strict';

const assert = require('assert');
const fs = require('fs');
const file = 'write-external.txt';
const expected = '中文';
externalizeString(expected);

fs.open(file, 'w', 0o644, function(err, fd) {
  fs.write(fd, expected, 0, 'utf8', function(err, written) {
    fs.closeSync(fd);
    const found = fs.readFileSync(file, 'utf8');
    fs.unlinkSync(file);
    assert.strictEqual(expected, found);  // AssertionError [ERR_ASSERTION]: '中文' strictEqual '-N�e'
  });
});
@joyeecheung joyeecheung added the fs Issues and PRs related to the fs subsystem / file system. label Jan 14, 2018
@targos
Copy link
Member

targos commented Jan 15, 2018

/cc @nodejs/v8

@bnoordhuis
Copy link
Member

The surface issue is that fs.write() ignores the encoding argument with externalized strings. The broader issue is that the way node handles external strings is broken and unsafe.

Broken because string_bytes.cc ignores the encoding. Unsafe because the string is collectible by the GC. Storing the pointer like async fs.write() does can result in segfaults and use-after-frees.

I'll see if I can put together a pull request.

@bnoordhuis
Copy link
Member

#18216

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Jan 23, 2018
* Respect `encoding` argument when the string is externalized.

* Copy the string when the write request can outlive the externalized
  string.

This commit removes `StringBytes::GetExternalParts()` because it is
fundamentally broken.

Fixes: nodejs#18146
PR-URL: nodejs#18216
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
* Respect `encoding` argument when the string is externalized.

* Copy the string when the write request can outlive the externalized
  string.

This commit removes `StringBytes::GetExternalParts()` because it is
fundamentally broken.

Fixes: nodejs#18146
PR-URL: nodejs#18216
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax pushed a commit to addaleax/node that referenced this issue Oct 2, 2018
* Respect `encoding` argument when the string is externalized.

* Copy the string when the write request can outlive the externalized
  string.

This commit removes `StringBytes::GetExternalParts()` because it is
fundamentally broken.

Fixes: nodejs#18146
Fixes: nodejs#22728
PR-URL: nodejs#18216
BethGriggs pushed a commit that referenced this issue Oct 16, 2018
* Respect `encoding` argument when the string is externalized.

* Copy the string when the write request can outlive the externalized
  string.

This commit removes `StringBytes::GetExternalParts()` because it is
fundamentally broken.

Fixes: #18146
Fixes: #22728
Backport-PR-URL: #22731
PR-URL: #18216

Reviewed-By: James M Snell <jasnell@gmail.com>
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

No branches or pull requests

3 participants