asyncdispatch+stackTraceOverride: fix premature garbage collection #18039
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Araq
reviewed
May 19, 2021
lib/pure/asyncfutures.nim
Outdated
| @@ -299,20 +312,23 @@ proc `$`*(stackTraceEntries: seq[StackTraceEntry]): string = | |||
| else: | |||
| let entries = stackTraceEntries | |||
|
|
|||
| result = "" | |||
There was a problem hiding this comment.
Don't delete this assignment, Nim will get definite assignment analysis for all types.
There was a problem hiding this comment.
Empty string assignments brought back.
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.
dom96
approved these changes
May 19, 2021
I don't like breaking backwards compatibility in a public API. We committed the sin, we need to pay the price by maintaining that API for as long as we can ;-) |
narimiran
pushed a commit
that referenced
this pull request
May 19, 2021
…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. (cherry picked from commit a1c82c3)
|
well, this wouldn't be breaking backwards compat. It would be first a deprecation, we can figure out what to do next. The idea is to make it clear that these fields are dangerous. |
narimiran
pushed a commit
that referenced
this pull request
May 20, 2021
…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. (cherry picked from commit a1c82c3)
PMunch
pushed a commit
to PMunch/Nim
that referenced
this pull request
Mar 28, 2022
…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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 and some trivial cleanup.
It would make sense to backport this to 1.4 and 1.2.