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 nil pointer dereference on CreateVM #176

Merged
merged 1 commit into from
Oct 27, 2021
Merged

Conversation

yitsushi
Copy link
Contributor

What this PR does / why we need it:

The system does not allow to create a VM with both rootfs and initrd
specified, that means we should be able to create a vm without initrd,
but the code had some strange assumption about that:

   160:         _, err = client.PutGuestBootSource(ctx, &fcmodels.BootSource{
   161:                 KernelImagePath: &cfg.BootSource.KernelImagePage,
   162:                 BootArgs:        *cfg.BootSource.BootArgs,
=> 163:                 InitrdPath:      *cfg.BootSource.InitrdPath,
   164:         })

At the same time, it can happen with BootArgs too as it's not a required
field.

Which issue(s) this PR fixes:

Fixes #175

Special notes for your reviewer:

Checklist:

  • squashed commits into logical changes
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@yitsushi yitsushi added the kind/bug Something isn't working label Oct 27, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #176 (382368a) into main (94ab549) will decrease coverage by 0.14%.
The diff coverage is 0.00%.

❗ Current head 382368a differs from pull request most recent head 7455a11. Consider uploading reports for the commit 7455a11 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #176      +/-   ##
==========================================
- Coverage   47.00%   46.85%   -0.15%     
==========================================
  Files          45       45              
  Lines        1951     1957       +6     
==========================================
  Hits          917      917              
- Misses        930      936       +6     
  Partials      104      104              
Impacted Files Coverage Δ
infrastructure/firecracker/config.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3217c47...7455a11. Read the comment docs.

richardcase
richardcase previously approved these changes Oct 27, 2021

bootArgs := ""
if cfg.BootSource.BootArgs != nil {
bootArgs = *cfg.BootSource.BootArgs
Copy link
Member

@richardcase richardcase Oct 27, 2021

Choose a reason for hiding this comment

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

As an alternative, we could create the fcmodels.BootSource before the api call and set the values if not nil. This works though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that more, updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// I tried to remember why I did this and I remember. First I just solved the initrd problem and then realized bootargs can face the same issue, so fixed that too and my brain did not say "hey you conditionally set 2 fields from a struct that has 3 fields"

The system does not allow to create a VM with both rootfs and initrd
specified, that means we should be able to create a vm without initrd,
but the code had some strange assumption about that:

       160:         _, err = client.PutGuestBootSource(ctx, &fcmodels.BootSource{
       161:                 KernelImagePath: &cfg.BootSource.KernelImagePage,
       162:                 BootArgs:        *cfg.BootSource.BootArgs,
    => 163:                 InitrdPath:      *cfg.BootSource.InitrdPath,
       164:         })

At the same time, it can happen with BootArgs too as it's not a required
field.

Fixes #175
@yitsushi yitsushi merged commit eda48d1 into main Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working size/xs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nil pointer dereference on CreateVM
3 participants