rizin icon indicating copy to clipboard operation
rizin copied to clipboard

Add confirmation to large hexdump reads.

Open vykt opened this issue 7 months ago • 3 comments

Your checklist for this pull request

  • [x] I've read the guidelines for contributing to this repository.
  • [x] I made sure to follow the project's coding style.
  • [x] I've documented every RZ_API function and struct this PR changes.
  • [x] I've added tests that prove my changes are effective (required for changes to RZ_API).
  • [x] I've updated the Rizin book with the relevant information (if needed).

Detailed description

If you mistakenly provide a very large length parameter to px (in my case, px rsp), rizin will continue the read until it is complete. This can take several minutes, and can't be cancelled with SIGINT.

This PR adds a check via rz_cons_yesno(), requesting the user to confirm hexdumps above some threshhold. For now, this threshhold is 0x1000.

If a rz_core_print_hexdump_or_hexdiff_str() returns NULL, an additional error message is printed. To avoid this message in case the user answers n to the confirmation, the length is now normalised and confirmed before calling rz_core_print_hexdump_or_hexdiff_str().

I imagine the real solution here is to register a temporary SIGINT handler that would cancel the read. I'm inclined to say that an even better solution is probably to register a generic SIGINT handler, and have it call a callback for whichever operation is currently ongoing. I'm not sure how feasible that is given the current architecture. I'm also not familiar with the codebase enough yet to implement something like this.

...

Test plan

$ rizin -d a.out
> s main
> px 0x40000000
[attempt both y & n paths]

I'd be happy to define some tests for this if you'd like.

...

Closing issues

N/A ...

vykt avatar May 19 '25 22:05 vykt

I should note that other calls to rz_core_print_hexdump_or_hexdiff_str() pass constants below core->blocksize_max as the length (core->blocksize & 128), so normalisation can safely be moved up the callstack here.

vykt avatar May 19 '25 22:05 vykt

 Formatting [84/1854] librz/core/cprint.c
librz/core/cprint.c:390:18: error: code should be clang-formatted [-Wclang-format-violations]
        if (len > 0x1000
                        ^

notxvilka avatar May 20 '25 04:05 notxvilka

Codecov Report

Attention: Patch coverage is 25.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 44.87%. Comparing base (76d41f7) to head (0fa999c). Report is 8 commits behind head on dev.

Files with missing lines Patch % Lines
librz/core/cprint.c 25.00% 2 Missing and 1 partial :warning:
Additional details and impacted files
Files with missing lines Coverage Δ
librz/core/cprint.c 65.75% <25.00%> (-0.29%) :arrow_down:

... and 11 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 76d41f7...0fa999c. Read the comment docs.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar May 20 '25 05:05 codecov[bot]