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

volumemgr/updatestatus: fix crash by checking status.Blobs is not empty #2966

Merged
merged 2 commits into from
Dec 15, 2022

Conversation

rouming
Copy link
Contributor

@rouming rouming commented Dec 14, 2022

There is a crash with the following backtrace:

  github.com/lf-edge/eve/pkg/pillar/cmd/volumemgr.doUpdateContentTree(0xc000cf52c0, 0xc001a8d360, 0x42)
  pillar/cmd/volumemgr/updatestatus.go:233 +0x54b6
  github.com/lf-edge/eve/pkg/pillar/cmd/volumemgr.updateStatusByDatastore(0xc000cf52c0, 0xb42d8eee7664805, 0x23f4997dd60c22a7, 0xc001a4c190, 0xb, 0xc0018c30b0, 0x2c, 0x0, 0x0, 0x0, ...)
  /pillar/cmd/volumemgr/updatestatus.go:778 +0x3d2
  github.com/lf-edge/eve/pkg/pillar/cmd/volumemgr.handleDatastoreConfigImpl(0x23890e0, 0xc000cf52c0, 0xc0015b3ce0, 0x24, 0x26f6cc0, 0xc001a5e6e0
  /pillar/cmd/volumemgr/handledatastore.go:26 +0x198
  github.com/lf-edge/eve/pkg/pillar/cmd/volumemgr.handleDatastoreConfigCreate(0x23890e0, 0xc000cf52c0, 0xc0015b3ce0, 0x24, 0x26f6cc0, 0xc001a5e6e0)
  /pillar/cmd/volumemgr/handledatastore.go:12 +0x5d
  github.com/lf-edge/eve/pkg/pillar/pubsub.handleModify(0x26ac320, 0xc00078c100, 0xc0015b3ce0, 0x24, 0xc0012d42c0, 0x2a7, 0x2a9)
  /pillar/pubsub/subscribe.go:268 +0x7fd
  github.com/lf-edge/eve/pkg/pillar/pubsub.(*SubscriptionImpl).ProcessChange(0xc00078c100, 0x3, 0xc0015b3ce0, 0x24, 0xc0012d42c0, 0x2a7, 0x2a9)
  /pillar/pubsub/subscribe.go:132 +0x586

Patch introduces a simple check before array access and if array is empty - return an error.

!!! This patch more likely fixes the effect, not the cause. I need help from you @giggsoff @milan-zededa guys, if you know this particular code path.

UPDATE: @giggsoff pointed out that the following config (originally used by the customer) does not have a proper SHA256 for the image:

        {
            "uuid": "35ce9d94-f8c8-4a76-92ad-d43d764110a2",
            "dsId": "054866e7-eed8-420b-a722-0cd67d99f423",
            "URL": "container-disk-1G.qcow2",
            "iformat": "QCOW2",
            "sha256": "",
            "maxSizeBytes": "0",
            "siginfo": {
                "intercertsurl": "",
                "signercerturl": "",
                "signature": ""
            },
            "displayName": "container-disk-1G",
            "generationCount": "0"
        },

which causes the crash (reproduced by @giggsoff). Not clear how it is possible to create such a config without SHA256 on zedcloud, but this is another question.

For now I will keep the check introduced in this PR. There are a couple of suggestions from @giggsoff for additional checks, so will think how to add those too.

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

LGTM, but it presumably also makes sense to check for a sha256 earlier (in a separate PR; such a check would need to handle that a container reference might not have a sha and volumemgr uses downloading to resolve the OCI tag (e.g., latest) to get a sha.)
But let's merge the safety check in this PR.

@rouming
Copy link
Contributor Author

rouming commented Dec 15, 2022

Then I will keep is as a "hot fix" as is. Other checks will follow in another PR.

@rouming rouming changed the title updatestatus: fix crash by checking status.Blobs is not empty volumemgr/updatestatus: fix crash by checking status.Blobs is not empty Dec 15, 2022
@rouming
Copy link
Contributor Author

rouming commented Dec 15, 2022

Difference to the previous version:

  • add one commit with a sha256 check

Copy link
Contributor

@giggsoff giggsoff left a comment

Choose a reason for hiding this comment

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

It shows me the correct error when I create volume with no sha256 defined, thank you for finding and fixing it.

There is a crash with the following backtrace:

  runtime error: index out of range [0] with length 0

  github.com/lf-edge/eve/pkg/pillar/cmd/volumemgr.doUpdateContentTree(0xc000cf52c0, 0xc001a8d360, 0x42)
  pillar/cmd/volumemgr/updatestatus.go:233 +0x54b6
  github.com/lf-edge/eve/pkg/pillar/cmd/volumemgr.updateStatusByDatastore(0xc000cf52c0, 0xb42d8eee7664805, 0x23f4997dd60c22a7, 0xc001a4c190, 0xb, 0xc0018c30b0, 0x2c, 0x0, 0x0, 0x0, ...)
  /pillar/cmd/volumemgr/updatestatus.go:778 +0x3d2
  github.com/lf-edge/eve/pkg/pillar/cmd/volumemgr.handleDatastoreConfigImpl(0x23890e0, 0xc000cf52c0, 0xc0015b3ce0, 0x24, 0x26f6cc0, 0xc001a5e6e0
  /pillar/cmd/volumemgr/handledatastore.go:26 +0x198
  github.com/lf-edge/eve/pkg/pillar/cmd/volumemgr.handleDatastoreConfigCreate(0x23890e0, 0xc000cf52c0, 0xc0015b3ce0, 0x24, 0x26f6cc0, 0xc001a5e6e0)
  /pillar/cmd/volumemgr/handledatastore.go:12 +0x5d
  github.com/lf-edge/eve/pkg/pillar/pubsub.handleModify(0x26ac320, 0xc00078c100, 0xc0015b3ce0, 0x24, 0xc0012d42c0, 0x2a7, 0x2a9)
  /pillar/pubsub/subscribe.go:268 +0x7fd
  github.com/lf-edge/eve/pkg/pillar/pubsub.(*SubscriptionImpl).ProcessChange(0xc00078c100, 0x3, 0xc0015b3ce0, 0x24, 0xc0012d42c0, 0x2a7, 0x2a9)
  /pillar/pubsub/subscribe.go:132 +0x586

Patch introduces a simple check before array access and if
array is empty - return an error.

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
It is quite possible that we can get a configuration without a proper
sha256. Consider this as an error and return to the caller.

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
Co-developed-by: Petr Fedchenkov <giggsoff@gmail.com>
@rouming
Copy link
Contributor Author

rouming commented Dec 15, 2022

Difference to the previous version:

  • commit message tweaks

@eriknordmark eriknordmark merged commit afd753b into lf-edge:master Dec 15, 2022
@rouming rouming deleted the crash-updatestatus branch December 15, 2022 15:18
@giggsoff giggsoff added the stable Should be backported to stable release(s) label Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stable Should be backported to stable release(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants