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

Add a hotkey for toggling immediate base (#2420) #2429

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

Conversation

plaets
Copy link

@plaets plaets commented Sep 23, 2020

Your checklist for this pull request

Detailed description

This pull request implements issue #2420
Currently, the hotkey to toggle the base is set to h, however, I see that Cutter tries to mimic some vim key bindings (j and k) and because of that h might not be the best option. I've also considered b or maybe even t but I would need some feedback on that.

As I already mentioned in the comments of #2420:

the user needs to press the hotkey twice to switch from decimal to hexadecimal base (but only if the value is decimal by default, after the first switch the hotkey works as I expect it to). That is because I'm using ahj to get the current immediate base, and it looks like this command specifies it only if it was changed before. If there is no immbase in ahj output I just assume that the value is hexadecimal, but this is not always true.

I'm not sure how to fix this yet.
(Also, since I reverted 1473d2 this works in graph view)

Another (potential) problem is that this hotkey is not respecting the condition from
https://github.com/radareorg/cutter/blob/ad66718f375b013a3021e531891c71082ee48867/src/menus/DisassemblyContextMenu.cpp#L415-L421
I can't just disable it from there, since aboutToShowSlot is run only on the context menu trigger. That would mean that the hotkey would be disabled every time a user right-clicks on an address that does not meet this condition and enabled only after the user right-clicks on an address that meets this condition. I thought about adding a method to disable/enable the hotkey that would be called every time DissasemblyWidget/GraphWidget is updated, but I'm not sure if this is the best approach and where exactly such method should be called.

Test plan (required)

  1. Go to the disassembly/graph tab
  2. Find an instruction with a numerical value
  3. Press h or right-click -> Toggle Immediate Base (Hex/Dec)

cutter2420

Closing issues

closes #2420

@XVilka XVilka changed the title Add a hotkey for toggling immidiate base (#2420) Add a hotkey for toggling immediate base (#2420) Sep 24, 2020
@ITAYC0HEN ITAYC0HEN self-requested a review September 25, 2020 08:02
Copy link
Member

@ITAYC0HEN ITAYC0HEN left a comment

Choose a reason for hiding this comment

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

Hey @plaets thanks for a very nice pull request! Welcome to Cutter <3
This feature is a very good addition as it makes it easier to perform common task such as replacing immediate base.

Before even looking at the code, I have some design concerns I wish to share:

  1. The item is shown even when we don't have any candidate for immediate base change. We aspire to make the context menu as much context-aware as possible. And this means not to show redundant menu items that can be irrelevant.
    image

  2. Instead of showing "Toggle immediate base (Dec/Hex)" I'd prefer to see the outcome. If it is decimal, show me the hex value in the menu (and in the future also the string value) and if it is hex, show em the decimal.
    image

image

  1. as you very wisely mentioned, it's bad that users need to press twice on H in order to change the initial immediate base. We can discuss with people in radare2 as well as trying to figure it out by ourselves.

@plaets
Copy link
Author

plaets commented Sep 25, 2020

Thanks for the review!

  1. That's probably related to that check in DisassemblyContextMenu::aboutToShowSlot, I'm working on it, and hopefully, I will be able to push relevant changes soon.
  2. Sounds really cool! For that, the code would need to know which value will be changed by ahi, I just noticed that only the first value is affected (ex. byte [rax + 1], 0x30 - ahi 16 changes 1 to 0x1, after switching to att syntax: movb 0x30, -1(%rcx) - ahi 16 affects 0x30. But there could be exceptions from this rule, or maybe I'm just misunderstanding something). Not sure what to do next - the only thing I came up with is parsing the opcode, but that does not sound like a good idea knowing how many different instruction sets/syntaxes radare2 supports.
  3. Honestly, I'm out of ideas for this one. I tried reading radare2 source code, and I believe that there is no way to get the default immediate base without reimplementing parts of radare2 (but I could be very wrong). Having the value (with analysis hints applied) that will be affected by ahi would be much better since that helps us solve 2. But I'm again not sure how to implement that... That's one of my first more serious contributions (at least more serious than anything before), and I'm not sure if I should ask on IRC or open an issue on radare2 😅

src/menus/DisassemblyContextMenu.cpp Outdated Show resolved Hide resolved
src/menus/DisassemblyContextMenu.cpp Outdated Show resolved Hide resolved
src/menus/DisassemblyContextMenu.h Outdated Show resolved Hide resolved
@ITAYC0HEN
Copy link
Member

I just noticed that only the first value is affected

Good point in your (2) paragraph. Maybe @karliss or @thestr4ng3r will know better

For point (3), it might require solving it in radare2 to add the immediate base. Need to ask in radare2 group and bug tracker. Maybe @karliss and @thestr4ng3r can help with it as well

(cc @ret2libc for point (3) as well, you might know how to solve this issue that the immediate base isn't available)

@ret2libc
Copy link
Member

I think there is no way currently to determine what is the base used for a number when no hint is defined yet. I see https://github.com/radareorg/radare2/blob/master/libr/parse/filter.c#L402 , which seems to mean that whatever is returned by the disassembler is shown to the user. Capstone seems to show the number as "decimal" if < 10 (by "decimal" i mean that it doesn't show 0x, but of course numbers < 10 are the same in hex and dec), but you probably can't assume that, as radare2 supports multiple disassemblers.

With regards to which number the hint is applied, you could use ahi1 <base> (or ahi2 <base, etc.) to apply the hint to the second (or third, etc.) number. However, it doesn't work very well as you can't specify the hint for both numbers, just for one at a time.

I think the best thing would be to open an issue on radare2 repository and discuss it there, but probably r2 should provide an API to get these kind of info, though it could be harder than what we think because, as said above, when no hints are set we just get the disassembler default.

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.

Add a hotkey for toggling immidiate base
3 participants