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

packing: Do use extra arguments in p* and u* #2526

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Jan 18, 2025

Make use of extra arguments (without specifying keyword) like endianness in packing utils.
Before, they were ignored if one doesn't use keyword syntaxs for them.

Target Branch

A small bugfix but it doesn't seem to worth backporting to stable,
How about backporting to beta?

Closes #2525

@peace-maker
Copy link
Member

Ah, good catch, thank you. When I spelled the functions out into their own functions instead of being generated in #2189, I defined the endian and sign arguments explicitly with IDE support in mind and didn't notice this change in expected behavior. The arguments weren't defined before and were thus picked up solely by the @LocalNoArchContext decorator when passing them as kwarg.

It's annoying that we have to pass those arguments through like this and I like the verbatism of having to use keyword args instead of allowing positional args, since you'd have to remember the order this way. To avoid confusing p64 calls with magic values like p64(x, 'big', True) removing the extra "unused" arguments from all the packing and unpacking functions would be the way to go imo.

This seems to be personal preference but pwntools scripts are designed to make exploits readable and using keyword arguments makes the function usage more explicit. What do you think?

@Arusekk
Copy link
Member

Arusekk commented Jan 19, 2025

BTW with py2 support deprecated we can finally use def f(a1, *, a2, a3): syntax for keyword-only arguments.

@tesuji
Copy link
Contributor Author

tesuji commented Jan 19, 2025

To avoid confusing p64 calls with magic values like p64(x, 'big', True) removing the extra "unused" arguments from all the packing and unpacking functions would be the way to go imo.

I've never written exploit code using IDE, only by raw editors like vim or sublime text. So the extra typing
endianness slows me down a little bit. Also, 'big' or 'little' hints me about endianness anyway so I don't feel it
much more clear to explicitly use keyword arguments. However, I agree that 'sign` should be explicitly spelled out
as I seldomly use them.

It's your decision, I'm happy with whatever.

Edit: I went ahead and removed sign from normal arguments.

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.

packing.p*: extra arguments (without specifying keyword) have no effects
3 participants