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: #41346 PR-URL: #41373 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
nodejs · Jan 12, 2022 · 74b9baa4265a8f0d112dabc3efc63c2e13276488 · 74b9baa
1 parent 36035e0
commit 74b9baa4265a8f0d112dabc3efc63c2e13276488
Showing with 32 additions and 10 deletions.

+4 −0 doc/api/v8.md

+1 −1 src/env.cc

+13 −8 src/heap_utils.cc

+1 −1 src/node_internals.h

+13 −0 test/sequential/test-heapdump.js
diff --git a/doc/api/v8.md b/doc/api/v8.md
@@ -267,6 +267,10 @@ disk unless [`v8.stopCoverage()`][] is invoked before the process exits.
 
 <!-- YAML
 added: v11.13.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/41373
+    description: An exception will now be thrown if the file could not be written.
 -->
 
 * `filename` {string} The file path where the V8 heap snapshot is to be

diff --git 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
@@ -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<Value>& 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<Value>& 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
@@ -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
@@ -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',