Skip to content

Commit 81970f8

Browse files
bnoordhuisgibfahn
authored andcommitted
src: fix UB in InternalModuleReadFile()
`&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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent 4ede5ec commit 81970f8

File tree

3 files changed

+21
-6
lines changed

3 files changed

+21
-6
lines changed

src/node_file.cc

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -539,12 +539,17 @@ static void InternalModuleReadFile(const FunctionCallbackInfo<Value>& args) {
539539
start = 3; // Skip UTF-8 BOM.
540540
}
541541

542-
Local<String> chars_string =
543-
String::NewFromUtf8(env->isolate(),
544-
&chars[start],
545-
String::kNormalString,
546-
offset - start);
547-
args.GetReturnValue().Set(chars_string);
542+
const size_t size = offset - start;
543+
if (size == 0) {
544+
args.GetReturnValue().SetEmptyString();
545+
} else {
546+
Local<String> chars_string =
547+
String::NewFromUtf8(env->isolate(),
548+
&chars[start],
549+
String::kNormalString,
550+
size);
551+
args.GetReturnValue().Set(chars_string);
552+
}
548553
}
549554

550555
// Used to speed up module loading. Returns 0 if the path refers to

test/fixtures/empty-with-bom.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+


test/parallel/test-module-binding.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
'use strict';
2+
require('../common');
3+
const fixtures = require('../common/fixtures');
4+
const { internalModuleReadFile } = process.binding('fs');
5+
const { strictEqual } = require('assert');
6+
7+
strictEqual(internalModuleReadFile('nosuchfile'), undefined);
8+
strictEqual(internalModuleReadFile(fixtures.path('empty.txt')), '');
9+
strictEqual(internalModuleReadFile(fixtures.path('empty-with-bom.txt')), '');

0 commit comments

Comments
 (0)