Skip to content
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.

Sign up for GitHub

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

Jump to bottom

RzSearch refactor #4762

Draft
wants to merge 21 commits into
base: dev
Choose a base branch
from
Draft

RzSearch refactor #4762

wants to merge 21 commits into from

Conversation

Rot127
Copy link
Member

@Rot127 Rot127 commented Dec 10, 2024 •
edited
Loading

WIP

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the rizin book with the relevant information (if needed)

Detailed description

Companion PR: rizinorg/rizin-testbins#162

What happens here:
Slowly copying changes from https://github.com/Rot127/rizin/tree/rz-search-reference (which is #4742 with some comments already addressed).

Will resend to fuzz-dist when all the tests pass here.

Stuff to do (without any order)

  • Move essential changes from https://github.com/Rot127/rizin/tree/rz-search-reference / Add / (search) commands to newshell + rewrite of rz_search #4742 before starting with commands
  • Fix commands
    • Bytes search
      • Implement
      • Fix suggestions
      • Use RzBuffer
      • Document usage in book (normal, wildcards, different maps, searching with debug + don't forget to run until mapped, API).
    • String search
      • Implement
      • Fix suggestions
      • Testing
        • Case insensitivity on non latin alphabets.
        • Encoding + endianess
        • diacritics
        • Search for strings less then 4 bytes.
        • Search within buffers which have no \0 in it.
      • Document in book (basic usage, API, usage of distance, behavior of edge cases of distance: utf8 and other encodings, diacritics), case (in)sensitive.
  • Update book (with example API implementation).
  • Implement general search IO API which doesn't require knowledge of search spaces (what rz_search_run() was before).
  • Show progress of search.
  • Use RzBuffer for byte space searches
  • search.align shoudl default to search.alignment=asm.cpu_bits/8.
  • (Optional) Allow to cancel search with ctrl + c
  • (Optional) Extend RzSearchHit to hold more complex data.
  • (Optional) Add option to define search/divide strategy.
  • (Optional) Dispatch views into IO as chunks to the find() callback.

Test plan

...

Closing issues

closes #1586

@Rot127 Rot127 force-pushed the rz-search branch 2 times, most recently from 0c558c2 to b59292d Compare December 12, 2024 19:41
wargio
wargio reviewed Dec 30, 2024
View reviewed changes
librz/include/rz_search.h Outdated Show resolved Hide resolved
wargio
wargio reviewed Dec 30, 2024
View reviewed changes
librz/include/rz_search.h Outdated Show resolved Hide resolved
@wargio

This comment was marked as resolved.

Sign in to view
@Rot127

This comment was marked as resolved.

Sign in to view
Rot127 added 7 commits January 2, 2025 11:39
@Rot127
Sorry mister, we don't do search no longer.
844c05d
@Rot127
Add RzShell handlers for all search commands.
f0cbdcd
@Rot127
Import new RzSearch from reference branch.
61ac6e1 
This makes only small edits (renames) to keep the build working.
@Rot127
Reimplement hex bytes (/x) search.
f4240bc 
This sets search.overlap=true by default.
Mostly because I think this what people expect.
Also, with the search.align or search.alignment options there will
be the option to search at alignments.

This also adds some hex string to bytes helpers.
Fixes them and adds tests.

Unfinished reimplementation of /x.

Fix up the hex string to buffer functions.

Fix fast mask comparison macro.

Fix byte comparison with UB.

Invert condition to match documentation of callback

Handle search options from the settings and add tests.

This sets search.overlap=true by default.
Mostly because I think this what people expect.
Also, with the search.align or search.alignment there are way
better options to consider for seraching not overlapping data.

Fix rz_hex_str2bin_mask() did set mask bits for 0

Refine documentation for rz_hex_str2bin functions.

Add byte search tests.

Don't allow wildcards with a custom mask.

Change buffer to chunk size and make it a search setting.

Make search over bytes more efficient. Searching over windows of memory.

Add some helper functions to read from IO into a RzBuffer

Simplify buffer management and fix leaks

Fix bugs add tests regarding search windows

Revert design with RzBuffer due to rizinorg#4769

Fix several stupidity bugs

Change order of byte pattern and mask.

Apply suggestions.
@Rot127
Add search test within debug maps.
cfbfe45
@Rot127
Make BytesPattern implementation private.
d9c4159
@Rot127
Use RzBuffer for byte search.
f8cac67
wargio
wargio reviewed Jan 3, 2025
View reviewed changes
librz/core/cconfig.c
RzConfigNode *node = (RzConfigNode *)data;
RzThreadNCores max_threads = rz_th_max_threads(node->i_value);
if (node->value[0] == '?') {
rz_cons_printf("%d\n", max_threads);
Copy link
Member

Choose a reason for hiding this comment

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

This should return ncores. It's under rz_th.h

librz/core/cmd_descs/cmd_search.yaml
- name: opcodes
type: RZ_CMD_ARG_TYPE_STRING

- name: "/c"
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking to change this and have one command which search for all or one specific material

Rot127 added 14 commits January 3, 2025 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wargio wargio wargio left review comments

@ret2libc ret2libc Awaiting requested review from ret2libc ret2libc will be requested when the pull request is marked ready for review ret2libc is a code owner

@thestr4ng3r thestr4ng3r Awaiting requested review from thestr4ng3r thestr4ng3r will be requested when the pull request is marked ready for review thestr4ng3r is a code owner

@XVilka XVilka Awaiting requested review from XVilka XVilka will be requested when the pull request is marked ready for review XVilka is a code owner

@kazarmy kazarmy Awaiting requested review from kazarmy kazarmy will be requested when the pull request is marked ready for review kazarmy is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
API blocker high-priority rz-test RzCore RzIO RzSearch RzUtil
Projects
None yet
Milestone
0.8.0
Development

Successfully merging this pull request may close these issues.

Port / (search) to rzshell
3 participants
@Rot127 @wargio @XVilka