From bc626c237be17352a12ca2ca0ad194c70fe613a0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Antonio=20Rom=C3=A1n?= <kyradiscord@gmail.com>
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 <anna@addaleax.net>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
---
 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.
 
 <!-- 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
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<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
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',