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.

Sign up for GitHub

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

Jump to bottom

Fix endianness for win pagedump and winkd #3204

Merged
merged 26 commits into from
Dec 9, 2022
Merged

Fix endianness for win pagedump and winkd #3204

merged 26 commits into from
Dec 9, 2022

Conversation

wargio
Copy link
Member

@wargio wargio commented Nov 26, 2022 •
edited by XVilka
Loading

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

This PR fixes the endianness issues on Windows pagedmp (DMP) parsing and loading into the debugger and some endianness issues linked to WinKD.

Also Fix arm32 windows profile (ARM_MAX_WATCHPOINTS == 1 not 2)

Test plan

db/formats/dmp/dmp tests must pass on s390x travis

Closes #297

@wargio wargio force-pushed the fix-endianness-windows-pagedump branch 3 times, most recently from 27930e7 to e68285b Compare December 4, 2022 21:27
@wargio wargio force-pushed the fix-endianness-windows-pagedump branch from 479df09 to 4bb8861 Compare December 5, 2022 15:15
@wargio wargio marked this pull request as ready for review December 5, 2022 16:26
@wargio wargio changed the title Fix endianness for win pagedump Fix endianness for win pagedump and winkd Dec 5, 2022
@wargio
Copy link
Member Author

wargio commented Dec 5, 2022

Test db/formats/dmp/dmp is now passing :)

@wargio
Copy link
Member Author

wargio commented Dec 5, 2022

@GustavoLCR can you check that i didn't break the windows debugger ?

XVilka
XVilka approved these changes Dec 5, 2022
View reviewed changes
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.

Amazing job! I didn't realize so many changes are necessary for DMP to work on BE systems. From the code point of view everything looks fine aside that code duplication, but this one isn't critical at all. We should wait for @GustavoLCR feedback or test debugger on Windows ourselves.
@ITAYC0HEN could you please help us with that?

librz/debug/p/native/bt/windows-x64.c
return uc;
}

static Win64UnwindInfo *windows_x64_unwind_info_new(RzDebug *dbg, ut64 at) {
Copy link
Member

@XVilka XVilka Dec 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if it's possible to get rid of this code duplication

Copy link
Member Author

Choose a reason for hiding this comment

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

it's not really code duplication since the native debugger needs the UnwindCode data, meanwhile the other one does not. but i see your point

@wargio
Fix unwind bit endianness
828c75f
@wargio
Avoid parsing header manually when is already parsed.
c8f64e7
@wargio
Testing dmp output (remove me)
2d93386
@wargio
Fix bitmap code.
4a0c632
@wargio
Use proper comparator
49f18eb
@wargio
Fix endianness when reg write on winkd
86408c0
@wargio
Fix more endianness in winkd
0fe8c6a
@wargio
Ensure the unwind info is endian compatible.
2cc4a72
@wargio
Ensure extracted bits are endian compatible.
9c69577
@wargio
Cleanup
9aef617
@wargio
Use indexed registers instead of pointer access
e4df0d2
@wargio
Fix arm32 windows profile (ARM_MAX_WATCHPOINTS == 1 not 2)
b2248eb
@wargio
Use sync_registers callback instead of reg_read to keep endianness
47fcdce
@wargio
Fix winkd_get_sp endianness
868ec74
@wargio
Rewrite how the unwind info/code structure is parsed
6d71042
@wargio
Fix endianness of winkd_va_to_pa
a973b40
@wargio
Fix bit endianness on seh x64
afb8435
@wargio
Remove travis allow_failures section to enforece s390x
702bdef
@wargio wargio force-pushed the fix-endianness-windows-pagedump branch from 586e497 to 702bdef Compare December 6, 2022 12:53
@XVilka XVilka added this to the 0.5.0 milestone Dec 6, 2022
thestr4ng3r
thestr4ng3r requested changes Dec 6, 2022
View reviewed changes
Copy link
Member

@thestr4ng3r thestr4ng3r left a comment

Choose a reason for hiding this comment

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

I don't know anything about the formats themselves, but overall lgtm.

librz/bin/format/dmp/dmp64.c Outdated Show resolved Hide resolved
librz/reg/arena.c Show resolved Hide resolved
ret2libc
ret2libc reviewed Dec 9, 2022
View reviewed changes
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.

Not a lot to comment, sorry, it's code I'm not familiar with at all.

librz/util/bitmap.c
@@ -34,9 +42,11 @@ RZ_API RZ_OWN RzBitmap *rz_bitmap_new(size_t len) {

RZ_API void rz_bitmap_set_bytes(RZ_NONNULL RzBitmap *b, RZ_NONNULL const ut8 *buf, size_t len) {
rz_return_if_fail(b && buf && len >= 0);
if (b->length < len) {
len = b->length;
size_t blen = b->length << BITWORD_BITS_SHIFT;
Copy link
Member

Choose a reason for hiding this comment

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

isn't it better to use BITMAP_WORD_COUNT on len and compare that?

Copy link
Member Author

Choose a reason for hiding this comment

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

is the same.

@XVilka
Copy link
Member

XVilka commented Dec 9, 2022

I tested this on Windows 10 x64 with all updates installed. Basic features of the debugger seem to work as well as on dev:

  • ds and dc to step and continue
  • dr to view and set registers
  • di to show the info
  • dp to show the processes, threads
  • Setting/unsetting breakpoints and listing them with db
  • Listing memory maps with dm
  • Identifying Windows with dW commands

The only problem I met is that prompt offset doesn't update sometimes after ds or dc, while RIP register has a new value. I think, it's supposed to be updated. It happens on "dev" branch too though.

I think it's safe to merge.

@wargio wargio merged commit 55e6909 into dev Dec 9, 2022
@wargio wargio deleted the fix-endianness-windows-pagedump branch December 9, 2022 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ret2libc ret2libc ret2libc left review comments

@XVilka XVilka XVilka approved these changes

@thestr4ng3r thestr4ng3r thestr4ng3r approved these changes

@yossizap yossizap Awaiting requested review from yossizap yossizap is a code owner

@kazarmy kazarmy Awaiting requested review from kazarmy kazarmy is a code owner

@GustavoLCR GustavoLCR Awaiting requested review from GustavoLCR

@ITAYC0HEN ITAYC0HEN Awaiting requested review from ITAYC0HEN

Assignees

@GustavoLCR GustavoLCR

Labels
API infrastructure PE rz-test RzBin RzDebug RzReg RzUtil WinDbg Windows
Projects
None yet
Milestone
0.5.0
Development

Successfully merging this pull request may close these issues.

Fix broken tests on SystemZ platform
5 participants
@wargio @XVilka @ret2libc @thestr4ng3r @GustavoLCR