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

discrepancy between sark.Line(ea=foo).is_tail and is_tail(idc.GetFlags(ea)), also for is_code #85

Open
mewmew opened this issue Nov 30, 2019 · 9 comments
Assignees
Labels

Comments

@mewmew
Copy link

mewmew commented Nov 30, 2019

Hi,

First off. Sark is what I've always wished the IDA API looked like. Thank you so much for reigniting the joy and excitement of binary analysis!! The graph API looks extremely powerful, and the concepts of Sark feel very well thought out. Hats off!

Now, for the issue I happened to stumble upon today, when exploring what the Sark world had to offer.

It seems there is currently a discrepancy between how IDA and Sark handles is_code and is_tail (I did not check is_data, but it would make sense to check that too as part of this issue).

The proof of concept is as follows:

Python>is_code(idc.GetFlags(0x4785A4))
True
Python>is_code(idc.GetFlags(0x4785A5))
False
Python>is_tail(idc.GetFlags(0x4785A4))
False
Python>is_tail(idc.GetFlags(0x4785A5))
True
Python>sark.Line(ea=0x4785A4).is_code
True
Python>sark.Line(ea=0x4785A5).is_code
True
Python>sark.Line(ea=0x4785A4).is_tail
False
Python>sark.Line(ea=0x4785A5).is_tail
False
Python>print(sark.Line(ea=0x4785A4))
[004785A4]    test    esi, esi
Python>print(sark.Line(ea=0x4785A5))
[004785A4]    test    esi, esi

I can't share this database, however, I hope this should be easy to reproduce. If not, just ping me and we'll do some troubleshooting together.

Oh, and for the record, this is at IDA 7.3, using the 6.x branch of Sark (downloaded today, so should be the latest).

Cheers,
Robin

@tmr232 tmr232 added the bug label Jan 5, 2020
@tmr232 tmr232 self-assigned this Jan 5, 2020
@tmr232
Copy link
Owner

tmr232 commented Jan 5, 2020

Thanks!
This definitely looks like a bug, I'll have to look into it.
Sorry for the delayed response...

@mewmew
Copy link
Author

mewmew commented Jan 5, 2020

Hi Tamir,

No worries, it's important to take time off also from hobby projects.

Hope you've had a good start of the new year :)

Happy coding!
/robin

@tmr232
Copy link
Owner

tmr232 commented Jan 6, 2020

Just looked into it again - I don't think I consider it a bug.
Sark's line normalizes the address, as it represents a line, not an address. It cannot be used to point to the middle of an instruction.
This means that any attempt to use it on such an address will fail.

Why is this needed?

@mewmew
Copy link
Author

mewmew commented Jan 6, 2020

Just looked into it again - I don't think I consider it a bug.

Thanks for taking a look at this. I appreciate that Sark is trying to do the right thing by normalizing addresses. I must have missed this in the documentation.

Sark's line normalizes the address, as it represents a line, not an address. It cannot be used to point to the middle of an instruction.

Given that Sark normalizes the address (to represent a valid line), it would make sense to remove the is_tail method from the Sark API, considering that is_tail is only ever used to report when an address is in the middle of an instruction, something that would be impossible with normalized addresses.

So to avoid confusion, I'd suggest removing the is_tail function from the Sark API.

At least, this would be my understanding, but I might have misunderstood the function of is_tail ^^

@mewmew
Copy link
Author

mewmew commented Jan 6, 2020

Why is this needed?

I was using is_tail to determine if an address was the start of an instruction, or if it was just part of a multi-byte instruction.

@tmr232
Copy link
Owner

tmr232 commented Jan 7, 2020

It seems that you are correct.
As for the check you want to perform - while it will be costly using Sark, you can always compare the address to the address of the Line you build from it. But direct access will probably be faster.

@mewmew
Copy link
Author

mewmew commented Jan 7, 2020

As for the check you want to perform - while it will be costly using Sark, you can always compare the address to the address of the Line you build from it.

Thanks, that work-around would suffice.

It seems that you are correct.

Ok, so would you agree that removing is_tail from Sark is the way forward?

@tmr232
Copy link
Owner

tmr232 commented Jan 7, 2020

I am willing to add a deprecation warning, but since some code may already be using it, I'd rather not just break people's code.

@tmr232
Copy link
Owner

tmr232 commented Jan 7, 2020

But yeah, I agree that it should eventually be removed as it is misleading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants