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

Added arch and bits specific flag to asm call from elf wrapper #2137

Merged
merged 1 commit into from Dec 28, 2022

Conversation

en0
Copy link
Contributor

@en0 en0 commented Nov 13, 2022

In the ELF class, the asm(...) method wraps the pwnlib.asm:asm which defaults to 32 bits. I added bits, arch, and endianness to that call so that calls to the ELF.asm(...) method will use the flags that apply to the loaded ELF file.

@Arusekk
Copy link
Member

Arusekk commented Nov 14, 2022

This looks good, although it might be useful to override the settings.

Just saying, not a requirement, since it was not there before.

@en0
Copy link
Contributor Author

en0 commented Nov 22, 2022

This looks good, although it might be useful to override the settings.

Just saying, not a requirement, since it was not there before.

Are you suggesting we change the signature of ELF.asm(...) from def asm(self, address, assembly): to something like def asm(self, address, assembly, **kwargs):?

@peace-maker
Copy link
Member

Probably using a local context.

@en0
Copy link
Contributor Author

en0 commented Nov 23, 2022

I am not sure I understand why or how this would apply. The method I updated is an instance method on the ELF object. The assembled instructions will be written into the ELF data which is going to match self.arch. I am willing to make the suggested changes. But, I am not sure i understand what your ask is. Can you help me understand?

@Arusekk
Copy link
Member

Arusekk commented Nov 24, 2022 via email

@en0
Copy link
Contributor Author

en0 commented Nov 24, 2022

Okay, I understand. Thank you for explaining. I think that is a good idea. However, can we add it as a subsequent merge? I think this MR holds value now and i don't want to hold it up while i figure out how to include the local context part. I will start a second MR with that if that works for you.

@Arusekk Arusekk merged commit e540760 into Gallopsled:dev Dec 28, 2022
gogo2464 pushed a commit to gogo2464/pwntools that referenced this pull request Sep 10, 2023
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

3 participants