Skip to content

Conversation

@korniltsev
Copy link

@korniltsev korniltsev commented Apr 11, 2025

Inspirations:
benfred#755
benfred#736

  • fix oob read panic
  • fix debug int overflow panics
  • add fuzzer

@korniltsev korniltsev requested review from a team and simonswine April 11, 2025 05:57
@korniltsev korniltsev force-pushed the get_line_number_py11_fix branch 3 times, most recently from 1181d9b to 99c9866 Compare April 11, 2025 07:21
@korniltsev korniltsev requested a review from Copilot April 11, 2025 07:21
@korniltsev korniltsev marked this pull request as ready for review April 11, 2025 07:21
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

src/stack_trace.rs:252

  • [nitpick] Consider providing additional context in the error message (for example, including the value of 'lasti') so that debugging failures in line number lookup becomes easier.
.or_else(|err| bail!("Failed to get line number {err:?}"))

src/line_numbers.rs:4

  • [nitpick] Consider implementing Display and std::error::Error for the Error type to supply more useful information when errors occur.
pub struct Error;

@korniltsev
Copy link
Author

added a fuzzer, imediately found more problems

stack backtrace:
   0: __rustc::rust_begin_unwind
             at /rustc/0fe8f3454dbe9dda52a254991347e96bec579a6f/library/std/src/panicking.rs:697:5
   1: core::panicking::panic_fmt
             at /rustc/0fe8f3454dbe9dda52a254991347e96bec579a6f/library/core/src/panicking.rs:75:14
   2: core::panicking::panic_const::panic_const_sub_overflow
             at /rustc/0fe8f3454dbe9dda52a254991347e96bec579a6f/library/core/src/panicking.rs:178:21
   3: <py_spy::python_bindings::v3_13_0::PyCodeObject as py_spy::python_interpreters::CodeObject>::get_line_number
             at ./src/python_interpreters.rs:281:29
   4: py_spy::python_interpreters::tests::test_line_numbers
             at ./src/python_interpreters.rs:879:21
   5: py_spy::python_interpreters::tests::test_line_numbers::{{closure}}
             at ./src/python_interpreters.rs:793:27
   6: core::ops::function::FnOnce::call_once

@korniltsev korniltsev marked this pull request as draft April 11, 2025 08:18
@korniltsev korniltsev marked this pull request as ready for review April 11, 2025 08:52
@korniltsev korniltsev force-pushed the get_line_number_py11_fix branch from c7f3dc0 to ba5d000 Compare April 11, 2025 08:53
@korniltsev korniltsev marked this pull request as draft April 11, 2025 09:01
@korniltsev korniltsev force-pushed the get_line_number_py11_fix branch from fd623fa to 7ec9e14 Compare April 11, 2025 09:10
@korniltsev korniltsev requested a review from Copilot April 11, 2025 09:11
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.

Files not reviewed (1)
  • fuzz/.gitignore: Language not supported

@korniltsev korniltsev force-pushed the get_line_number_py11_fix branch 5 times, most recently from 096cd9a to 144fb10 Compare April 11, 2025 10:34
@korniltsev korniltsev marked this pull request as ready for review April 11, 2025 10:42
- fix oob read panic
- fix int overflow in debug mode
- add fuzzer
- lintt
- fix upload artifacts names
- maybe fix CI
Copy link

@kolesnikovae kolesnikovae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@korniltsev korniltsev requested a review from kolesnikovae April 14, 2025 10:20
@korniltsev korniltsev merged commit 062d827 into master Apr 14, 2025
51 checks passed
korniltsev added a commit to grafana/pyroscope-rs that referenced this pull request Apr 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants