Skip to content

Fix 64-bit Windows compatibility - #14

Merged
warner merged 1 commit into
warner:masterfrom
okready:publickey-win64-fix
Jun 1, 2019
Merged

warner merged 1 commit into
warner:masterfrom
okready:publickey-win64-fix

Conversation

@okready

@okready okready commented Apr 17, 2019

Copy link
Copy Markdown
Contributor

Py_BuildValue() expects Py_ssize_t values for buffer lengths provided
for "y#" and similar format specifiers. The PUBLICKEYBYTES and
SECRETKEYBYTES constants passed to Py_BuildValue() in
ed25519_publickey() are treated as 32-bit int values by default, so they
must be cast to Py_ssize_t to work correctly on 64-bit architectures, as
Py_ssize_t is 64 bits on such targets.

For x86-64 targets, this was only causing issues on Windows, as the
Microsoft x64 calling convention only uses four registers for argument
passing instead of the six used by the System V AMD64 ABI (Linux, macOS,
etc.). 32-bit values are automatically zero-extended to 64-bits when
loaded into a register, so the size arguments get promoted to 64-bits
automatically if they can be passed in a register. The SECRETKEYBYTES
argument, being the fifth argument to Py_BuildValue(), gets passed on
the stack on Windows, so the upper 32-bits of the value read for that
argument are pulled from whatever is in memory adjacent to the value in
the stack, often leading to a MemoryError being raised as
Py_BuildValue() attempts to allocate a large amount of memory for the
"signkey" bytes object.

Py_BuildValue() expects Py_ssize_t values for buffer lengths provided
for "y#" and similar format specifiers. The PUBLICKEYBYTES and
SECRETKEYBYTES constants passed to Py_BuildValue() in
ed25519_publickey() are treated as 32-bit int values by default, so they
must be cast to Py_ssize_t to work correctly on 64-bit architectures, as
Py_ssize_t is 64 bits on such targets.

For x86-64 targets, this was only causing issues on Windows, as the
Microsoft x64 calling convention only uses four registers for argument
passing instead of the six used by the System V AMD64 ABI (Linux, macOS,
etc.). 32-bit values are automatically zero-extended to 64-bits when
loaded into a register, so the size arguments get promoted to 64-bits
automatically if they can be passed in a register. The SECRETKEYBYTES
argument, being the fifth argument to Py_BuildValue(), gets passed on
the stack on Windows, so the upper 32-bits of the value read for that
argument are pulled from whatever is in memory adjacent to the value in
the stack, often leading to a MemoryError being raised as
Py_BuildValue() attempts to allocate a large amount of memory for the
"signkey" bytes object.
@okready

okready commented Apr 17, 2019

Copy link
Copy Markdown
Contributor Author

I believe this fixes #12 (some comments mentioned the issue went away after downgrading to various Python versions, but I don't know whether that included switching from 64-bit to 32-bit versions of Python).

@okready okready mentioned this pull request Apr 18, 2019
@hclivess

Copy link
Copy Markdown

When merge?

@warner

warner commented Jun 1, 2019

Copy link
Copy Markdown
Owner

Wow, good catch and great analysis. The patch looks good, I'll land it shortly.

@warner
warner merged commit 19637be into warner:master Jun 1, 2019
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.

3 participants