PWX-29108 : Handle nil pointer dereference in cloudops #140
Conversation
* Add nil checks for pointer variables in aws/aws.go * Add nil checks for pointer variables in azure/azure.go * Add nil checks for pointer variables in oracle/oracle.go
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #140 +/- ##
=========================================
- Coverage 8.10% 7.61% -0.50%
=========================================
Files 18 18
Lines 4664 4966 +302
=========================================
Hits 378 378
- Misses 4264 4566 +302
Partials 22 22
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
Discussed offline. Summarizing here: |
|
Thanks Priyanshu for the advice. |
* Be very cautious with nil pointer check, avoid any false-positive check * Follow the following rules to detect nil pointer or index out of range - variable being deferenced in the same function - pointer variable used to access field or element in the same function - potential index out of range - variable mandatory for corresponding cloudops - variable caused panic bug before
| @@ -796,7 +808,18 @@ func (s *awsOps) Create( | |||
| return nil, cloudops.NewStorageError(cloudops.ErrVolInval, | |||
| "Drive type not specified in the storage spec", "") | |||
| } | |||
|
|
|||
| if vol.AvailabilityZone == nil { | |||
There was a problem hiding this comment.
According to CreateVolumeInput documentation
- AvailabilityZone is a required field
- The size of the volume, in GiBs. You must specify either a snapshot ID or a volume size.
- The snapshot from which to create the volume. You must specify either a snapshot ID or a volume size.
| @@ -796,7 +808,18 @@ func (s *awsOps) Create( | |||
| return nil, cloudops.NewStorageError(cloudops.ErrVolInval, | |||
There was a problem hiding this comment.
It is not mandatory but being dereference below
if *vol.VolumeType == opsworks.VolumeTypeIo1 || *vol.VolumeType == "gp3" {
| @@ -371,14 +371,20 @@ func (a *azureOps) Create( | |||
| options map[string]string, | |||
| ) (interface{}, error) { | |||
| d, ok := template.(*compute.Disk) | |||
| if !ok || d.DiskProperties == nil || d.DiskProperties.DiskSizeGB == nil { | |||
| if !ok || d.DiskProperties == nil { | |||
There was a problem hiding this comment.
According to the documentation of Azure - If creationData.createOption is Empty, this field is mandatory and it indicates the size of the disk to create.
Also, this parameter caused panic before - https://portworx.atlassian.net/browse/PWX-29069
What this PR does / why we need it:
Following issue was reported on gs-rel 2.13 on Azure PWX-29069. This was related to nil drive size being passed during drive create which was resulting into nil pointer dereference. This was fixed for Azure.
We need to do a thorough check on all the supported cloud platforms supported in cloudops for all the params that can be passed as “nil” that can result into a panic and handle those.
Which issue(s) this PR fixes
PWX-29108
Special notes for your reviewer:
General Workflow : Be very cautious with nil pointer check, avoid any false-positive
Follow the following rules to detect nil pointer or index out of range
Add special comments for rule 4.
Also, did a go fmt, some changes are based on that.