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: incorrect conversion between integer types #461

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

tenthirtyam
Copy link
Collaborator

Summary

Fixes incorrect conversion of an unsigned 32-bit integer from to a lower bit size type int32 without an upper bound check.

Testing

packer-plugin-vsphere on  fix/incorrect-conversion via 🐹 v1.23.0 go fmt ./...

packer-plugin-vsphere on  fix/incorrect-conversion via 🐹 v1.23.0 make test   
?       github.com/hashicorp/packer-plugin-vsphere      [no test files]
?       github.com/hashicorp/packer-plugin-vsphere/builder/vsphere/common/testing [no test files]
ok      github.com/hashicorp/packer-plugin-vsphere/builder/vsphere/clone        2.267s
?       github.com/hashicorp/packer-plugin-vsphere/examples/driver      [no test files]
?       github.com/hashicorp/packer-plugin-vsphere/version      [no test files]
ok      github.com/hashicorp/packer-plugin-vsphere/builder/vsphere/common       4.408s
ok      github.com/hashicorp/packer-plugin-vsphere/builder/vsphere/driver       12.610s
ok      github.com/hashicorp/packer-plugin-vsphere/builder/vsphere/iso  4.657s
ok      github.com/hashicorp/packer-plugin-vsphere/builder/vsphere/supervisor   12.468s
ok      github.com/hashicorp/packer-plugin-vsphere/post-processor/vsphere       6.774s
ok      github.com/hashicorp/packer-plugin-vsphere/post-processor/vsphere-template4.488s

Reference

@tenthirtyam tenthirtyam added this to the v1.5.0 milestone Aug 22, 2024
@tenthirtyam tenthirtyam self-assigned this Aug 22, 2024
@tenthirtyam tenthirtyam requested a review from a team as a code owner August 22, 2024 00:53
@tenthirtyam tenthirtyam added the security Security issues/fixes. label Aug 22, 2024
Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @tenthirtyam,

At first glance it would seem that the clamping is unnecessary as ParseInt already errors if the value can't fit on the provided bitSize, that said I understand this is meant to silence the errors reported by CodeQL, but I wonder if there's some other way we can document that those checks don't need to exist, and that they shouldn't be reported.

That said, those checks don't fundamentally harm the code so I'm not completely against merging this, but I think we would benefit from investigating how we can silence this check as it is being too cautious I think.

builder/vsphere/driver/vm.go Outdated Show resolved Hide resolved
builder/vsphere/driver/vm.go Outdated Show resolved Hide resolved
Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the reroll @tenthirtyam.

Only thing I can think of left is maybe using %s for interpolating errors in the error string, as you did a PR not too long ago to harmonise that, it may be good to continue using %s for those use cases.

That said it's very nitpicky so feel free to disregard if you think it's find as-is

Pre-approving to not block later

Fixes incorrect conversion of an unsigned 32-bit integer from  to a lower bit size type int32 without an upper bound check.

Signed-off-by: Ryan Johnson <[email protected]>
@lbajolet-hashicorp lbajolet-hashicorp merged commit 63f987f into main Aug 26, 2024
14 checks passed
@lbajolet-hashicorp lbajolet-hashicorp deleted the fix/incorrect-conversion branch August 26, 2024 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Security issues/fixes. tech-debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants