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

tpmutil: Fix integer casting on 32-bit platforms #253

Merged
merged 1 commit into from
Jun 29, 2021

Conversation

josephlr
Copy link
Member

We need to make sure we only cast to ints once we are sure the value is
within the appropriate range (otherwise the value will wrap causing
errors).

Fixes #252

Signed-off-by: Joe Richey joerichey@google.com

We need to make sure we only cast to ints once we are sure the value is
within the appropriate range (otherwise the value will wrap causing
errors).

Fixes google#252

Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr josephlr requested a review from a team as a code owner June 22, 2021 06:29
@josephlr
Copy link
Member Author

CC: @davide125 can you confirm this fixes your issue

@josephlr
Copy link
Member Author

I can confirm that GOARCH=386 go test ./... panicked before this change, and now no longer panics.

I would add test with GOARCH=386 to our CI, but:

  • Getting CGO stuff to work on a cross-compiled build is a pain
  • Even if you set CGO_CFLAGS=-m32 GOARCH=386 CGO_ENABLED=1, the build still fails because you also need a 32-bit version of OpenSSL's libcrypto
  • I don't know how to install a 32-bit version of libcrypto (or even if the distros still carry it).
  • I don't know how to have the -lcrypto flag conditionally point to the 32-bit version.

So it doesn't seem worth the effort for such an obscure target.

@davide125
Copy link

Yep, that worked, thanks! You can see the full logs here: https://koji.fedoraproject.org/koji/taskinfo?taskID=70624896

wrt testing on different arches, one thing you may wanna look into is https://packit.dev/docs/guide/ which allows running CI jobs against all Fedora-supported architectures. I do agree it's probably not worth the trouble just for this though.

Copy link
Member

@chrisfenner chrisfenner left a comment

Choose a reason for hiding this comment

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

LGTM

@josephlr josephlr merged commit d5eb928 into google:master Jun 29, 2021
@josephlr josephlr deleted the cast branch June 29, 2021 18:53
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.

TestUnpackMalformedBytes fails on 32 bit architectures
3 participants