Skip to content

Commit

Permalink
src: fix UB in InternalModuleReadFile()
Browse files Browse the repository at this point in the history
`&vec[0]` is undefined behavior when `vec.size() == 0`.

It is mostly academic because package.json files are not usually empty
and because with most STL implementations it decays to something that
is legal C++ as long as the result is not dereferenced, but better safe
than sorry.

Note that the tests don't actually fail because of that, I added them
as sanity checks.

PR-URL: #16871
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
bnoordhuis authored and evanlucas committed Nov 13, 2017
1 parent 28b7bf0 commit 43c5726
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 6 deletions.
17 changes: 11 additions & 6 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -515,12 +515,17 @@ static void InternalModuleReadFile(const FunctionCallbackInfo<Value>& args) {
start = 3; // Skip UTF-8 BOM.
}

Local<String> chars_string =
String::NewFromUtf8(env->isolate(),
&chars[start],
String::kNormalString,
offset - start);
args.GetReturnValue().Set(chars_string);
const size_t size = offset - start;
if (size == 0) {
args.GetReturnValue().SetEmptyString();
} else {
Local<String> chars_string =
String::NewFromUtf8(env->isolate(),
&chars[start],
String::kNormalString,
size);
args.GetReturnValue().Set(chars_string);
}
}

// Used to speed up module loading. Returns 0 if the path refers to
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/empty-with-bom.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@

9 changes: 9 additions & 0 deletions test/parallel/test-module-binding.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
'use strict';
require('../common');
const fixtures = require('../common/fixtures');
const { internalModuleReadFile } = process.binding('fs');
const { strictEqual } = require('assert');

strictEqual(internalModuleReadFile('nosuchfile'), undefined);
strictEqual(internalModuleReadFile(fixtures.path('empty.txt')), '');
strictEqual(internalModuleReadFile(fixtures.path('empty-with-bom.txt')), '');

0 comments on commit 43c5726

Please sign in to comment.