Skip to content

Reworked encoding of branching instructions (Fixes #268, Fixes #266)#284

Merged
athre0z merged 2 commits intozyantific:masterfrom
mappzor:branching_rework
Dec 14, 2021
Merged

Reworked encoding of branching instructions (Fixes #268, Fixes #266)#284
athre0z merged 2 commits intozyantific:masterfrom
mappzor:branching_rework

Conversation

@mappzor
Copy link
Copy Markdown
Contributor

@mappzor mappzor commented Dec 9, 2021 •
edited
Loading

Fixes #268
Fixes #266

@flobernd flobernd requested a review from athre0z December 9, 2021 11:58
@flobernd
Copy link
Copy Markdown
Member

flobernd commented Dec 9, 2021

LGTM!

New API looks nice and documentation is well done.

Just roughly swiped over the code details and noticed the special case handling for certain instructions by checking their mnemonics. While I don't really like that approach, I don't have a better solution in mind for that right now (besides adding new flags ofc. 😋). It might cause complications if - at some point - we can finally allow the user to choose what kind of extensions should be build into the binary (using CMake options). But until then, it's okay for me to leave the hardcoded checks like they are right now.

@mappzor
Copy link
Copy Markdown
Contributor Author

mappzor commented Dec 9, 2021

Yeah, I didn't like hardcoding XBEGIN AGAIN but we don't really have much choice. This is a very unique instruction at the moment and we already have a special case for it in the formatter.

With bugged MVEXinstructions (actually they are more like one instruction with slightly different variants, they even have same opcode just different REX.W and MVEX.EH) I decided to hardcode because it's dead hardware with only a single offender. Every other solution seemed to be a massive overkill.

@flobernd flobernd self-requested a review December 13, 2021 13:37
@flobernd
Copy link
Copy Markdown
Member

flobernd commented Dec 13, 2021

LGTM from my side, maybe @athre0z want's to have a look as well.

athre0z
athre0z approved these changes Dec 14, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@athre0z athre0z left a comment

Choose a reason for hiding this comment

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

Awesome changes, thanks a lot for that! Definitely makes for a much better UX. I'll merge this later after I pushed the corresponding new fuzzing corpus.

examples/EncodeFromScratch.c
// Assemble `ret`.
memset(&req, 0, sizeof(req));
req.mnemonic = ZYDIS_MNEMONIC_RET;
req.branch_type = ZYDIS_ENCODABLE_BRANCH_TYPE_NEAR64;
Copy link
Copy Markdown
Member

@athre0z athre0z Dec 14, 2021

Choose a reason for hiding this comment

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

This is my favourite line in this PR! :)

@athre0z athre0z merged commit a8e43f8 into zyantific:master Dec 14, 2021
@athre0z
Copy link
Copy Markdown
Member

athre0z commented Dec 14, 2021 •
edited
Loading

Change of mind: did it immediately so I don't forget it later (also uploaded new corpus). :)

@mappzor mappzor deleted the branching_rework branch December 14, 2021 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@athre0z athre0z athre0z approved these changes

@flobernd flobernd flobernd approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Consider reworking branch types in the encoder Cant encode using ZYDIS_ENCODABLE_BRANCH_TYPE_NEAR32

3 participants

@mappzor @flobernd @athre0z