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
CLoses #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
@spacegospod spacegospod force-pushed the bugfix/vm-disk-int-overflow branch from 59ff63e to 566beb3 Compare May 17, 2024 13:28
@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 <[email protected]>
@tenthirtyam tenthirtyam force-pushed the bugfix/vm-disk-int-overflow branch from 566beb3 to 4431701 Compare May 17, 2024 14:14
@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
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 24, 2024
@iBrandyJackson iBrandyJackson linked an issue Jun 25, 2024 that may be closed by this pull request
4 tasks
@tenthirtyam tenthirtyam removed the needs-review Status: Pull Request Needs Review label Aug 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Type: Documentation provider Type: Provider
Projects
None yet
3 participants