search

Gallopsled / pwntools

CTF framework and exploit development library
http://pwntools.com
Other
12.12k stars 1.71k forks source link

Merge `drop` and `keepends` kwargs #2258

Open MrQubo opened 1 year ago

MrQubo commented 1 year ago

recvuntil() uses drop kwarg, while recvline() and recvlines() use keepends. keepends is a logical negation of drop, while both serve the same purpose. I find this unnecessarily confusing.

I would like to propose using the same kwarg for the purpose of those functions.

It's a matter of discussion whether drop or keepends or maybe a new different keyword is better.

The new keyword can be added while preserving the old behaviour.

peace-maker commented 1 year ago

I agree it's better to have them consistent. We could just support both drop and keepends in both functions too :D I'm in favor of drop since keepends doesn't make much sense for recvuntil.

sanjitkumar2016 commented 6 months ago

In my opinion, this one is a bit tricky because changing/removing kwargs ruins backwards compatibility for everyone using the keepends kwarg. The keepends kwarg is also used much more than the drop kwarg, so I'm not sure if this is something that a lot of users would expect. Supporting a both could also be weird/conflicting if a user sets both kwargs to the same values (even though they're supposed to be opposites). If we do want to remove the keepends kwarg, we could start off by issuing a Deprecation warning in the code and then implementing the changes in a future release, and raising an exception if keepends is used. Please let me know your thoughts!

MrQubo commented 5 months ago

In my opinion, this one is a bit tricky because changing/removing kwargs ruins backwards compatibility for everyone using the keepends kwarg. The keepends kwarg is also used much more than the drop kwarg, so I'm not sure if this is something that a lot of users would expect. Supporting a both could also be weird/conflicting if a user sets both kwargs to the same values (even though they're supposed to be opposites). If we do want to remove the keepends kwarg, we could start off by issuing a Deprecation warning in the code and then implementing the changes in a future release, and raising an exception if keepends is used. Please let me know your thoughts!

I agree, I had the same thoughts. There's no need to remove keepends, we can depreciate it. Also, setting both drop and keepends should result in an error in my opinion.

We could do some poll whether drop or keepends is better. But I think that drop wins, simply because it's shorter to write. Technically, there's no problem in supporting both, but I think that could potentially become source of confusion, so it's better to stick to only one.

MrQubo commented 1 month ago

recvline() and recvlines() have different defaults for 'keepends'. It would also be good to use the same default in both for consistency, though I'm not sure how to go about changing the default. That would be obviously not backward-compatible but printing depreciation warning for everyone using default value would be quite annoying for the users.

Arusekk commented 1 month ago

Note that .readlines might need to be compatible with file-like .readlines. Not sure if it is the case, but see what happened after refactoring term.readline for instance.

MrQubo commented 1 month ago

readlines() is incompatible atm, because it drops newline. Also, it makes 0 sense for me that recvline() doesn't drop the newline, but e.g. recvline_startswith() does. From my experience, most of the time I want newline dropped when writing exploits.