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

Guard clause to handle 32 bits unsupported in setup.py #2229

Closed
wants to merge 3 commits into from

Conversation

fercevik729
Copy link
Contributor

Added a guard clause to setup.py in order to prevent incorrectly trying to install pwntools with a 32-bit Python installation. Resolves Gallopsled/pwntools-tutorial#15.

@peace-maker peace-maker changed the base branch from stable to dev July 14, 2023 03:41
Copy link
Member

@peace-maker peace-maker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Is this actually still true, now that Python 2 is dead for longer? The initial discussion in #518 suggests, that Python 3 32-bit would be ok. Did you test to run pwntools in a 32-bit Python?

I could see more problems with our dependencies not providing 32-bit wheels anymore and requiring more work to compile, but I didn't test it. Having this fail earlier until someone steps in and makes it work is good.

setup.py Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@fercevik729
Copy link
Contributor Author

I thought it was worth adding since the issue was still open in pwntools-tutorial and python 32 bit installations are still available for Windows. However, this issue in particular had been stale for quite some time (~3 years).

@Arusekk
Copy link
Member

Arusekk commented Jul 14, 2023

I consider the issue with 32 bits only applicable to Python 2.x. (Except for the tests, which could be made more portable maybe.) Also, I would prefer a warning (maybe with an interactive question asked) than a big error impossible to bypass.

setup.py Outdated Show resolved Hide resolved
@fercevik729
Copy link
Contributor Author

What kind of logic flow did you have in mind? Based off the issue I thought it was worth stopping early since it is kind of an irrecoverable issue.

Copy link
Contributor Author

@fercevik729 fercevik729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied requested formatting changes.

@fercevik729 fercevik729 reopened this Jul 15, 2023
@Arusekk
Copy link
Member

Arusekk commented Jul 15, 2023

I mean that pwntools is useful on 32-bit arches as well, but some features of it may not work under python 2. Here is all I know:

  • The only officially supported host platform for now is latest Ubuntu LTS on amd64 (read: this is a reference platform).
  • Some of the doctests currently only work on Ubuntu LTS on amd64, because they must assume something.
  • I personally tested pwntools on both arm64 (py2, py3) and i386 (py3) to work in every case I could possibly imagine.
  • Historically, pwntools is known to have had broken stuff on 32-bit builds of Python 2 (one thing still broken is MIPS shellcraft). This is caused by Python 2 integers being limited to platform-dependent value ranges, and us using hex(n) or repr(n) instead of "%#x" % n or "{:#x}".format(n) in some places. Python 3 does not have such a problem, because it simply treats everything similar to longs from py2 (bignums) and never adds any L suffix.

I think I will fix all of 32-bit problems shortly, and then we can get rid of the warning, which is worded too strongly in the first place.

And I really do not like some commercial software going to great lengths trying to make 'unsupported' mean 'impossible', so I do not want pwntools to be like that. This is just being mean to all well-aware users. Yes, yes, I know advanced users can just patch the check away, but still.

@peace-maker
Copy link
Member

We could limit the check to python 2 only. But I don't 100% understand what's broken exactly.

The other message doesn't prevent you from using pwntools on 32bit and only warns you, that there might be problems.

We could only log a warning during pip install, but I don't know if it's a good idea to stall on an input() call or similar, since it could break scripted installs.

This isn't your fault @fercevik729. You just brought up the issue again. Documenting possible incompatibility with 32bit Python 2 in the install instructions could fix the original issue in the pwntools-tutorial repo as well, without touching the setup.py?

@fercevik729
Copy link
Contributor Author

I see. In that case should I modify the installation instructions in the README and revert my changes?

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.

Please make it explicit that 32bits are not supported
3 participants