Don't let backtrace_cleanup_callback affect abs_path and separate filename handling#2474
Closed
sl0thentr0py wants to merge 1 commit into
Closed
Don't let backtrace_cleanup_callback affect abs_path and separate filename handling#2474sl0thentr0py wants to merge 1 commit into
backtrace_cleanup_callback affect abs_path and separate filename handling#2474sl0thentr0py wants to merge 1 commit into
Conversation
sl0thentr0py
changed the title
backtrace_cleanup_callback affect abs_path and separate filename handling
Member
Author
|
NOTE: since this affects the payload and grouping, I explicitly tested that the old and new payloads end up in the same issue group and it seems fine since we are only changing the underlying |
sl0thentr0py
force-pushed
the
neel/fix-rails-bt-linecache
branch
from
November 25, 2024 16:42
acca59c to
7027718
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2474 +/- ##
=======================================
Coverage 98.17% 98.17%
=======================================
Files 128 128
Lines 4825 4833 +8
=======================================
+ Hits 4737 4745 +8
Misses 88 88
|
sl0thentr0py
force-pushed
the
neel/fix-rails-bt-linecache
branch
4 times, most recently
from
November 26, 2024 12:41
e513e20 to
d5ffd07
Compare
sl0thentr0py
commented
Nov 26, 2024
| _, file, number, _, method = ruby_match.to_a | ||
| file.sub!(/\.class$/, RB_EXTENSION) | ||
| _, abs_path, number, _, method = ruby_match.to_a | ||
| abs_path.sub!(/\.class$/, RB_EXTENSION) |
Member
Author
There was a problem hiding this comment.
the original file is now the abs_path and the file is computed if necessary later properly
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
sl0thentr0py
force-pushed
the
neel/fix-rails-bt-linecache
branch
from
November 26, 2024 13:46
d5ffd07 to
4a00256
Compare
| _, module_name, method, abs_path, number = java_match.to_a | ||
|
|
||
|
|
||
| cleaned_match = cleaned_line.match(JAVA_INPUT_FORMAT) if cleaned_line |
Check failure
Code scanning / CodeQL
Polynomial regular expression used on uncontrolled data
At some point, rails added a [default gem filter to their `BacktraceCleaner`](https://github.com/rails/rails/blob/a8709e6ea26eca73a652af4fdd0a9f7db5352af4/activesupport/lib/active_support/backtrace_cleaner.rb#L118-L125) which messed up our linecache/context lines parsing logic since we get paths like ``` activesupport (7.1.2) lib/active_support/callbacks.rb ``` instead of raw paths. This PR cleans up handling this case by making a clear distinction between `abs_path` and `filename` as was intended in the original relay protocol and we were never populating these properly.
sl0thentr0py
force-pushed
the
neel/fix-rails-bt-linecache
branch
from
November 26, 2024 14:22
4a00256 to
2309e5b
Compare
Member
Author
|
testing all frames have context now 
grouping is the same 
|
Contributor
|
Please hold off merging this, I think the cause may be at other places and thus require a different change. |
sl0thentr0py
marked this pull request as draft
November 27, 2024 14:08
Member
Author
|
closed in favor of #2475 |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
At some point, rails added a default gem filter to their
BacktraceCleanerwhich messed up our linecache/context lines parsing logic since we get paths likeinstead of raw paths.
This PR cleans up handling this case by making a clear distinction between
abs_pathandfilenameas was intended in the original relay protocol and we were never populating these properly.Fixes #2472