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

[Bug] Changes made in commit #81ee52d mean that register order is no longer maintained in context #986

Closed
1 of 9 tasks
BarryStokes opened this issue Aug 15, 2023 · 3 comments · Fixed by #993
Closed
1 of 9 tasks
Assignees
Milestone

Comments

@BarryStokes
Copy link

GEF+GDB version

master branch of GEF

Operating System

Ubuntu, Debian

Describe the issue you encountered

Previously list comprehensions were used for filtering the registers requested in DetailRegistersCommand(GenericCommand) (around line 6733). This got changed in commit 81ee52d to use set instead.

Because set is unordered, changing the way the register filtering is done results in the registers printing in an unexpected order.

Previously you'd get $rax, $rbx, $rcx in order:

$rax   : 0x0               
$rbx   : 0x0               
$rcx   : 0x0               
$rdx   : 0x0               
$rsp   : 0x00007fffffffde70  →  0x0000000000000001
$rbp   : 0x0               
$rsi   : 0x0               
$rdi   : 0x0               
$rip   : 0x00007ffff7fe32b0  →  <_start+0> mov rdi, rsp
$r8    : 0x0               
$r9    : 0x0               
$r10   : 0x0               
$r11   : 0x0               
$r12   : 0x0               
$r13   : 0x0               
$r14   : 0x0               
$r15   : 0x0               
$eflags: [zero carry parity adjust sign trap INTERRUPT direction overflow resume virtualx86 identification]
$cs: 0x0033 $ss: 0x002b $ds: 0x0000 $es: 0x0000 $fs: 0x0000 $gs: 0x0000 

Now you get:

$rdx   : 0x0               
$rsi   : 0x0               
$rdi   : 0x0               
$rbp   : 0x0               
$eflags: [zero carry parity adjust sign trap INTERRUPT direction overflow resume virtualx86 identification]
$r8    : 0x0               
$rbx   : 0x0               
$r12   : 0x0               
$rcx   : 0x0               
$r9    : 0x0               
$rsp   : 0x00007fffffffe590  →  0x0000000000000001
$rip   : 0x00007ffff7fe32b0  →  <_start+0> mov rdi, rsp
$r11   : 0x0               
$r13   : 0x0               
$r15   : 0x0               
$r10   : 0x0               
$r14   : 0x0               
$rax   : 0x0               
$es: 0x00 $ds: 0x00 $fs: 0x00 $cs: 0x33 $ss: 0x2b $gs: 0x00 

It's possible that the new code will always return the registers in the same order, but I don't believe that there's any guarantee of this due to set being specifically unordered and the order they are returned in does not feel logical to me whilst looking for the contents of a specific register (i.e. I expect $rax to be the first result, I don't expect to have to hunt for it). As such, I believe that this should be reverted so that the behaviour returns results in an expected and consistent order.

Do you read the docs and look at previously closed issues/PRs for similar cases?

Yes

Architecture impacted

  • X86
  • X64
  • ARM
  • ARM64
  • MIPS
  • MIPS64
  • PPC
  • PPC64
  • RISCV

Describe your issue. Without a proper reproduction step-by-step, your issue will be ignored.

Run GDB, hit a breakpoint or use starti.

Older version show registers in expected order, new version doesn't.

Minimalist test case

N/A - Any binary should do it

Additional context?

N/A

@hugsy hugsy added confirmed and removed triage labels Aug 15, 2023
@hugsy hugsy added this to the 2023.08 milestone Aug 15, 2023
@hugsy
Copy link
Owner

hugsy commented Aug 15, 2023

Great catch and root cause investigation!
We were looking into this bug we found recently too.

The fix will come soon!

@hugsy hugsy added the urgent label Aug 15, 2023
@therealdreg therealdreg self-assigned this Aug 15, 2023
@therealdreg
Copy link
Collaborator

fixed and will be merge today (I hope)

@hugsy
Copy link
Owner

hugsy commented Aug 15, 2023

Reopening, the issue will be closed automatically when the PR is merged

@hugsy hugsy reopened this Aug 15, 2023
hugsy pushed a commit that referenced this issue Aug 16, 2023
Fixes a bug I added in #972 which lost the order of registers
Fixes #986
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment