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

Merge #929 with some changes to get things compile #930

Merged
merged 1 commit into from
May 25, 2017
Merged

Merge #929 with some changes to get things compile #930

merged 1 commit into from
May 25, 2017

Conversation

vit9696
Copy link
Contributor

@vit9696 vit9696 commented May 24, 2017

This implements the changes made in #929 along with two small header fixes to get things compile for OS X kernel.

While these two changes are exactly necessary, there are (at least) two more changes for next branch to be used in the kernel:

  1. qsort — there is no public KPI for it
  2. Vulnerability fix Fix oob in X86_insn_reg_intel #725 (comment), this remains present in next branch.

Could you fix both of these on your own? They clearly are not part of this PR.
There are multiple qsort implementations with permissive licenses: example 1, example 2, example 3.
And the vulnerability issue is properly explained in the comment.

Thanks

@@ -62,6 +62,11 @@ typedef unsigned long long uint64_t;
#define UINT64_MAX 0xffffffffffffffffui64
#endif // defined(_MSC_VER) && (_MSC_VER <= 1700 || defined(_KERNEL_MODE))

#ifdef CAPSTONE_HAS_OSXKERNEL
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you actually mean "ifndef" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? It should be like it is. Kernel.framework provides stdint.h but does not provide inttypes.h.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah i misread it with including stdlib.h

@vit9696
Copy link
Contributor Author

vit9696 commented May 24, 2017

Also, while merging the changes I discovered the following line: https://github.com/aquynh/capstone/blob/next/arch/X86/X86IntelInstPrinter.c#L732
It sounded worrying, so I started to wonder why is that even here. Soon I discovered pretty much the only place that could make the use of it: https://github.com/aquynh/capstone/blob/next/arch/X86/X86Disassembler.c#L958

Both of these places look like ugly code duplication to my eyes, ud0 string mnemonic exists in a different place, and there is no reason for a strncpy with a hardcoded mnemonic to happen here, this should be done later by instruction printer.

And I also think that the extra if in X86_Intel_printInst is nothing but obscure. Would you like me to remove those in this pull request?

@aquynh
Copy link
Collaborator

aquynh commented May 24, 2017

this code is indeed ugly: i wanted to add UD0 instruction, without syncing with LLVM yet. this is a stop gap solution, and will be removed in the next major update. leave it there for now, unless you find a bug there.

@aquynh
Copy link
Collaborator

aquynh commented May 24, 2017

can you submit a pull req for #725, and another pull req to add qsort for OSX kernel?

@vit9696
Copy link
Contributor Author

vit9696 commented May 24, 2017

Well, as for qsort, I created a branch for it:
https://github.com/vit9696/capstone/commit/742edec9beeb48804b5b1c2e10f692be1e92e143
You could merge it as is, once this PR is merged.

As for #725, the proposed solution is pretty much valid, I don't see a reason to create another PR by myself.

this code is indeed ugly: i wanted to add UD0 instruction, without syncing with LLVM yet. this is a stop gap solution, and will be removed in the next major update. leave it there for now, unless you find a bug there.

Well, ok, better to remove it completely I guess. As for bugs… I could imagine some appearing in the future, since the assembly bypasses walking through filling cs_detail stuff. But as for now there should be none. Please remove them all, once you get a minute.

@aquynh
Copy link
Collaborator

aquynh commented May 24, 2017 via email

@aquynh aquynh merged commit a8c7e4a into capstone-engine:next May 25, 2017
@aquynh
Copy link
Collaborator

aquynh commented May 25, 2017

merged, thanks!

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.

None yet

2 participants