-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Secure array slicing when expanding macro for stack trace #11109
Secure array slicing when expanding macro for stack trace #11109
Conversation
src/compiler/crystal/exception.cr
Outdated
@@ -254,7 +254,8 @@ module Crystal | |||
io << Crystal.with_line_numbers(source, line_number, @color) | |||
else | |||
from_index = [0, line_number - MACRO_LINES_TO_SHOW].max | |||
source_slice = source.lines[from_index...line_number] | |||
to_index = [source.lines.size, line_number].max |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to_index = [source.lines.size, line_number].max | |
to_index = {source.lines.size, line_number}.max |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why ? Has it performance benefits ?
In any case, if I do this change, it would be good to modify the line above too, isn't it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why ? Has it performance benefits ?
Yes, Tuple
is stack-allocated, unlike the heap-allocated Array
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another alternative is Math.max
which behaves slightly differently for floating-point values (not relevant here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to clamp both values to 0..source.lines.size
to prevent any kind of issue with weird values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be a min
? to_index
is the upper bound of the slice range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lbguilherme Oopsie.
I'm updating with @straight-shoota suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just updated the code. I also fixed from_index
value definition so it keeps consistent with to_index
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @pyrsmk 🙏
83f5096
to
2bd10da
Compare
Thanks ;) I just rebased my fixups so this PR is ready to be merged. |
Thanks. But please merge master instead of a rebase next time. This preserves review history. |
@straight-shoota Mmmh ok. So should it be better to let the rebase to admins next time ? Because keeping fixup commits on the history is not really clean either. |
Yeah, we don't want to keep the fixup commits. We're squashing all commits when merging to master. |
As #11008 I fell upon a bug into the stack trace generation. This time, I haven't the stack trace that was occuring with this error, but it wasn't very talkative about the origin of the problem.
Anyway, it is possible the two issues are related. The previous PR was fixing an issue with colum numbers, and this one is dealing with line numbers. This time, there's a crash because the caller is passing an incorrect line number for a macro that is less tinier than expected.
I tried to understand why but sadly I couldn't find a way to know what the original caller of that stack trace printing was.
Since it's the second time I encounter this kind of bug, maybe it would be good to verify the globality of
src/compiler/crystal/exception.cr
andsrc/compiler/crystal/semantic/exception.cr
to avoid these errors ?