From a1c82c39af812b54cd3dd1e472c9088457fb7dc5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C8=98tefan=20Talpalaru?= <stefantalpalaru@yahoo.com>
Date: Wed, 19 May 2021 19:19:11 +0200
Subject: [PATCH] asyncdispatch+stackTraceOverride: fix premature collection
 (#18039) [backport:1.2]

Copying StackTraceEntry instances when nimStackTraceOverride is defined
breaks the link between a cstring field that's supposed to point at
another string field in the same object.

Sometimes, the original object is garbage collected, that memory region
reused for storing other strings, so when the StackTraceEntry copy tries
to use its cstring pointer to construct a traceback message, it accesses
unrelated strings.

This only happens for async tracebacks and this patch prevents that by
making sure we only use the string fields when nimStackTraceOverride is
defined.

Async tracebacks also beautified slightly by getting rid of an extra line
that was supposed to be commented out, along with the corresponding debugging output.

There's also a micro-optimisation to avoid concatenating two strings just
to get their combined length.
---
 changelog.md                     |  1 +
 lib/pure/asyncfutures.nim        | 46 ++++++++++++++++++++++----------
 lib/system/exceptions.nim        |  2 +-
 tests/async/tasync_traceback.nim |  4 +--
 4 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/changelog.md b/changelog.md
index f9c48e0bf787..992cd89c0d63 100644
--- a/changelog.md
+++ b/changelog.md
@@ -316,6 +316,7 @@
 
 - Added `copyWithin` [for `seq` and `array` for JavaScript targets](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/copyWithin).
 
+- Fixed premature garbage collection in asyncdispatch, when a stack trace override is in place.
 
 ## Language changes
 
diff --git a/lib/pure/asyncfutures.nim b/lib/pure/asyncfutures.nim
index 0178ee4d6e13..af3fbb3ecb4a 100644
--- a/lib/pure/asyncfutures.nim
+++ b/lib/pure/asyncfutures.nim
@@ -278,19 +278,33 @@ proc `callback=`*[T](future: Future[T],
   ## If future has already completed then `cb` will be called immediately.
   future.callback = proc () = cb(future)
 
+template getFilenameProcname(entry: StackTraceEntry): (string, string) =
+  when compiles(entry.filenameStr) and compiles(entry.procnameStr):
+    # We can't rely on "entry.filename" and "entry.procname" still being valid
+    # cstring pointers, because the "string.data" buffers they pointed to might
+    # be already garbage collected (this entry being a non-shallow copy,
+    # "entry.filename" no longer points to "entry.filenameStr.data", but to the
+    # buffer of the original object).
+    (entry.filenameStr, entry.procnameStr)
+  else:
+    ($entry.filename, $entry.procname)
+
 proc getHint(entry: StackTraceEntry): string =
   ## We try to provide some hints about stack trace entries that the user
   ## may not be familiar with, in particular calls inside the stdlib.
+
+  let (filename, procname) = getFilenameProcname(entry)
+
   result = ""
-  if entry.procname == cstring"processPendingCallbacks":
-    if cmpIgnoreStyle(entry.filename, "asyncdispatch.nim") == 0:
+  if procname == "processPendingCallbacks":
+    if cmpIgnoreStyle(filename, "asyncdispatch.nim") == 0:
       return "Executes pending callbacks"
-  elif entry.procname == cstring"poll":
-    if cmpIgnoreStyle(entry.filename, "asyncdispatch.nim") == 0:
+  elif procname == "poll":
+    if cmpIgnoreStyle(filename, "asyncdispatch.nim") == 0:
       return "Processes asynchronous completion events"
 
-  if entry.procname.endsWith(NimAsyncContinueSuffix):
-    if cmpIgnoreStyle(entry.filename, "asyncmacro.nim") == 0:
+  if procname.endsWith(NimAsyncContinueSuffix):
+    if cmpIgnoreStyle(filename, "asyncmacro.nim") == 0:
       return "Resumes an async procedure"
 
 proc `$`*(stackTraceEntries: seq[StackTraceEntry]): string =
@@ -303,16 +317,20 @@ proc `$`*(stackTraceEntries: seq[StackTraceEntry]): string =
   # Find longest filename & line number combo for alignment purposes.
   var longestLeft = 0
   for entry in entries:
-    if entry.procname.isNil: continue
+    let (filename, procname) = getFilenameProcname(entry)
+
+    if procname == "": continue
 
-    let left = $entry.filename & $entry.line
-    if left.len > longestLeft:
-      longestLeft = left.len
+    let leftLen = filename.len + len($entry.line)
+    if leftLen > longestLeft:
+      longestLeft = leftLen
 
   var indent = 2
   # Format the entries.
   for entry in entries:
-    if entry.procname.isNil:
+    let (filename, procname) = getFilenameProcname(entry)
+
+    if procname == "":
       if entry.line == reraisedFromBegin:
         result.add(spaces(indent) & "#[\n")
         indent.inc(2)
@@ -321,11 +339,11 @@ proc `$`*(stackTraceEntries: seq[StackTraceEntry]): string =
         result.add(spaces(indent) & "]#\n")
       continue
 
-    let left = "$#($#)" % [$entry.filename, $entry.line]
+    let left = "$#($#)" % [filename, $entry.line]
     result.add((spaces(indent) & "$#$# $#\n") % [
       left,
       spaces(longestLeft - left.len + 2),
-      $entry.procname
+      procname
     ])
     let hint = getHint(entry)
     if hint.len > 0:
@@ -349,9 +367,9 @@ proc injectStacktrace[T](future: Future[T]) =
     newMsg.add($entries)
 
     newMsg.add("Exception message: " & exceptionMsg & "\n")
-    newMsg.add("Exception type:")
 
     # # For debugging purposes
+    # newMsg.add("Exception type:")
     # for entry in getStackTraceEntries(future.error):
     #   newMsg.add "\n" & $entry
     future.error.msg = newMsg
diff --git a/lib/system/exceptions.nim b/lib/system/exceptions.nim
index b5f4fc3255a2..5dcd77bd0be8 100644
--- a/lib/system/exceptions.nim
+++ b/lib/system/exceptions.nim
@@ -29,7 +29,7 @@ type
       programCounter*: uint ## Program counter - will be used to get the rest of the info,
                             ## when `$` is called on this type. We can't use
                             ## "cuintptr_t" in here.
-      procnameStr*, filenameStr*: string ## GC-ed objects holding the cstrings in "procname" and "filename"
+      procnameStr*, filenameStr*: string ## GC-ed alternatives to "procname" and "filename"
 
   Exception* {.compilerproc, magic: "Exception".} = object of RootObj ## \
     ## Base exception class.
diff --git a/tests/async/tasync_traceback.nim b/tests/async/tasync_traceback.nim
index 9f787929b33a..cd16b2257f57 100644
--- a/tests/async/tasync_traceback.nim
+++ b/tests/async/tasync_traceback.nim
@@ -86,7 +86,7 @@ Async traceback:
     asyncfutures\.nim\(\d+?\)\s+?read
   \]#
 Exception message: b failure
-Exception type:
+
 
 bar failure
 Async traceback:
@@ -114,7 +114,7 @@ Async traceback:
     asyncfutures\.nim\(\d+?\)\s+?read
   \]#
 Exception message: bar failure
-Exception type:
+
 """
 
 # TODO: is asyncmacro good enough location for fooIter traceback/debugging? just put the callsite info for all?