From bc626c237be17352a12ca2ca0ad194c70fe613a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Rom=C3=A1n?= Date: Wed, 12 Jan 2022 09:39:49 +0100 Subject: [PATCH] v8: make writeHeapSnapshot throw if fopen fails If the file fails to be written (e.g. missing permissions, no space left on device, etc), `writeHeapSnapshot` will now throw an exception. This commit also adds error handling for the `fclose` call, returning false if a non-zero value was returned. Fixes: https://github.com/nodejs/node/issues/41346 PR-URL: https://github.com/nodejs/node/pull/41373 Reviewed-By: Anna Henningsen Reviewed-By: Darshan Sen Reviewed-By: Mohammed Keyvanzadeh Reviewed-By: Colin Ihrig --- doc/api/v8.md | 4 ++++ src/env.cc | 2 +- src/heap_utils.cc | 21 +++++++++++++-------- src/node_internals.h | 2 +- test/sequential/test-heapdump.js | 13 +++++++++++++ 5 files changed, 32 insertions(+), 10 deletions(-) diff --git a/doc/api/v8.md b/doc/api/v8.md index 6c14615e08ee41..0359b20396f053 100644 --- a/doc/api/v8.md +++ b/doc/api/v8.md @@ -267,6 +267,10 @@ disk unless [`v8.stopCoverage()`][] is invoked before the process exits. * `filename` {string} The file path where the V8 heap snapshot is to be diff --git a/src/env.cc b/src/env.cc index e3e06598a57529..f2cb5243944c9d 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1643,7 +1643,7 @@ size_t Environment::NearHeapLimitCallback(void* data, env->isolate()->RemoveNearHeapLimitCallback(NearHeapLimitCallback, initial_heap_limit); - heap::WriteSnapshot(env->isolate(), filename.c_str()); + heap::WriteSnapshot(env, filename.c_str()); env->heap_limit_snapshot_taken_ += 1; // Don't take more snapshots than the number specified by diff --git a/src/heap_utils.cc b/src/heap_utils.cc index bc779489637048..83334fa52695a4 100644 --- a/src/heap_utils.cc +++ b/src/heap_utils.cc @@ -308,21 +308,26 @@ class HeapSnapshotStream : public AsyncWrap, HeapSnapshotPointer snapshot_; }; -inline void TakeSnapshot(Isolate* isolate, v8::OutputStream* out) { +inline void TakeSnapshot(Environment* env, v8::OutputStream* out) { HeapSnapshotPointer snapshot { - isolate->GetHeapProfiler()->TakeHeapSnapshot() }; + env->isolate()->GetHeapProfiler()->TakeHeapSnapshot() }; snapshot->Serialize(out, HeapSnapshot::kJSON); } } // namespace -bool WriteSnapshot(Isolate* isolate, const char* filename) { +bool WriteSnapshot(Environment* env, const char* filename) { FILE* fp = fopen(filename, "w"); - if (fp == nullptr) + if (fp == nullptr) { + env->ThrowErrnoException(errno, "open"); return false; + } FileOutputStream stream(fp); - TakeSnapshot(isolate, &stream); - fclose(fp); + TakeSnapshot(env, &stream); + if (fclose(fp) == EOF) { + env->ThrowErrnoException(errno, "close"); + return false; + } return true; } @@ -374,7 +379,7 @@ void TriggerHeapSnapshot(const FunctionCallbackInfo& args) { if (filename_v->IsUndefined()) { DiagnosticFilename name(env, "Heap", "heapsnapshot"); - if (!WriteSnapshot(isolate, *name)) + if (!WriteSnapshot(env, *name)) return; if (String::NewFromUtf8(isolate, *name).ToLocal(&filename_v)) { args.GetReturnValue().Set(filename_v); @@ -384,7 +389,7 @@ void TriggerHeapSnapshot(const FunctionCallbackInfo& args) { BufferValue path(isolate, filename_v); CHECK_NOT_NULL(*path); - if (!WriteSnapshot(isolate, *path)) + if (!WriteSnapshot(env, *path)) return; return args.GetReturnValue().Set(filename_v); } diff --git a/src/node_internals.h b/src/node_internals.h index fc82053cdbd262..5a33dad196dd95 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -379,7 +379,7 @@ class DiagnosticFilename { }; namespace heap { -bool WriteSnapshot(v8::Isolate* isolate, const char* filename); +bool WriteSnapshot(Environment* env, const char* filename); } class TraceEventScope { diff --git a/test/sequential/test-heapdump.js b/test/sequential/test-heapdump.js index 61a089dc677b70..e18c6189332574 100644 --- a/test/sequential/test-heapdump.js +++ b/test/sequential/test-heapdump.js @@ -24,6 +24,19 @@ process.chdir(tmpdir.path); fs.accessSync(heapdump); } +{ + const readonlyFile = 'ro'; + fs.writeFileSync(readonlyFile, Buffer.alloc(0), { mode: 0o444 }); + assert.throws(() => { + writeHeapSnapshot(readonlyFile); + }, (e) => { + assert.ok(e, 'writeHeapSnapshot should error'); + assert.strictEqual(e.code, 'EACCES'); + assert.strictEqual(e.syscall, 'open'); + return true; + }); +} + [1, true, {}, [], null, Infinity, NaN].forEach((i) => { assert.throws(() => writeHeapSnapshot(i), { code: 'ERR_INVALID_ARG_TYPE',