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

Use fixed-length integers to improve cross-platform compatibility #492

Open
pierluca opened this issue Aug 24, 2023 · 5 comments · Fixed by #529
Open

Use fixed-length integers to improve cross-platform compatibility #492

pierluca opened this issue Aug 24, 2023 · 5 comments · Fixed by #529

Comments

@pierluca
Copy link
Contributor

pierluca commented Aug 24, 2023

As discussed in the review of #490 with @ineiti and @howjmay , Kyber uses structures with Go int-typed fields, which are unsized. This can lead to differences in serialization and hash computations across different processor architectures.

It would be preferable to switch to sized types (int32 and int64), which would provide consistent behaviours across platforms.

@ineiti
Copy link
Member

ineiti commented Nov 15, 2023

Additional notes: this probably needs a major version change, as the fields are involved in the hashes. So if a 64-bit machine stored the fields as int, it would store them as 64-bit fields. Then if we change the int to int32, it would be incompatible. The same thing would happen with a 32-bit system and changing the int to int64.

However, #493 suggests that this never worked on a 32-bit system. So I think if we change every int to int64, it should work.

@matteosz matteosz self-assigned this Jun 3, 2024
@matteosz matteosz linked a pull request Jun 9, 2024 that will close this issue
@pierluca
Copy link
Contributor Author

Following #529 , Kyber now compiles and works in both 32-bit and 64-bit architectures.
This effectively makes this issue more problematic.

@pierluca pierluca assigned pierluca, AnomalRoil and K1li4nL and unassigned matteosz Jul 15, 2024
@pierluca
Copy link
Contributor Author

I've added you (@AnomalRoil and @K1li4nL) and me as assignees so that we can discuss this and decide together the best way forward ahead of the v4 release.

@pierluca pierluca assigned jbsv and unassigned K1li4nL Oct 23, 2024
@pierluca
Copy link
Contributor Author

Discussed with @K1li4nL , we might want to do some automatic AST changes. Assigning @jbsv who may be able to help.

@ineiti
Copy link
Member

ineiti commented Oct 24, 2024

Why not changing every int to either int32 or int64? As additional security measure, tell protobuf to panic if it sees an int.

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

Successfully merging a pull request may close this issue.

6 participants