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

core/cmd: port /R commands to the rzshell #4504

Merged
merged 14 commits into from
Jun 1, 2024
Merged

Conversation

giridharprasath
Copy link
Contributor

@giridharprasath giridharprasath commented May 20, 2024

SQUASH ME

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

  • Inital Port command yaml changes

ScreenShot_2024-05-24_at_06:36:51-PM

Test plan

...

Closing issues

...

@giridharprasath giridharprasath changed the title Initial command port template Initial command port template changes May 20, 2024
Copy link
Member

@ret2libc ret2libc left a comment

Choose a reason for hiding this comment

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

What is the issue related to this? Why under i (which is usually for binary-related info)? I think there's something like this already under /R.

Copy link
Member

@XVilka XVilka left a comment

Choose a reason for hiding this comment

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

Agree, they should be in the /R commands, just put them into:

  • librz/core/cmd_descs/cmd_search.yaml
  • librz/core/cmd/cmd_search_rop.c (this one should be updated from the old handlers to use the new handlers.

Moreover, I also recommend adding TABLE output, with more information about each gadget

@giridharprasath
Copy link
Contributor Author

Still WIP:

  • Fix memory leaks
  • Add table view for gadget
  • Regex filter is broken, should be fixed.

Copy link
Member

@XVilka XVilka left a comment

Choose a reason for hiding this comment

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

Thanks, much better now!

librz/core/cmd/cmd_search.c Outdated Show resolved Hide resolved
librz/core/cmd/cmd_search.c Outdated Show resolved Hide resolved
@giridharprasath
Copy link
Contributor Author

Some notes:

  • /Rk is unusable, Will be refactoring it and adding testcases for it in a separate PR.
  • Current table view with only columns:
[0x08048330]> /Rt
addr bytes disasm 
------------------

Please lmk if any other fields needed to be added which would be helpful in table mode. @XVilka

@XVilka
Copy link
Member

XVilka commented May 29, 2024

Some notes:

* `/Rk` is unusable, Will be refactoring it and adding testcases for it in a separate PR.

* Current table view with only columns:
[0x08048330]> /Rt
addr bytes disasm 
------------------

Please lmk if any other fields needed to be added which would be helpful in table mode. @XVilka

Should be enough for now. Further improvements could be done when necessary.

Copy link
Member

@XVilka XVilka left a comment

Choose a reason for hiding this comment

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

Looks good overall, please address my feedback first though

librz/core/cmd/cmd_search.c Outdated Show resolved Hide resolved
librz/core/cmd/cmd_search.c Outdated Show resolved Hide resolved
librz/core/cmd/cmd_search.c Outdated Show resolved Hide resolved
@XVilka

This comment was marked as resolved.

@XVilka XVilka changed the title Initial command port template changes core/cmd: port /R commands to the rzshell Jun 1, 2024
@XVilka XVilka merged commit 6e4ea01 into rizinorg:dev Jun 1, 2024
44 checks passed
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.

None yet

3 participants