Skip to content

fix: incorrect conversion between integer types #461

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.

Sign up for GitHub

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

Jump to bottom
Merged
lbajolet-hashicorp merged 1 commit into from
Aug 26, 2024
Merged

fix: incorrect conversion between integer types #461

lbajolet-hashicorp merged 1 commit into from
Aug 26, 2024

Conversation

@tenthirtyam
Copy link
Collaborator

@tenthirtyam tenthirtyam commented Aug 22, 2024

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
lbajolet-hashicorp
lbajolet-hashicorp reviewed Aug 26, 2024
View reviewed changes
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.

@tenthirtyam tenthirtyam force-pushed the fix/incorrect-conversion branch from 5cb9506 to 4acaf07 Compare August 26, 2024 15:51
lbajolet-hashicorp
lbajolet-hashicorp approved these changes Aug 26, 2024
View reviewed changes
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

@tenthirtyam
fix: incorrect conversion between integer types
8c8e408 
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]>
@tenthirtyam tenthirtyam force-pushed the fix/incorrect-conversion branch from 4acaf07 to 8c8e408 Compare August 26, 2024 17:44
@lbajolet-hashicorp lbajolet-hashicorp merged commit 63f987f into main Aug 26, 2024
@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

Reviewers

@lbajolet-hashicorp lbajolet-hashicorp Awaiting requested review from lbajolet-hashicorp

Assignees

@tenthirtyam tenthirtyam

Labels

security Security issues/fixes. tech-debt

Projects

None yet

Milestone

v1.4.1

Development

Successfully merging this pull request may close these issues.

2 participants

@tenthirtyam @lbajolet-hashicorp