Skip to content

Add a bfdname for msp430 to make asm and disasm work #1606

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

Merged
merged 5 commits into from
Jul 7, 2020

Conversation

meithecatte
Copy link
Contributor

Not sure whether you consider this a bugfix, or new functionality...

Currently, out of the architectures that pwntools supports, msp430 and s390 error out with a missing bfd name. This PR resolves it for the former, but I couldn't determine what bfdname should be used for the latter - I have found two options: elf32-s390 and elf64-s390.

This is confusing, as all the other architectures use a separate name for their 64-bit variant. It seems that the 64-bit s390 is called the s390x, does this mean that pwntools only supports the 32-bit variant?

@heapcrash
Copy link
Collaborator

heapcrash commented Jun 24, 2020

Thanks for the contribution! Looks like good stuff we missed for a long time.

Can you add an entry to the CHANGELOG.md for this?

Fixing this on dev is probably fine, if nobody ever complained before it's likely that nobody tried it (and thus not worth a backport / release cycle). As long as you're happy with it on dev, I'm happy. If you want us to cut a release, we can.

@meithecatte meithecatte force-pushed the bfdname-msp430 branch 2 times, most recently from d09df20 to 1a3cf71 Compare June 24, 2020 10:19
@meithecatte
Copy link
Contributor Author

I added an entry to the changelog. Do you have any idea about the s390?

@Arusekk
Copy link
Member

Arusekk commented Jun 24, 2020

It could probably be solved by querying context.bits, so something like

"elf%d-s390" % context.bits

would do the job. I personally don't know s390 and never encountered it, but if it supports two widths, this may be the best option.

@heapcrash
Copy link
Collaborator

Looks good! Can you add a doctest so that we can exercise this code during our tests and make sure things don't break? Just an asm(..., arch='msp430) should be all that's needed in pwnlib.asm.asm.

@meithecatte meithecatte force-pushed the bfdname-msp430 branch 2 times, most recently from 7a1bf0b to 71aaa53 Compare June 28, 2020 17:56
@meithecatte
Copy link
Contributor Author

Ok! Just add it to the docs like this?

BTW, I added the bfdname for s390 too. With this PR, asm and disasm will work for all supported architectures!

@heapcrash
Copy link
Collaborator

It looks like there are a few conflicts to resolve before merging this. Probably changelog stuff. Resolve and I can merge it!

@meithecatte
Copy link
Contributor Author

CI failure looks unrelated to the change - it failed to install Android AVD.

@meithecatte
Copy link
Contributor Author

Is there anything else that still needs to be done here?

@heapcrash
Copy link
Collaborator

Would you mind updating the CHANGELOG.md to the new version this is being added for? We did a new release today.

@heapcrash
Copy link
Collaborator

Actually looks like this will need to be rebased or amended to use CI that's not broken

Arusekk added a commit to Arusekk/pwntools that referenced this pull request Jul 4, 2020
@meithecatte
Copy link
Contributor Author

Rebased on top of dev, CI now passes. Changelog updated too.

@meithecatte
Copy link
Contributor Author

Was the merge supposed to go this way?

@Arusekk Arusekk merged commit 2c33601 into Gallopsled:dev Jul 7, 2020
heapcrash pushed a commit that referenced this pull request Jul 10, 2020
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.

3 participants