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

Make PyCrypto work with PyPy (bug #1131452) #59

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
@Legrandin
Contributor

Legrandin commented Aug 25, 2013

With this set of patches, PyCrypto works under PyPy.

NOTE: It would be cleaner and faster to have this done via cffi or via a Cython extension. Still, this is a start and at least it allows PyCrypto-based application to run under pypy.

The main change concerns the 2 routines longObjToMPZ and mpzToLongObj in _fastmath.c.
They have been completely changed so that they don't access the internals of a PyLong anymore (which are not exposed by PyPy).
Even though the new routines are less efficient, I didn't notice any significant performance degradation with CPython; that probably means these routines are not hotspots anyway. I left one of the two routines only for Python < 2.7; the alternative was to backport PyLong_AsLongAndOverflow.

The interesting thing is that PyCrypto Public Key functions are slower under PyPy compared to CPython by a 30% factor.

This patch solves this bug https://bugs.launchpad.net/pycrypto/+bug/1131452 .

Note: In order to build the C extensions, the following patch needs to be applied to PyPy: http://web.archiveorange.com/archive/v/ly9iT75Cutf1uTbpBled . Most recent versions of PyPy include it already.

@tycho

This comment has been minimized.

Show comment
Hide comment
@tycho

tycho Oct 7, 2013

Aside from typo mentioned above, this also fails unit testing. 😞

tycho commented Oct 7, 2013

Aside from typo mentioned above, this also fails unit testing. 😞

@tycho

This comment has been minimized.

Show comment
Hide comment
@tycho

tycho Oct 7, 2013

I've also confirmed that the patches proposed introduce a regression with CPython 2.7.x.

Current HEAD @ f9a0fc7:

running test
running build
running build_py
running build_ext
running build_configure
..................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................SelfTest: You can ignore the RandomPool_DeprecationWarning that follows.
......................................................
----------------------------------------------------------------------
Ran 1176 tests in 12.673s

OK

HEAD + proposed patches for pull #59:

$ python setup.py test
running test
running build
running build_py
running build_ext
running build_configure
.......................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................F.FEEE.F.EEEE...........................................................................................................FFFFFFFF....FEEFFEFFFFEFEEFFEFFFFE.........FE.FFFFFFFFF.FFFFFFFFFF.....................FFFFFF.FFFFFF.FF..FFFFFFFFFF.FFFFF.............FFFFFF.....FF................SelfTest: You can ignore the RandomPool_DeprecationWarning that follows.
.......F.....

Note that the tests hang here indefinitely. Ouch.

tycho commented Oct 7, 2013

I've also confirmed that the patches proposed introduce a regression with CPython 2.7.x.

Current HEAD @ f9a0fc7:

running test
running build
running build_py
running build_ext
running build_configure
..................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................SelfTest: You can ignore the RandomPool_DeprecationWarning that follows.
......................................................
----------------------------------------------------------------------
Ran 1176 tests in 12.673s

OK

HEAD + proposed patches for pull #59:

$ python setup.py test
running test
running build
running build_py
running build_ext
running build_configure
.......................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................F.FEEE.F.EEEE...........................................................................................................FFFFFFFF....FEEFFEFFFFEFEEFFEFFFFE.........FE.FFFFFFFFF.FFFFFFFFFF.....................FFFFFF.FFFFFF.FF..FFFFFFFFFF.FFFFF.............FFFFFF.....FF................SelfTest: You can ignore the RandomPool_DeprecationWarning that follows.
.......F.....

Note that the tests hang here indefinitely. Ouch.

@Legrandin

This comment has been minimized.

Show comment
Hide comment
@Legrandin

Legrandin Oct 7, 2013

Contributor

Thanks for trying this out.

I tested this only on PyPy 32-bits (Linux x86) and not with PyPy 64 bits. I will try to setup a VM and have a look at it,

Contributor

Legrandin commented Oct 7, 2013

Thanks for trying this out.

I tested this only on PyPy 32-bits (Linux x86) and not with PyPy 64 bits. I will try to setup a VM and have a look at it,

@tycho

This comment has been minimized.

Show comment
Hide comment
@tycho

tycho Oct 7, 2013

Thanks @Legrandin. Looking forward to seeing this working, it's the one thing preventing me from using PyPy for some of my more major projects at the moment.

tycho commented Oct 7, 2013

Thanks @Legrandin. Looking forward to seeing this working, it's the one thing preventing me from using PyPy for some of my more major projects at the moment.

@Legrandin

This comment has been minimized.

Show comment
Hide comment
@Legrandin

Legrandin Oct 8, 2013

Contributor

Could you try this last change, @tycho ? I tested it on 64-bit Linux and PyPy 2.1.
The regression on CPython 2.7 goes away too.

Contributor

Legrandin commented Oct 8, 2013

Could you try this last change, @tycho ? I tested it on 64-bit Linux and PyPy 2.1.
The regression on CPython 2.7 goes away too.

@tycho

This comment has been minimized.

Show comment
Hide comment
@tycho

tycho Oct 8, 2013

Confirmed, no test failures! Looks great.

PyPy:

Ran 1176 tests in 15.183s

OK

CPython 2.7

Ran 1176 tests in 19.686s

OK

tycho commented Oct 8, 2013

Confirmed, no test failures! Looks great.

PyPy:

Ran 1176 tests in 15.183s

OK

CPython 2.7

Ran 1176 tests in 19.686s

OK
@msabramo

This comment has been minimized.

Show comment
Hide comment
@msabramo

msabramo Nov 11, 2013

Contributor

Confirmed here as well (using tox.ini that I added in #64). Nice work, @Legrandin!

vagrant@ubuntu:~/dev/git-repos/pycrypto$ git --no-pager log -n 1
commit 8e04540862d3742e57660b4e81556c6cb49f06f1
Merge: f9a0fc7 c42ed04
Author: Legrandin <e.bened@gmail.com>
Date:   Mon Oct 7 23:49:18 2013 -0700

    Merge c42ed0402a258dd9357bdb01dc18b23c10ab6226 into f9a0fc77e1c8847c1a17503e5a1b86a409b8cb2d

vagrant@ubuntu:~/dev/git-repos/pycrypto$ tox
...
  py25: commands succeeded
  py26: commands succeeded
  py27: commands succeeded
  py32: commands succeeded
  py33: commands succeeded
  pypy: commands succeeded
  congratulations :)
Contributor

msabramo commented Nov 11, 2013

Confirmed here as well (using tox.ini that I added in #64). Nice work, @Legrandin!

vagrant@ubuntu:~/dev/git-repos/pycrypto$ git --no-pager log -n 1
commit 8e04540862d3742e57660b4e81556c6cb49f06f1
Merge: f9a0fc7 c42ed04
Author: Legrandin <e.bened@gmail.com>
Date:   Mon Oct 7 23:49:18 2013 -0700

    Merge c42ed0402a258dd9357bdb01dc18b23c10ab6226 into f9a0fc77e1c8847c1a17503e5a1b86a409b8cb2d

vagrant@ubuntu:~/dev/git-repos/pycrypto$ tox
...
  py25: commands succeeded
  py26: commands succeeded
  py27: commands succeeded
  py32: commands succeeded
  py33: commands succeeded
  pypy: commands succeeded
  congratulations :)

Legrandin added some commits Aug 24, 2013

Simplify _fastmath (remove import)
The randfunc parameter of getRandomInteger() can
be a callable object or None.

If it is None, C code will import the Random module
and create a new RNG object.

Apart from breaking pypy, the C code is ugly and unnecessary:
it is much easier to ensure that getRandomInteger()
is always called with a valid object from the Python side.
Stop poking inside PyLong's
This patch modifies the 2 routines to convert
libgmp integers into (and from) PyLongs, so
that internals or PyLongs are not accessed
directly anymore.

With this last patch, PyCrypto should run under PyPy.
Fix for 64-bit platforms and negative numbers
This patch fixes 2 problems:
* The code only worked on 32-bit platforms (not on 64-bit)
* The code did not handle conversion of negative numbers
  correctly.
@lopuhin

This comment has been minimized.

Show comment
Hide comment
@lopuhin

lopuhin Mar 17, 2014

FWIW, it fails to build on OS X under pypy 2.1 and pypy 2.2 (while pycrypto from pypi works fine), although it could be a problem with my setup.

pip install -e git+git://github.com/Legrandin/pycrypto.git@pypy#egg=pycrypto

...

building 'Crypto.Cipher._AESNI' extension

cc -arch x86_64 -fPIC -Wimplicit -g -O2 -Wall -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -DHAVE_CONFIG_H -Isrc/ -I/Users/kostia/apps/netdb_demo_pypy/include -c src/AESNI.c -o build/temp.macosx-10.9-x86_64-2.7/src/AESNI.o -maes

clang: warning: argument unused during compilation: '-g -O2 -Wall -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -DHAVE_CONFIG_H'

src/AESNI.c:83:16: error: index for __builtin_shufflevector must be a constant integer

    keygened = _mm_shuffle_epi32(keygened, shuf);

               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/clang/5.0/include/emmintrin.h:1276:12: note: expanded from macro '_mm_shuffle_epi32'

  (__m128i)__builtin_shufflevector((__v4si)__a, (__v4si) _mm_set1_epi32(0), \

           ^

src/AESNI.c:83:14: error: assigning to '__m128i' from incompatible type 'void'

    keygened = _mm_shuffle_epi32(keygened, shuf);

             ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

2 errors generated.

error: command 'cc' failed with exit status 1

lopuhin commented Mar 17, 2014

FWIW, it fails to build on OS X under pypy 2.1 and pypy 2.2 (while pycrypto from pypi works fine), although it could be a problem with my setup.

pip install -e git+git://github.com/Legrandin/pycrypto.git@pypy#egg=pycrypto

...

building 'Crypto.Cipher._AESNI' extension

cc -arch x86_64 -fPIC -Wimplicit -g -O2 -Wall -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -DHAVE_CONFIG_H -Isrc/ -I/Users/kostia/apps/netdb_demo_pypy/include -c src/AESNI.c -o build/temp.macosx-10.9-x86_64-2.7/src/AESNI.o -maes

clang: warning: argument unused during compilation: '-g -O2 -Wall -Wextra -Wno-missing-field-initializers -Wno-unused-parameter -DHAVE_CONFIG_H'

src/AESNI.c:83:16: error: index for __builtin_shufflevector must be a constant integer

    keygened = _mm_shuffle_epi32(keygened, shuf);

               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/clang/5.0/include/emmintrin.h:1276:12: note: expanded from macro '_mm_shuffle_epi32'

  (__m128i)__builtin_shufflevector((__v4si)__a, (__v4si) _mm_set1_epi32(0), \

           ^

src/AESNI.c:83:14: error: assigning to '__m128i' from incompatible type 'void'

    keygened = _mm_shuffle_epi32(keygened, shuf);

             ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

2 errors generated.

error: command 'cc' failed with exit status 1
@sebastinas

This comment has been minimized.

Show comment
Hide comment
@sebastinas

sebastinas Mar 17, 2014

Contributor

There is a fix for that in PR #63.

Contributor

sebastinas commented Mar 17, 2014

There is a fix for that in PR #63.

@Legrandin Legrandin closed this May 20, 2014

@tycho

This comment has been minimized.

Show comment
Hide comment
@tycho

tycho May 20, 2014

Why was this closed? AFAIK it wasn't merged?

tycho commented May 20, 2014

Why was this closed? AFAIK it wasn't merged?

@Legrandin

This comment has been minimized.

Show comment
Hide comment
@Legrandin

Legrandin May 20, 2014

Contributor

This PR will never be merged. As explained on the ML, PyPy will only be supported via cffi.

On the other hand, it is unfeasible to me to maintain a one year old PR. Since too much stuff has been piling up, I prefer to close it.

Contributor

Legrandin commented May 20, 2014

This PR will never be merged. As explained on the ML, PyPy will only be supported via cffi.

On the other hand, it is unfeasible to me to maintain a one year old PR. Since too much stuff has been piling up, I prefer to close it.

@OOPMan

This comment has been minimized.

Show comment
Hide comment
@OOPMan

OOPMan Jul 3, 2014

When will a working PyCrypto for PyPy arrive. The lack of one is a real pain for numerous reasons. Or should I just fork the the projects I need and get them to work with cryptography.io?

OOPMan commented Jul 3, 2014

When will a working PyCrypto for PyPy arrive. The lack of one is a real pain for numerous reasons. Or should I just fork the the projects I need and get them to work with cryptography.io?

@mdevaev

This comment has been minimized.

Show comment
Hide comment
@mdevaev

mdevaev Jul 24, 2014

@Legrandin it's a bullshit.
You do not take this PR, as a result, the library is not work with PyPy. It is needed for other libraries (twisted, paramiko, etc). What is offered to do to those who want to PyPy?

mdevaev commented Jul 24, 2014

@Legrandin it's a bullshit.
You do not take this PR, as a result, the library is not work with PyPy. It is needed for other libraries (twisted, paramiko, etc). What is offered to do to those who want to PyPy?

@OOPMan

This comment has been minimized.

Show comment
Hide comment
@OOPMan

OOPMan Jul 24, 2014

@mdevaev I believe there is a plan going forward to try and merge PyCrypto with cryptograhy.io.

OOPMan commented Jul 24, 2014

@mdevaev I believe there is a plan going forward to try and merge PyCrypto with cryptograhy.io.

@mdevaev

This comment has been minimized.

Show comment
Hide comment
@mdevaev

mdevaev Jul 24, 2014

@OOPMan What do I do until then? Spit at the ceiling and wait?
I do not understand what the problem is. PR tested, a year has passed and nothing has changed. PyCrypto still does not work with PyPy.

mdevaev commented Jul 24, 2014

@OOPMan What do I do until then? Spit at the ceiling and wait?
I do not understand what the problem is. PR tested, a year has passed and nothing has changed. PyCrypto still does not work with PyPy.

@OOPMan

This comment has been minimized.

Show comment
Hide comment
@OOPMan

OOPMan Jul 24, 2014

@mdevaev Well, I don't know about you but in my case I'm probably going to fork oauthlib and rejigger it to use cryptography.io rather than PyCrypto. I suggest you consider doing the same or something similar for projects you need that rely on PyCrypto.

OOPMan commented Jul 24, 2014

@mdevaev Well, I don't know about you but in my case I'm probably going to fork oauthlib and rejigger it to use cryptography.io rather than PyCrypto. I suggest you consider doing the same or something similar for projects you need that rely on PyCrypto.

@mdevaev

This comment has been minimized.

Show comment
Hide comment
@mdevaev

mdevaev Jul 24, 2014

@OOPMan The fact that I do not need PyCrypto. I need Paramiko. It uses PyCrypto :-)

mdevaev commented Jul 24, 2014

@OOPMan The fact that I do not need PyCrypto. I need Paramiko. It uses PyCrypto :-)

@OOPMan

This comment has been minimized.

Show comment
Hide comment
@OOPMan

OOPMan Jul 24, 2014

@mdevaev I feel you pain. It pisses me off every time I have to run Fabric using a custom CPython venv

OOPMan commented Jul 24, 2014

@mdevaev I feel you pain. It pisses me off every time I have to run Fabric using a custom CPython venv

@Legrandin

This comment has been minimized.

Show comment
Hide comment
@Legrandin

Legrandin Jul 24, 2014

Contributor

@OOPMan @mdevaev I was on the same boat (though I also needed a decent way to build on Windows). I ended up forking PyCrypto.

Contributor

Legrandin commented Jul 24, 2014

@OOPMan @mdevaev I was on the same boat (though I also needed a decent way to build on Windows). I ended up forking PyCrypto.

@mdevaev

This comment has been minimized.

Show comment
Hide comment
@mdevaev

mdevaev Jul 24, 2014

@Legrandin Maybe it makes sense to reopen the PR? Code still works. You do not necessarily support him after he goes in upstream.

mdevaev commented Jul 24, 2014

@Legrandin Maybe it makes sense to reopen the PR? Code still works. You do not necessarily support him after he goes in upstream.

@Legrandin

This comment has been minimized.

Show comment
Hide comment
@Legrandin

Legrandin Jul 24, 2014

Contributor

Understood. I reopen if it helps.

Contributor

Legrandin commented Jul 24, 2014

Understood. I reopen if it helps.

@Legrandin Legrandin reopened this Jul 24, 2014

@mdevaev

This comment has been minimized.

Show comment
Hide comment
@mdevaev

mdevaev commented Jul 24, 2014

@Legrandin Thanks, man!

@mdevaev

This comment has been minimized.

Show comment
Hide comment
@mdevaev

mdevaev Jul 24, 2014

@dlitz How about a merge? :-)

mdevaev commented Jul 24, 2014

@dlitz How about a merge? :-)

@dlitz

This comment has been minimized.

Show comment
Hide comment
@dlitz

dlitz Jul 24, 2014

Owner

You guys know that the C API is very slow under PyPy, right? Is that okay?

Owner

dlitz commented Jul 24, 2014

You guys know that the C API is very slow under PyPy, right? Is that okay?

@tycho

This comment has been minimized.

Show comment
Hide comment
@tycho

tycho Jul 24, 2014

Slow is better than completely broken.
On Jul 24, 2014 9:02 AM, "Dwayne Litzenberger" notifications@github.com
wrote:

You guys know that the C API is very slow under PyPy, right? Is that
okay?


Reply to this email directly or view it on GitHub
#59 (comment).

tycho commented Jul 24, 2014

Slow is better than completely broken.
On Jul 24, 2014 9:02 AM, "Dwayne Litzenberger" notifications@github.com
wrote:

You guys know that the C API is very slow under PyPy, right? Is that
okay?


Reply to this email directly or view it on GitHub
#59 (comment).

@mdevaev

This comment has been minimized.

Show comment
Hide comment
@mdevaev

mdevaev commented Jul 24, 2014

@dlitz Yep.

@OOPMan

This comment has been minimized.

Show comment
Hide comment
@OOPMan

OOPMan Jul 24, 2014

@dlitz Something that works now and is slow is better than something which doesn't work at all :-)

OOPMan commented Jul 24, 2014

@dlitz Something that works now and is slow is better than something which doesn't work at all :-)

@mdevaev

This comment has been minimized.

Show comment
Hide comment
@mdevaev

mdevaev commented Oct 10, 2014

@dlitz A-a-a-and?

@dlitz

This comment has been minimized.

Show comment
Hide comment
@dlitz

dlitz Oct 10, 2014

Owner

My first priority is always to avoid making things less secure for systems that are already deployed.

I don't have a lot of time to look this over now, but my main concern (without looking closely at it) is that this modifies the behavior of the code in the non-pypy case. In particular, I wonder if this introduces a timing side-channel.

Owner

dlitz commented Oct 10, 2014

My first priority is always to avoid making things less secure for systems that are already deployed.

I don't have a lot of time to look this over now, but my main concern (without looking closely at it) is that this modifies the behavior of the code in the non-pypy case. In particular, I wonder if this introduces a timing side-channel.

@mdevaev

This comment has been minimized.

Show comment
Hide comment
@mdevaev

mdevaev commented Oct 10, 2014

Great.

@arigo

This comment has been minimized.

Show comment
Hide comment
@arigo

arigo Feb 11, 2015

Note: it seems to work (or at least compile) correctly on PyPy if the system does not have libgmp-dev or libmpir-dev installed, because then _fastmath.c is not compiled. Maybe an intermediate fix could be checked in into pycrypto's official repository where the setup.py simply detects that it's running PyPy and if so, assume we don't want _fastmath.

It may make sense purely on performance grounds: _slowmath.py is likely faster than _fastmath.c because of PyPy's slow emulation of the CPython C API. I don't know about timing attacks though --- but also, these are very hard to reason about in a JITting environment anyway...

arigo commented Feb 11, 2015

Note: it seems to work (or at least compile) correctly on PyPy if the system does not have libgmp-dev or libmpir-dev installed, because then _fastmath.c is not compiled. Maybe an intermediate fix could be checked in into pycrypto's official repository where the setup.py simply detects that it's running PyPy and if so, assume we don't want _fastmath.

It may make sense purely on performance grounds: _slowmath.py is likely faster than _fastmath.c because of PyPy's slow emulation of the CPython C API. I don't know about timing attacks though --- but also, these are very hard to reason about in a JITting environment anyway...

@christopherobin

This comment has been minimized.

Show comment
Hide comment
@christopherobin

christopherobin Mar 26, 2015

In the meantime, a quick fix is to do:

> with_gmp=no pip install pycrypto

If using a requirements.txt you can also set that env variable.
Only downside is that any other module that might depend on gmp might also ignore it.

christopherobin commented Mar 26, 2015

In the meantime, a quick fix is to do:

> with_gmp=no pip install pycrypto

If using a requirements.txt you can also set that env variable.
Only downside is that any other module that might depend on gmp might also ignore it.

dhermes added a commit to dhermes/oauth2client that referenced this pull request Aug 13, 2015

Making sure libgmp is ignored in pypy tox env.
This is because when libgmp is detected, installing
PyCrypto tries to build _fastmath.c. But in PyPy this
fails because _fastmath.c tries to access parts of the
CPython implementation.

See for `with_gm=no` suggestion:
dlitz/pycrypto#59 (comment)

dhermes added a commit to dhermes/oauth2client that referenced this pull request Aug 13, 2015

Making sure libgmp is ignored in pypy tox env.
This is because when libgmp is detected, installing
PyCrypto tries to build _fastmath.c. But in PyPy this
fails because _fastmath.c tries to access parts of the
CPython implementation.

See for `with_gmp=no` suggestion:
dlitz/pycrypto#59 (comment)

dhermes added a commit to dhermes/oauth2client that referenced this pull request Aug 14, 2015

Making sure libgmp is ignored in pypy tox env.
This is because when libgmp is detected, installing
PyCrypto tries to build _fastmath.c. But in PyPy this
fails because _fastmath.c tries to access parts of the
CPython implementation.

See for `with_gmp=no` suggestion:
dlitz/pycrypto#59 (comment)
@sontek

This comment has been minimized.

Show comment
Hide comment
@sontek

sontek Oct 30, 2015

Anything need addressing to get this landed?

sontek commented Oct 30, 2015

Anything need addressing to get this landed?

@dlo

This comment has been minimized.

Show comment
Hide comment
@dlo

dlo Apr 21, 2016

@christopherobin thank you for that env variable tip. Got us over the edge. 👍

dlo commented Apr 21, 2016

@christopherobin thank you for that env variable tip. Got us over the edge. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment