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

Fix segemation errror for ARM64/ Apple siliion #541

Closed
wants to merge 4 commits into from

Conversation

ghassanmas
Copy link

Relevant issues are

Description of work:

Solve segmentation error which might occur on apple silicon when using GMP,
the work around was to raise a new exception in Numbers module, if ARM64 is detected as the platform,
so that Numbers would export IntegerCustom or IntegerNative instead of IntegerGMP.

@ghassanmas
Copy link
Author

ghassanmas commented Aug 5, 2021

Please note, that the segmentation error doesn't occur if the system/platform failes to load the GMP at,

lib = load_lib("gmp", gmp_defs)
, since the ImportError will be raised at:
except (ImportError, OSError, AttributeError):
.

However if the GMP is in the system/platform, (which was the case for me when python3.9 was installed through brew) the load_lib will succeed, but then an function is called when for example creating a new instance of the Integer at:

_gmp.mpz_init(self._mpz_p)

A new instance of Integer is created or used by the RSA.PublicKey, when calling it's generate fucntion, hence this

Also lastly. for reference here is what GMP lastest release note state v6.2.1:

Issues with GMP 6.2.1:
While we added support for Apple's new Arm based computers, our support has a problem. The problem is that Apple reserves CPU register x18, but GMP's mpn/arm64 assembly code uses that register. While GMP runs fine in our tests, we expect things to go awry in some execution situation. (Apple has not been kind enough to specify how they use x18. Therefore, we don't know what the consequences of using x18 might be.)

@regisb
Copy link

regisb commented Aug 13, 2021

Could someone from pycryptodome please review this ASAP? I don't mean to be pushy, but (in my understanding) pycryptodome is now broken for all Mac OS users running Python 3.9, which affects our project.

Thanks a lot, and keep up the great work!

regisb added a commit to overhangio/tutor that referenced this pull request Sep 7, 2021
On Mac OS M1 (ARM processor), when the GMP library is available, pycryptodome
crashes with a segmentation fault during the generation of the RSA keys.

Upstream issue: Legrandin/pycryptodome#506
Upstream fix: Legrandin/pycryptodome#541

Close #473.
regisb added a commit to overhangio/tutor that referenced this pull request Sep 9, 2021
On Mac OS M1 (ARM processor), when the GMP library is available, pycryptodome
crashes with a segmentation fault during the generation of the RSA keys.

Upstream issue: Legrandin/pycryptodome#506
Upstream fix: Legrandin/pycryptodome#541

Close #473.
regisb added a commit to overhangio/tutor that referenced this pull request Sep 9, 2021
On Mac OS M1 (ARM processor), when the GMP library is available, pycryptodome
crashes with a segmentation fault during the generation of the RSA keys.

Upstream issue: Legrandin/pycryptodome#506
Upstream fix: Legrandin/pycryptodome#541

Close #473.
regisb added a commit to overhangio/tutor that referenced this pull request Sep 17, 2021
On Mac OS M1 (ARM processor), when the GMP library is available, pycryptodome
crashes with a segmentation fault during the generation of the RSA keys.

Upstream issue: Legrandin/pycryptodome#506
Upstream fix: Legrandin/pycryptodome#541

Close #473.
regisb added a commit to overhangio/tutor that referenced this pull request Sep 17, 2021
On Mac OS M1 (ARM processor), when the GMP library is available, pycryptodome
crashes with a segmentation fault during the generation of the RSA keys.

Upstream issue: Legrandin/pycryptodome#506
Upstream fix: Legrandin/pycryptodome#541

Close #473.
@Legrandin
Copy link
Owner

Is this fix really needed on all ARM64 platforms, or only on the M1 (one of the several types of ARM64)?
I will look into this for the M1, but we should not have regressions on other platforms unless it's needed.

@ghassanmas
Copy link
Author

@Legrandin No certainly it's only needed when for ARM and drawin/MAC. And actualy @regisb have had the same rationale, where he added a new condition to check for the OS running, i.e. sys.platform == "darwin".
That being said, I should this commit should be udpated to check for the OS as well.

@Legrandin
Copy link
Owner

Thanks for this PR.
In parallel, I have chased the root cause and I think I finally fixed with 2e7dad9, so it won't be necessary anymore to disable GMP on ARM (or on M1). The fix will end up in a new release very soon.

@Legrandin Legrandin closed this Sep 22, 2021
@regisb
Copy link

regisb commented Sep 23, 2021

Thanks again for your work @Legrandin!

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