From 81970f87edca4f0659a28c2ff74c03d1eaa6ab77 Mon Sep 17 00:00:00 2001
From: Ben Noordhuis <info@bnoordhuis.nl>
Date: Tue, 7 Nov 2017 21:28:13 +0100
Subject: [PATCH] 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: https://github.com/nodejs/node/pull/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>
---
 src/node_file.cc                     | 17 +++++++++++------
 test/fixtures/empty-with-bom.txt     |  1 +
 test/parallel/test-module-binding.js |  9 +++++++++
 3 files changed, 21 insertions(+), 6 deletions(-)
 create mode 100644 test/fixtures/empty-with-bom.txt
 create mode 100644 test/parallel/test-module-binding.js

diff --git a/src/node_file.cc b/src/node_file.cc
index 5039f3ff1e71ff..2b67978556316c 100644
--- a/src/node_file.cc
+++ b/src/node_file.cc
@@ -539,12 +539,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
diff --git a/test/fixtures/empty-with-bom.txt b/test/fixtures/empty-with-bom.txt
new file mode 100644
index 00000000000000..5f282702bb03ef
--- /dev/null
+++ b/test/fixtures/empty-with-bom.txt
@@ -0,0 +1 @@
+
\ No newline at end of file
diff --git a/test/parallel/test-module-binding.js b/test/parallel/test-module-binding.js
new file mode 100644
index 00000000000000..ba11c3e47ec646
--- /dev/null
+++ b/test/parallel/test-module-binding.js
@@ -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')), '');