Reorder reset_arch: param forced, elf header, gdb conf #1004

josx · 2023-09-02T14:01:45Z

Description

Reorder how on reset_architecture fn is going to find arch. Now:

Param forced
Elf header
gdb conf

It is based on chat on #947 with @Grazfather

This change is needed because if first check for gdb conf if some has another language active would raise an exception making gef not work at all (some code match only english sentences).

This pattch would help non englihs users of gdb to not have many problems to use gef.

Checklist

My code follows the code style of this project.
My change includes a change to the documentation, if required.
If my change adds new code, adequate tests have been added.
I have read and agree to the CONTRIBUTING document.

github-actions · 2023-09-02T14:04:19Z

🤖 Coverage Update

Commit: 6a6e2a0
Current Coverage: 71.6614%
New Coverage: 71.6614%
Diff: 0.0

To this point, this PR:

does not include changes to tests
does not include changes to documentation
does not include forbidden words

hugsy · 2023-09-12T03:47:09Z

Hi @josx

First thanks for your PR but to be honest I'm not big on merging this: it doesn't fix the problem, just moves it as a last case hoping the first 2 conditions will be met.

Like I mentioned in the thread, the correct approach is to entirely remove the obsolete function get_arch by changing the only one spot it is still used, and where it parses the output string - expecting an english version of the error message.

Considering we only support ELF in GEF, having a valid gef.binary.e_machine should not be a big ask and that logic would make things a bit easier if/when we decide to finalize supporting PE/MachO.

josx · 2023-09-12T14:43:11Z

Sorry but i dont get very well, what do you want to do?
get rid of get_arch and just returning param set or e_machine?

hugsy · 2023-09-12T16:31:21Z

Sorry but i dont get very well, what do you want to do?
get rid of get_arch and just returning param set or e_machine?

The function get_arch() is an old function designed to abstract how GEF determines the architecture to use. Originally it was only using the show arch native GDB command and parsing the output string (where your problem originates).

Over time, other methods were added to determine the architecture to use. Currently the most reliable is through the parsing of the ELF file itself. So get_arch() which used to be used everywhere, is now only used in 1 location - in reset_architecture where you tried to apply your change.

gef/gef.py

Lines 3682 to 3695 in 5927df4

    
           gdb_arch = get_arch() 
        
           preciser_arch = next((a for a in arches.values() if a.supports_gdb_arch(gdb_arch)), None) 
        
           if preciser_arch: 
        
               gef.arch = preciser_arch() 
        
               return 
        
           # last resort, use the info from elf header to find it from the known architectures 
        
           try: 
        
               arch_name = gef.binary.e_machine if gef.binary else gdb_arch 
        
               gef.arch = arches[arch_name]() 
        
           except KeyError: 
        
               raise OSError(f"CPU type is currently not supported: {get_arch()}") 
        
           return

So what I'm suggesting is to re-work the logic so we don't need get_arch altogether, making the issue you're having nil.

There are currently (AFAIK) 2 ways to determine the architecture from the Python API, and both require the ELF to be loaded.

either using gdb.selected_frame().architecture() if the bin is running
either using the ELF header is not (gef.binary.e_machine)

So the only thing is in reset_architecture, we need is to make sure we match the architecture names for both cases so that doing gef.arch = arches[arch_name]() on L3692 (almost) never fails.

It is a bit more work but worth it, as we'll be also improving the perf (gdb.execute is very slow).

Hope this clarifies my point.

josx · 2023-09-12T18:08:54Z

Ok this clarify me a little more, something like this would work for you (BTW i get rid of get_arch):

def reset_architecture(arch: Optional[str] = None) -> None:
    """Sets the current architecture.
    If an architecture is explicitly specified by parameter, try to use that one. If this fails, an `OSError`
    exception will occur.
    If no architecture is specified, then GEF will attempt to determine automatically based on the current
    ELF target. If this fails, an `OSError` exception will occur.
    """
    global gef
    arches = __registered_architectures__

    # check if the architecture is forced by parameter
    if arch:
        try:
            gef.arch = arches[arch]()
        except KeyError:
            raise OSError(f"Specified arch {arch.upper()} is not supported")
        return

    # check for bin running
    if is_alive():
        try:
            arch = gdb.selected_frame().architecture()
            gef.arch = arches[arch.name()]()
        except KeyError:
            raise OSError(f"Running Binary arch {arch.name().upper()} is not supported")
        return

    # check for elf header architecture
    if gef.binary.e_machine:
        try:
            gef.arch = arches[gef.binary.e_machine]()
        except KeyError:
            raise OSError(f"CPU type is currently not supported: {gef.binary.e_machine}")
        return

    # Unknown, we throw an exception to be safe
    raise RuntimeError(f"Unknown architecture: {arch_str}")

hugsy · 2023-09-12T19:02:03Z

Yes, this is much closer to what I had in mind.

josx · 2023-09-12T20:43:25Z

Let me update de PR

github-actions · 2023-09-12T21:44:34Z

🤖 Coverage Update

Commit: 5927df4
Current Coverage: 71.6387%
New Coverage: 71.6387%
Diff: 0.0

To this point, this PR:

does not include changes to tests
does not include changes to documentation
does not include forbidden words

hugsy

seems all good, just minor changes

gef.py

hugsy · 2023-09-13T15:21:40Z

By the way @josx do you confirm this PR solves your problem?

github-actions · 2023-09-14T13:06:42Z

🤖 Coverage Update

Commit: 5927df4
Current Coverage: 71.6387%
New Coverage: 71.6387%
Diff: 0.0

To this point, this PR:

does not include changes to tests
does not include changes to documentation
does not include forbidden words

josx · 2023-09-14T13:28:18Z

By the way @josx do you confirm this PR solves your problem?

Yes, solves my problem.

josx · 2023-09-14T13:29:30Z

Updated with your comments.

hugsy

supports_gdb_arch was removed, it should not be as it is an important part of the logic to let users enforce their own gdb arch with gef. It should be restored

gef.py

…elf header Signed-off-by: José Luis Di Biase <[email protected]>

github-actions · 2023-09-14T18:50:39Z

🤖 Coverage Update

Commit: 5927df4
Current Coverage: 71.6387%
New Coverage: 71.6387%
Diff: 0.0

To this point, this PR:

does not include changes to tests
does not include changes to documentation
does not include forbidden words

hugsy · 2023-11-01T18:02:59Z

Sorry for the delay I was on long holiday. I'll review this PR asap

josx · 2023-11-02T01:28:44Z

No problem. Every one deserves a long vacation :)

hugsy

LGTM

hugsy · 2023-11-29T22:43:06Z

Quick local tests went ok so merging.
Thanks @josx 🙌 apologies for the (extreme) delay on this

josx mentioned this pull request Sep 2, 2023

Command 'registers' failed to execute properly, reason: max() arg is an empty sequence [Bug] #947

Closed

9 tasks

josx force-pushed the reorder_reset_arch branch from c49ea4f to 53d0aa5 Compare September 12, 2023 21:41

hugsy requested changes Sep 13, 2023

View reviewed changes

gef.py Show resolved Hide resolved

gef.py Outdated Show resolved Hide resolved

gef.py Outdated Show resolved Hide resolved

gef.py Outdated Show resolved Hide resolved

josx force-pushed the reorder_reset_arch branch from 53d0aa5 to 7042208 Compare September 14, 2023 13:03

hugsy requested changes Sep 14, 2023

View reviewed changes

gef.py Outdated Show resolved Hide resolved

gef.py Show resolved Hide resolved

Refactor reset_architecture: check for param forced, binary running, …

c34bda9

…elf header Signed-off-by: José Luis Di Biase <[email protected]>

josx force-pushed the reorder_reset_arch branch from 7042208 to c34bda9 Compare September 14, 2023 18:48

josx requested a review from hugsy October 2, 2023 12:56

hugsy approved these changes Nov 28, 2023

View reviewed changes

hugsy merged commit 295cbf7 into hugsy:main Nov 29, 2023

hugsy added this to the 2024.01 milestone Dec 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorder reset_arch: param forced, elf header, gdb conf #1004

Reorder reset_arch: param forced, elf header, gdb conf #1004

josx commented Sep 2, 2023 •

edited by hugsy

Loading

github-actions bot commented Sep 2, 2023

hugsy commented Sep 12, 2023 •

edited

Loading

josx commented Sep 12, 2023 •

edited

Loading

hugsy commented Sep 12, 2023

josx commented Sep 12, 2023

hugsy commented Sep 12, 2023 •

edited

Loading

josx commented Sep 12, 2023

github-actions bot commented Sep 12, 2023

hugsy left a comment

hugsy commented Sep 13, 2023

github-actions bot commented Sep 14, 2023

josx commented Sep 14, 2023

josx commented Sep 14, 2023

hugsy left a comment

github-actions bot commented Sep 14, 2023

hugsy commented Nov 1, 2023

josx commented Nov 2, 2023

hugsy left a comment

hugsy commented Nov 29, 2023

Reorder reset_arch: param forced, elf header, gdb conf #1004

Reorder reset_arch: param forced, elf header, gdb conf #1004

Conversation

josx commented Sep 2, 2023 • edited by hugsy Loading

Description

Checklist

github-actions bot commented Sep 2, 2023

🤖 Coverage Update

hugsy commented Sep 12, 2023 • edited Loading

josx commented Sep 12, 2023 • edited Loading

hugsy commented Sep 12, 2023

josx commented Sep 12, 2023

hugsy commented Sep 12, 2023 • edited Loading

josx commented Sep 12, 2023

github-actions bot commented Sep 12, 2023

🤖 Coverage Update

hugsy left a comment

Choose a reason for hiding this comment

hugsy commented Sep 13, 2023

github-actions bot commented Sep 14, 2023

🤖 Coverage Update

josx commented Sep 14, 2023

josx commented Sep 14, 2023

hugsy left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 14, 2023

🤖 Coverage Update

hugsy commented Nov 1, 2023

josx commented Nov 2, 2023

hugsy left a comment

Choose a reason for hiding this comment

hugsy commented Nov 29, 2023

josx commented Sep 2, 2023 •

edited by hugsy

Loading

hugsy commented Sep 12, 2023 •

edited

Loading

josx commented Sep 12, 2023 •

edited

Loading

hugsy commented Sep 12, 2023 •

edited

Loading