Skip to content

Conversation

@samuelkarp
Copy link
Member

A nil CRIImplementation field can cause a nil pointer dereference and panic during startup recovery.

Prior to this change, the nri.API struct would have a nil cri (CRIImplementation) field after nri.NewAPI until nri.Register was called. Register is called mid-way through initialization of the CRI plugin, but recovery for containers occurs prior to that. Container recovery includes establishing new exit monitors for existing containers that were discovered. When a container exits, NRI plugins are given the opportunity to be notified about the lifecycle event, and this is done by accessing that CRIImplementation field inside the nri.API. If a container exits prior to nri.Register being called, access to the CRIImplementation field can cause a panic.

Here's the call-path:

  • The CRI plugin starts running here
  • It then calls into recover() to recover state from previous runs of containerd
  • recover() then attempts to recover all containers through loadContainer()
  • When loadContainer() finds a container that is still running, it waits for the task (internal containerd object) to exit and sets up exit monitoring
  • Any exit that then happens must be handled
  • Handling an exit includes deleting the Task and specifying nri.WithContainerExit to notify any subscribed NRI plugins
  • NRI plugins need to know information about the pod (not just the sandbox), so before a plugin is notified the NRI API package queries the Sandbox Store through the CRI implementation
  • The cri implementation member field in the nri.API struct is set as part of the Register() method
  • The nri.Register() method is only called much further down in the CRI Run() method

A nil CRIImplementation field can cause a nil pointer dereference and
panic during startup recovery.

Prior to this change, the nri.API struct would have a nil cri
(CRIImplementation) field after nri.NewAPI until nri.Register was
called.  Register is called mid-way through initialization of the CRI
plugin, but recovery for containers occurs prior to that.  Container
recovery includes establishing new exit monitors for existing containers
that were discovered.  When a container exits, NRI plugins are given the
opportunity to be notified about the lifecycle event, and this is done
by accessing that CRIImplementation field inside the nri.API.  If a
container exits prior to nri.Register being called, access to the
CRIImplementation field can cause a panic.

Here's the call-path:

* The CRI plugin starts running
  [here](https://github.com/containerd/containerd/blob/ae71819c4f5e67bb4d5ae76a6b735f29cc25774e/pkg/cri/server/service.go#L222)
* It then [calls into](https://github.com/containerd/containerd/blob/ae71819c4f5e67bb4d5ae76a6b735f29cc25774e/pkg/cri/server/service.go#L227)
  `recover()` to recover state from previous runs of containerd
* `recover()` then attempts to recover all containers through
  [`loadContainer()`](https://github.com/containerd/containerd/blob/ae7d74b9e21bd08260586db104a1fe04af754545/internal/cri/server/restart.go#L175)
* When `loadContainer()` finds a container that is still running, it waits
  for the task (internal containerd object) to exit and sets up
  [exit monitoring](https://github.com/containerd/containerd/blob/ae7d74b9e21bd08260586db104a1fe04af754545/internal/cri/server/restart.go#L391)
* Any exit that then happens must be
  [handled](https://github.com/containerd/containerd/blob/ae7d74b9e21bd08260586db104a1fe04af754545/internal/cri/server/events.go#L145)
* Handling an exit includes
  [deleting the Task](https://github.com/containerd/containerd/blob/ae7d74b9e21bd08260586db104a1fe04af754545/internal/cri/server/events.go#L188)
  and specifying [`nri.WithContainerExit`](https://github.com/containerd/containerd/blob/ae7d74b9e21bd08260586db104a1fe04af754545/internal/cri/nri/nri_api_linux.go#L348)
  to [notify](https://github.com/containerd/containerd/blob/ae7d74b9e21bd08260586db104a1fe04af754545/internal/cri/nri/nri_api_linux.go#L356)
  any subscribed NRI plugins
* NRI plugins need to know information about the pod (not just the sandbox),
  so before a plugin is notified the NRI API package
  [queries the Sandbox Store](https://github.com/containerd/containerd/blob/ae7d74b9e21bd08260586db104a1fe04af754545/internal/cri/nri/nri_api_linux.go#L232)
  through the CRI implementation
* The `cri` implementation member field in the `nri.API` struct is set as part of the
  [`Register()`](https://github.com/containerd/containerd/blob/ae7d74b9e21bd08260586db104a1fe04af754545/internal/cri/nri/nri_api_linux.go#L66) method
* The `nri.Register()` method is only called
  [much further down in the CRI `Run()` method](https://github.com/containerd/containerd/blob/ae71819c4f5e67bb4d5ae76a6b735f29cc25774e/pkg/cri/server/service.go#L279)

Signed-off-by: Samuel Karp <samuelkarp@google.com>
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM

@samuelkarp samuelkarp added the cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch label Jul 1, 2024
@samuelkarp samuelkarp added this pull request to the merge queue Jul 1, 2024
@samuelkarp samuelkarp added the area/cri Container Runtime Interface (CRI) label Jul 1, 2024
Merged via the queue into containerd:main with commit ebcbbe5 Jul 1, 2024
@samuelkarp samuelkarp added cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch cherry-picked/sbserver Changes are backported to sbserver and removed cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/cri Container Runtime Interface (CRI) cherry-picked/sbserver Changes are backported to sbserver cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants