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.

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

search: Print dot in results for nonprintable chars #4880

Merged
merged 2 commits into from
Feb 7, 2025

Conversation

kazarmy
Copy link
Member

@kazarmy kazarmy commented Feb 5, 2025

Your checklist for this pull request

Detailed description

#4762 hasn't landed yet (hopefully it'll be merged soon) so this pr improves slightly the current search results by using '.' for nonprintable chars instead of ignoring them, so that some additional information is provided in the results, and words don't run together.

Test plan

The change makes sense. All builds are green.

Closing issues

...

Copy link

@notxvilka notxvilka left a comment

Choose a reason for hiding this comment

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

Nice improvement!

Copy link
Member

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

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

Mh, idk. The behavior doesn't make sense to me. If the user searches for "test" but gets as a result ........test....... it is not a match. Doesn't it? It would be a match for .+test.+ (assuming . also includes non-printable characters).

I think it is better to wait for the refactor merge and edit the process_one_string() function (used in #4762 to discover strings) and escapes the non-printable chars their. It doesn't handle Unicode non-printables as well, but this is not too difficult to fix. Also we should give the option to define which non-printables should be included in a string and which are not (mark the end of a string).

@kazarmy
Copy link
Member Author

kazarmy commented Feb 6, 2025

Mh, idk. The behavior doesn't make sense to me. If the user searches for "test" but gets as a result ........test....... it is not a match. Doesn't it?

If console colors are enabled (which is usually the case), then "test" will be highlighted i.e. ........test........ Even if colors are not enabled, the user does know what is being searched and the searched string will usually be somewhere in the middle.

I think it is better to wait for the refactor merge...

There's no harm merging it now (from my perspective, you can choose to ignore this pr in the refactor and I will probably, eventually fix any regressions, time permitting, though they should be easy fixes now that I have an idea of what the code should look like), and it's certainly better than what is currently being used, e.g. see this before/after below of a search on an actual binary:

Before After
lib_search_before lib_search_after

Copy link
Member

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

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

Valid point, go ahead then.

@kazarmy
Copy link
Member Author

kazarmy commented Feb 7, 2025

Thanks! ☺️

@kazarmy kazarmy merged commit f2ddbd7 into rizinorg:dev Feb 7, 2025
46 checks passed
@kazarmy
Copy link
Member Author

kazarmy commented Feb 7, 2025

It would be a match for .+test.+ (assuming . also includes non-printable characters).

Hmm it would be good if an actual . can be differentiated from a . that is a non-printable character.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants