src: use custom fprintf alike to write errors to stderr

This allows printing errors that contain nul characters, for example. Fixes: #28761 Fixes: #31218
nodejs · addaleax · Jan 21, 2020 · Jan 21, 2020 · Jan 21, 2020 · Jan 21, 2020
commit ec00fdabfb0006baf87da39c38c817620dafbed1
diff --git a/src/debug_utils-inl.h b/src/debug_utils-inl.h
@@ -79,6 +79,11 @@ std::string COLD_NOINLINE SPrintF(  // NOLINT(runtime/string)
   return SPrintFImpl(format, std::forward<Args>(args)...);
 }
 
+template <typename... Args>
+void COLD_NOINLINE FPrintF(FILE* file, const char* format, Args&&... args) {
+  FWrite(file, SPrintF(format, std::forward<Args>(args)...));
+}
+
 }  // namespace node
 
 #endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

diff --git a/src/debug_utils.cc b/src/debug_utils.cc
@@ -6,6 +6,10 @@
 #include <features.h>
 #endif
 
+#ifdef __ANDROID__
+#include <android/log.h>
+#endif
+
 #if defined(__linux__) && !defined(__GLIBC__) || \
     defined(__UCLIBC__) || \
     defined(_AIX)
@@ -437,6 +441,37 @@ std::vector<std::string> NativeSymbolDebuggingContext::GetLoadedLibraries() {
   return list;
 }
 
+void FWrite(FILE* file, const std::string& str) {
+  if (file != stderr && file != stdout) goto simple_fwrite;
+#ifdef _WIN32
+  HANDLE handle =
+      GetStdHandle(file == stdout ? STD_OUTPUT_HANDLE : STD_ERROR_HANDLE);
+
+  // Check if stderr is something other than a tty/console
+  if (handle == INVALID_HANDLE_VALUE || handle == nullptr ||
+      uv_guess_handle(_fileno(file)) != UV_TTY) {
+    goto simple_fwrite;
+  }
+
+  // Get required wide buffer size
+  int n = MultiByteToWideChar(CP_UTF8, 0, str.data(), str.size(), nullptr, 0);
+
+  std::vector<wchar_t> wbuf(n);
+  MultiByteToWideChar(CP_UTF8, 0, str.data(), str.size(), wbuf.data(), n);
+
+  // Don't include the final null character in the output
+  CHECK_GT(n, 0);
+  WriteConsoleW(handle, wbuf.data(), n - 1, nullptr, nullptr);
+  return;
+#elif defined(__ANDROID__)
+  if (file == stderr) {
+    __android_log_print(ANDROID_LOG_ERROR, "nodejs", "%s", str.data());
+    return;
+  }
+#endif
+simple_fwrite:
+  fwrite(str.data(), str.size(), 1, file);
+}
 
 }  // namespace node
 

diff --git a/src/debug_utils.h b/src/debug_utils.h
@@ -25,13 +25,16 @@ namespace node {
 template <typename T>
 inline std::string ToString(const T& value);
 
-// C++-style variant of sprintf() that:
+// C++-style variant of sprintf()/fprintf() that:
 // - Returns an std::string
 // - Handles \0 bytes correctly
 // - Supports %p and %s. %d, %i and %u are aliases for %s.
 // - Accepts any class that has a ToString() method for stringification.
 template <typename... Args>
 inline std::string SPrintF(const char* format, Args&&... args);
+template <typename... Args>
+inline void FPrintF(FILE* file, const char* format, Args&&... args);
+void FWrite(FILE* file, const std::string& str);
 
 template <typename... Args>
 inline void FORCE_INLINE Debug(Environment* env,
@@ -40,16 +43,15 @@ inline void FORCE_INLINE Debug(Environment* env,
                                Args&&... args) {
   if (!UNLIKELY(env->debug_enabled(cat)))
     return;
-  std::string out = SPrintF(format, std::forward<Args>(args)...);
-  fwrite(out.data(), out.size(), 1, stderr);
+  FPrintF(stderr, format, std::forward<Args>(args)...);
 }
 
 inline void FORCE_INLINE Debug(Environment* env,
                                DebugCategory cat,
                                const char* message) {
   if (!UNLIKELY(env->debug_enabled(cat)))
     return;
-  fprintf(stderr, "%s", message);
+  FPrintF(stderr, "%s", message);
 }
 
 template <typename... Args>

diff --git a/src/node_errors.cc b/src/node_errors.cc
@@ -1,6 +1,7 @@
 #include <cerrno>
 #include <cstdarg>
 
+#include "debug_utils-inl.h"
 #include "node_errors.h"
 #include "node_internals.h"
 #ifdef NODE_REPORT
@@ -10,10 +11,6 @@
 #include "node_v8_platform-inl.h"
 #include "util-inl.h"
 
-#ifdef __ANDROID__
-#include <android/log.h>
-#endif
-
 namespace node {
 
 using errors::TryCatchScope;
@@ -54,8 +51,6 @@ namespace per_process {
 static Mutex tty_mutex;
 }  // namespace per_process
 
-static const int kMaxErrorSourceLength = 1024;
-
 static std::string GetErrorSource(Isolate* isolate,
                                   Local<Context> context,
                                   Local<Message> message,
@@ -107,41 +102,35 @@ static std::string GetErrorSource(Isolate* isolate,
     end -= script_start;
   }
 
-  int max_off = kMaxErrorSourceLength - 2;
-
-  char buf[kMaxErrorSourceLength];
-  int off = snprintf(buf,
-                     kMaxErrorSourceLength,
-                     "%s:%i\n%s\n",
-                     filename_string,
-                     linenum,
-                     sourceline.c_str());
-  CHECK_GE(off, 0);
-  if (off > max_off) {
-    off = max_off;
-  }
+  std::string buf = SPrintF("%s:%i\n%s\n",
+                            filename_string,
+                            linenum,
+                            sourceline.c_str());
+  CHECK_GE(buf.size(), 0);
 
+  constexpr int kUnderlineBufsize = 1020;
+  char underline_buf[kUnderlineBufsize + 4];
+  int off = 0;
   // Print wavy underline (GetUnderline is deprecated).
   for (int i = 0; i < start; i++) {
-    if (sourceline[i] == '\0' || off >= max_off) {
+    if (sourceline[i] == '\0' || off >= kUnderlineBufsize) {
       break;
     }
-    CHECK_LT(off, max_off);
-    buf[off++] = (sourceline[i] == '\t') ? '\t' : ' ';
+    CHECK_LT(off, kUnderlineBufsize);
+    underline_buf[off++] = (sourceline[i] == '\t') ? '\t' : ' ';
   }
   for (int i = start; i < end; i++) {
-    if (sourceline[i] == '\0' || off >= max_off) {
+    if (sourceline[i] == '\0' || off >= kUnderlineBufsize) {
       break;
     }
-    CHECK_LT(off, max_off);
-    buf[off++] = '^';
+    CHECK_LT(off, kUnderlineBufsize);
+    underline_buf[off++] = '^';
   }
-  CHECK_LE(off, max_off);
-  buf[off] = '\n';
-  buf[off + 1] = '\0';
+  CHECK_LE(off, kUnderlineBufsize);
+  underline_buf[off++] = '\n';
 
   *added_exception_line = true;
-  return std::string(buf);
+  return buf + std::string(underline_buf, off);
 }
 
 void PrintStackTrace(Isolate* isolate, Local<StackTrace> stack) {
@@ -154,9 +143,9 @@ void PrintStackTrace(Isolate* isolate, Local<StackTrace> stack) {
 
     if (stack_frame->IsEval()) {
       if (stack_frame->GetScriptId() == Message::kNoScriptIdInfo) {
-        fprintf(stderr, "    at [eval]:%i:%i\n", line_number, column);
+        FPrintF(stderr, "    at [eval]:%i:%i\n", line_number, column);
       } else {
-        fprintf(stderr,
+        FPrintF(stderr,
                 "    at [eval] (%s:%i:%i)\n",
                 *script_name,
                 line_number,
@@ -166,12 +155,12 @@ void PrintStackTrace(Isolate* isolate, Local<StackTrace> stack) {
     }
 
     if (fn_name_s.length() == 0) {
-      fprintf(stderr, "    at %s:%i:%i\n", *script_name, line_number, column);
+      FPrintF(stderr, "    at %s:%i:%i\n", script_name, line_number, column);
     } else {
-      fprintf(stderr,
+      FPrintF(stderr,
               "    at %s (%s:%i:%i)\n",
-              *fn_name_s,
-              *script_name,
+              fn_name_s,
+              script_name,
               line_number,
               column);
     }
@@ -189,8 +178,8 @@ void PrintException(Isolate* isolate,
   bool added_exception_line = false;
   std::string source =
       GetErrorSource(isolate, context, message, &added_exception_line);
-  fprintf(stderr, "%s\n", source.c_str());
-  fprintf(stderr, "%s\n", *reason);
+  FPrintF(stderr, "%s\n", source);
+  FPrintF(stderr, "%s\n", reason);
 
   Local<v8::StackTrace> stack = message->GetStackTrace();
   if (!stack.IsEmpty()) PrintStackTrace(isolate, stack);
@@ -235,7 +224,7 @@ void AppendExceptionLine(Environment* env,
     env->set_printed_error(true);
 
     ResetStdio();
-    PrintErrorString("\n%s", source.c_str());
+    FPrintF(stderr, "\n%s", source);
     return;
   }
 
@@ -350,10 +339,10 @@ static void ReportFatalException(Environment* env,
   // range errors have a trace member set to undefined
   if (trace.length() > 0 && !stack_trace->IsUndefined()) {
     if (arrow.IsEmpty() || !arrow->IsString() || decorated) {
-      PrintErrorString("%s\n", *trace);
+      FPrintF(stderr, "%s\n", trace);
     } else {
       node::Utf8Value arrow_string(env->isolate(), arrow);
-      PrintErrorString("%s\n%s\n", *arrow_string, *trace);
+      FPrintF(stderr, "%s\n%s\n", arrow_string, trace);
     }
   } else {
     // this really only happens for RangeErrors, since they're the only
@@ -371,76 +360,40 @@ static void ReportFatalException(Environment* env,
     if (message.IsEmpty() || message.ToLocalChecked()->IsUndefined() ||
         name.IsEmpty() || name.ToLocalChecked()->IsUndefined()) {
       // Not an error object. Just print as-is.
-      String::Utf8Value message(env->isolate(), error);
+      node::Utf8Value message(env->isolate(), error);
 
-      PrintErrorString("%s\n",
-                       *message ? *message : "<toString() threw exception>");
+      FPrintF(stderr, "%s\n",
+              *message ? message.ToString() : "<toString() threw exception>");
     } else {
       node::Utf8Value name_string(env->isolate(), name.ToLocalChecked());
       node::Utf8Value message_string(env->isolate(), message.ToLocalChecked());
 
       if (arrow.IsEmpty() || !arrow->IsString() || decorated) {
-        PrintErrorString("%s: %s\n", *name_string, *message_string);
+        FPrintF(stderr, "%s: %s\n", name_string, message_string);
       } else {
         node::Utf8Value arrow_string(env->isolate(), arrow);
-        PrintErrorString(
-            "%s\n%s: %s\n", *arrow_string, *name_string, *message_string);
+        FPrintF(stderr,
+            "%s\n%s: %s\n", arrow_string, name_string, message_string);
       }
     }
 
     if (!env->options()->trace_uncaught) {
-      PrintErrorString("(Use `node --trace-uncaught ...` to show "
-                       "where the exception was thrown)\n");
+      FPrintF(stderr, "(Use `node --trace-uncaught ...` to show "
+                      "where the exception was thrown)\n");
     }
   }
 
   if (env->options()->trace_uncaught) {
     Local<StackTrace> trace = message->GetStackTrace();
     if (!trace.IsEmpty()) {
-      PrintErrorString("Thrown at:\n");
+      FPrintF(stderr, "Thrown at:\n");
       PrintStackTrace(env->isolate(), trace);
     }
   }
 
   fflush(stderr);
 }
 
-void PrintErrorString(const char* format, ...) {
-  va_list ap;
-  va_start(ap, format);
-#ifdef _WIN32
-  HANDLE stderr_handle = GetStdHandle(STD_ERROR_HANDLE);
-
-  // Check if stderr is something other than a tty/console
-  if (stderr_handle == INVALID_HANDLE_VALUE || stderr_handle == nullptr ||
-      uv_guess_handle(_fileno(stderr)) != UV_TTY) {
-    vfprintf(stderr, format, ap);
-    va_end(ap);
-    return;
-  }
-
-  // Fill in any placeholders
-  int n = _vscprintf(format, ap);
-  std::vector<char> out(n + 1);
-  vsprintf(out.data(), format, ap);
-
-  // Get required wide buffer size
-  n = MultiByteToWideChar(CP_UTF8, 0, out.data(), -1, nullptr, 0);
-
-  std::vector<wchar_t> wbuf(n);
-  MultiByteToWideChar(CP_UTF8, 0, out.data(), -1, wbuf.data(), n);
-
-  // Don't include the null character in the output
-  CHECK_GT(n, 0);
-  WriteConsoleW(stderr_handle, wbuf.data(), n - 1, nullptr, nullptr);
-#elif defined(__ANDROID__)
-  __android_log_vprint(ANDROID_LOG_ERROR, "nodejs", format, ap);
-#else
-  vfprintf(stderr, format, ap);
-#endif
-  va_end(ap);
-}
-
 [[noreturn]] void FatalError(const char* location, const char* message) {
   OnFatalError(location, message);
   // to suppress compiler warning
@@ -449,9 +402,9 @@ void PrintErrorString(const char* format, ...) {
 
 void OnFatalError(const char* location, const char* message) {
   if (location) {
-    PrintErrorString("FATAL ERROR: %s %s\n", location, message);
+    FPrintF(stderr, "FATAL ERROR: %s %s\n", location, message);
   } else {
-    PrintErrorString("FATAL ERROR: %s\n", message);
+    FPrintF(stderr, "FATAL ERROR: %s\n", message);
   }
 #ifdef NODE_REPORT
   Isolate* isolate = Isolate::GetCurrent();

diff --git a/src/node_errors.h b/src/node_errors.h
@@ -25,8 +25,6 @@ void AppendExceptionLine(Environment* env,
 [[noreturn]] void FatalError(const char* location, const char* message);
 void OnFatalError(const char* location, const char* message);
 
-void PrintErrorString(const char* format, ...);
-
 // Helpers to construct errors similar to the ones provided by
 // lib/internal/errors.js.
 // Example: with `V(ERR_INVALID_ARG_TYPE, TypeError)`, there will be

diff --git a/src/node_process_methods.cc b/src/node_process_methods.cc
@@ -1,4 +1,5 @@
 #include "base_object-inl.h"
+#include "debug_utils-inl.h"
 #include "env-inl.h"
 #include "node.h"
 #include "node_errors.h"
@@ -216,7 +217,7 @@ void RawDebug(const FunctionCallbackInfo<Value>& args) {
   CHECK(args.Length() == 1 && args[0]->IsString() &&
         "must be called with a single string");
   Utf8Value message(args.GetIsolate(), args[0]);
-  PrintErrorString("%s\n", *message);
+  FPrintF(stderr, "%s\n", message);
   fflush(stderr);
 }
 

diff --git a/src/util.h b/src/util.h
@@ -473,6 +473,8 @@ class ArrayBufferViewContents {
 class Utf8Value : public MaybeStackBuffer<char> {
  public:
   explicit Utf8Value(v8::Isolate* isolate, v8::Local<v8::Value> value);
+
+  inline std::string ToString() const { return std::string(out(), length()); }
 };
 
 class TwoByteValue : public MaybeStackBuffer<uint16_t> {
@@ -483,6 +485,8 @@ class TwoByteValue : public MaybeStackBuffer<uint16_t> {
 class BufferValue : public MaybeStackBuffer<char> {
  public:
   explicit BufferValue(v8::Isolate* isolate, v8::Local<v8::Value> value);
+
+  inline std::string ToString() const { return std::string(out(), length()); }
 };
 
 #define SPREAD_BUFFER_ARG(val, name)                                          \

diff --git a/test/message/error_with_nul.js b/test/message/error_with_nul.js
@@ -0,0 +1,11 @@
+'use strict';
+require('../common');
+
+function test() {
+  const a = 'abc\0def';
+  console.error(a);
+  throw new Error(a);
+}
+Object.defineProperty(test, 'name', { value: 'fun\0name' });
+
+test();
diff --git a/test/message/error_with_nul.out b/test/message/error_with_nul.out