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: prevent integer overflow in 32-bit binaries when configuring virtual disk #2200

Merged
merged 1 commit into from
May 24, 2024

Conversation

spacegospod
Copy link
Collaborator

@spacegospod spacegospod commented May 17, 2024

Description

Virtual disk sizes are passed to the API in bytes during VM reconfiguration. This means that sizes in excess of 2GB will overflow a signed 32-bit integer.

In golang the default int type's size is dependent on the OS. This means that trying to add a virtual disk larger than 2GB while cloning a VM and running a 32-bit version of the provider will cause an overflow.

The bindings that we use to call the API use int64 which means that the problem is in the provider and not in govmomi.

The actual conversion looks like this:

disk.CapacityInBytes = structure.GiBToByte(ns.(int))

The bug is caused by GiBToByte which overflows internally and hides the problem.

func GiBToByte(n interface{}) int64 {
	switch v := n.(type) {
	case int:
		return int64(v * int(math.Pow(1024, 3)))
	case int32:
		return int64(v * int32(math.Pow(1024, 3)))
	case int64:
		return v * int64(math.Pow(1024, 3))
	}
	panic(fmt.Errorf("non-integer type %T for value", n))
}

We end up in the 1st case. The integer cast in the brackets is effectively a int32 cast since we run on a 32bit OS.

This causes the overflow.

To fix this I am modifying the call to GiBToByte by passing the parameter as int64 which forces the function to go through the 64bit case.

Acceptance tests

  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch?

Ran TestAccResourceVSphereVirtualMachine_cloneFromTemplate and TestAccResourceVSphereVirtualMachine_basic just to make sure there are no obvious regressions

  • Prepared Terraform on a 32bit win10 VM
  • Created a configuration that reproduces the bug by cloning a VM and adding a 100GB disk
  • Recompiled the provider with the fix and configured a local override on the VM
  • Verified that the same configuration works as expected

Release Note

Release note for CHANGELOG:

NONE

References

Closes #2116 1578

@spacegospod spacegospod self-assigned this May 17, 2024
@spacegospod spacegospod requested a review from a team as a code owner May 17, 2024 13:19
@github-actions github-actions bot added provider Type: Provider needs-review Status: Pull Request Needs Review labels May 17, 2024
@tenthirtyam tenthirtyam changed the title bugfix: Prevent integer overflow in 32-bit binaries when configuring virtual disk fix: prevent integer overflow in 32-bit binaries when configuring virtual disk May 17, 2024
@tenthirtyam tenthirtyam added this to the v2.8.2 milestone May 17, 2024
Modified the call to `GiBToByte` by passing the parameter as int64 which forces the function to go through the 64bit case.

Signed-off-by: Stoyan Zhelyazkov <stoyan.zhelyazkov@broadcom.com>
@github-actions github-actions bot added the documentation Type: Documentation label May 17, 2024
Copy link
Collaborator

@tenthirtyam tenthirtyam left a comment

Choose a reason for hiding this comment

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

Nice fix!

I added an update to the CHANGELOG tagging this under v2.8.2.

@tenthirtyam tenthirtyam merged commit 89c78cb into main May 24, 2024
7 checks passed
@tenthirtyam tenthirtyam deleted the bugfix/vm-disk-int-overflow branch May 24, 2024 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Type: Documentation needs-review Status: Pull Request Needs Review provider Type: Provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TF reports error on VM creation when trying to switch back disk to Thick Provision Eager Zeroed (intermittent)
3 participants