From 76aad0e5e1ada72ca39e1f73c31d7e81bc6e2dc0 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 21 Jan 2020 20:30:35 +0100 Subject: [PATCH] src: use custom fprintf alike to write errors to stderr This allows printing errors that contain nul characters, for example. Fixes: https://github.com/nodejs/node/issues/28761 Fixes: https://github.com/nodejs/node/issues/31218 PR-URL: https://github.com/nodejs/node/pull/31446 Reviewed-By: James M Snell Reviewed-By: Gus Caplan Reviewed-By: Ben Noordhuis Reviewed-By: Colin Ihrig Reviewed-By: Rich Trott --- src/debug_utils-inl.h | 5 ++ src/debug_utils.cc | 43 +++++++++++ src/debug_utils.h | 10 ++- src/node_errors.cc | 127 ++++++++++---------------------- src/node_errors.h | 2 - src/node_process_methods.cc | 3 +- src/util.h | 4 + test/message/error_with_nul.js | 11 +++ test/message/error_with_nul.out | Bin 0 -> 549 bytes 9 files changed, 111 insertions(+), 94 deletions(-) create mode 100644 test/message/error_with_nul.js create mode 100644 test/message/error_with_nul.out diff --git a/src/debug_utils-inl.h b/src/debug_utils-inl.h index 69b49d9c50d2e3..f24643fbae3ad8 100644 --- a/src/debug_utils-inl.h +++ b/src/debug_utils-inl.h @@ -83,6 +83,11 @@ std::string COLD_NOINLINE SPrintF( // NOLINT(runtime/string) return SPrintFImpl(format, std::forward(args)...); } +template +void COLD_NOINLINE FPrintF(FILE* file, const char* format, Args&&... args) { + FWrite(file, SPrintF(format, std::forward(args)...)); +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/debug_utils.cc b/src/debug_utils.cc index b41cdd7a3f3f2f..8f1e6aa3e6f77c 100644 --- a/src/debug_utils.cc +++ b/src/debug_utils.cc @@ -6,6 +6,10 @@ #include #endif +#ifdef __ANDROID__ +#include +#endif + #if defined(__linux__) && !defined(__GLIBC__) || \ defined(__UCLIBC__) || \ defined(_AIX) @@ -437,6 +441,45 @@ std::vector NativeSymbolDebuggingContext::GetLoadedLibraries() { return list; } +void FWrite(FILE* file, const std::string& str) { + auto simple_fwrite = [&]() { + // The return value is ignored because there's no good way to handle it. + fwrite(str.data(), str.size(), 1, file); + }; + + if (file != stderr && file != stdout) { + simple_fwrite(); + return; + } +#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) { + simple_fwrite(); + return; + } + + // Get required wide buffer size + int n = MultiByteToWideChar(CP_UTF8, 0, str.data(), str.size(), nullptr, 0); + + std::vector 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(); +} } // namespace node diff --git a/src/debug_utils.h b/src/debug_utils.h index 08d23bb7703fe6..c745cbe0a1a74b 100644 --- a/src/debug_utils.h +++ b/src/debug_utils.h @@ -25,13 +25,16 @@ namespace node { template 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 inline std::string SPrintF(const char* format, Args&&... args); +template +inline void FPrintF(FILE* file, const char* format, Args&&... args); +void FWrite(FILE* file, const std::string& str); template inline void FORCE_INLINE Debug(Environment* env, @@ -40,8 +43,7 @@ inline void FORCE_INLINE Debug(Environment* env, Args&&... args) { if (!UNLIKELY(env->debug_enabled(cat))) return; - std::string out = SPrintF(format, std::forward(args)...); - fwrite(out.data(), out.size(), 1, stderr); + FPrintF(stderr, format, std::forward(args)...); } inline void FORCE_INLINE Debug(Environment* env, @@ -49,7 +51,7 @@ inline void FORCE_INLINE Debug(Environment* env, const char* message) { if (!UNLIKELY(env->debug_enabled(cat))) return; - fprintf(stderr, "%s", message); + FPrintF(stderr, "%s", message); } template diff --git a/src/node_errors.cc b/src/node_errors.cc index 7e23fca28dfc0e..f82054c339b493 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -1,6 +1,7 @@ #include #include +#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 -#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, Local 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_GT(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 stack) { @@ -154,9 +143,9 @@ void PrintStackTrace(Isolate* isolate, Local 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 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 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,33 +360,33 @@ 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 : ""); + FPrintF(stderr, "%s\n", + *message ? message.ToString() : ""); } 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 trace = message->GetStackTrace(); if (!trace.IsEmpty()) { - PrintErrorString("Thrown at:\n"); + FPrintF(stderr, "Thrown at:\n"); PrintStackTrace(env->isolate(), trace); } } @@ -405,42 +394,6 @@ static void ReportFatalException(Environment* env, 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 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 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 index bc180b2a68efd7..d56bf7ef5a5a53 100644 --- 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 index 7efe8efb9b9e6d..7b91f89e79159a 100644 --- 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& 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 index 8e90a5ee74ab2f..5eaa20b760168b 100644 --- a/src/util.h +++ b/src/util.h @@ -480,6 +480,8 @@ class ArrayBufferViewContents { class Utf8Value : public MaybeStackBuffer { public: explicit Utf8Value(v8::Isolate* isolate, v8::Local value); + + inline std::string ToString() const { return std::string(out(), length()); } }; class TwoByteValue : public MaybeStackBuffer { @@ -490,6 +492,8 @@ class TwoByteValue : public MaybeStackBuffer { class BufferValue : public MaybeStackBuffer { public: explicit BufferValue(v8::Isolate* isolate, v8::Local 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 new file mode 100644 index 00000000000000..2849e9d21c878c --- /dev/null +++ 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 new file mode 100644 index 0000000000000000000000000000000000000000..396d94debf0058b49d9be19dcc6519486b9f8a6d GIT binary patch literal 549 zcmbVIJ5R$f5bo?>abugo*#lxLTLlXf0_*BrRY&$2`JuG`o@+>i1eH+ncK)9FaBo*V z9aKXp#bicTj)tq(L+%;{P>v~%z%;`4g0FFNC%^AXO=kx<%RwF%I8i9zpmMc zcp^US2eL)qBS$`mSo6c5l3nbpCv9vDAI?jJ<3fqiw_!qZYlqzWh&`pL{_nWOf=-1v zyU1A!^CqX+;u3R{?y<5hlBkys|97Ah*;?o&Q`&M#=jSL(z$<1*m3Qd)?MgF&Oc3nn QGT$Q#*e)#dHR()NpH-Z=5C8xG literal 0 HcmV?d00001