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 Secure Comparator in PyThemis on 32-bit #555

Merged
merged 3 commits into from
Nov 11, 2019

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Nov 8, 2019

We have been using incorrect return type which breaks PyThemis on 32-bit systems. Let’s use correct type.

Checklist

  • Change is covered by automated tests (32-bit platforms tested in private build system)
  • Benchmark results are attached (not applicable)
  • The coding guidelines are followed
  • Public API has proper documentation
  • Example projects and code samples are updated (no API changes)
  • Changelog is updated if needed (good news added!)

secure_comparator_get_result() returns themis_status_t which is defined
as int32_t. We should expect 32-bit integer to be returned here.

This kinda worked on 64-bit platforms because of how registers work on
x86_64 and how 64-bit values are returned in System V ABI. However, this
does not work on i686 and causes Python to see garbage returned.
@ilammy ilammy added W-PyThemis 🐍 Wrapper: PyThemis, Python API compatibility Backward and forward compatibility, platform interoperability issues, breaking changes labels Nov 8, 2019
Copy link
Collaborator

@Lagovas Lagovas left a comment

Choose a reason for hiding this comment

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

how you found this problem? can we test it somehow?

@ilammy
Copy link
Collaborator Author

ilammy commented Nov 9, 2019

Our internal CI system has been failing because of Python tests being broken on 32-bit systems due to this bug. (No links since that triggers @shadinua and @gene-eu, see T1393 you-know-where.)

I've fired up a Debian Stretch i686 VM, installed Python, run the tests, and the failure immediately reproduced. Keen observation of the source file revealed the issue, and the fix worked on my machine. It also seems to fix the CI build as well, see you-know-what again.

Copy link
Collaborator

@Lagovas Lagovas left a comment

Choose a reason for hiding this comment

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

awesome)

Copy link
Contributor

@vixentael vixentael left a comment

Choose a reason for hiding this comment

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

great finding. merge it all!

@ilammy ilammy merged commit 0f60d10 into cossacklabs:master Nov 11, 2019
@ilammy ilammy deleted the python-32-bit branch November 11, 2019 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility Backward and forward compatibility, platform interoperability issues, breaking changes W-PyThemis 🐍 Wrapper: PyThemis, Python API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants