Resolve docker.NewResolver race condition #8748

djdongjin · 2023-06-27T12:23:29Z

It can be reproduced with the added test:

# without the change:
$ go test -race ./remotes/docker/...
?       github.com/containerd/containerd/remotes/docker/schema1 [no test files]
==================
WARNING: DATA RACE
Read at 0x00c0003e08a0 by goroutine 189:
  runtime.mapaccess2_faststr()
      /usr/local/go/src/runtime/map_faststr.go:108 +0x0
  github.com/containerd/containerd/remotes/docker.NewResolver()
      /home/azureuser/code/containerd/remotes/docker/resolver.go:157 +0x22c
  github.com/containerd/containerd/remotes/docker.runBasicTest()
...

# with the change
$ go test -race ./remotes/docker/...
?       github.com/containerd/containerd/remotes/docker/schema1 [no test files]
ok      github.com/containerd/containerd/remotes/docker 0.677s
ok      github.com/containerd/containerd/remotes/docker/auth    (cached)
ok      github.com/containerd/containerd/remotes/docker/config  0.046s

remotes/docker/resolver.go

shuaichang

Thanks for sending out this quick PR for fixing it and greate go test with race detection!

remotes/docker/resolver.go

djdongjin · 2023-06-28T16:46:18Z

Added another commit to replace a similar case to using the builtin Clone(). #4855 fixed a similar issue a while ago.

@thaJeztah @dcantah wonder if you guys feel the current fix looks good or any suggestions I can improve (either in this PR or as follow-up). 😁

Very orthogonal, but I was actually looking at this part of the code recently, as it's somewhat awkward to use

I've always found this design kind of odd (NewResolver), as for CRI if you look at image_pull, we need to make a

Totally agree more improvement/refactor can be made to this part of code. I'm relatively new to this part of code so haven't figure out a good way to make larger improvement 😁

djdongjin · 2023-07-03T21:56:29Z

/cc @thaJeztah @dcantah
kindly ping, really appreciate any comments/feedback 🙏

k8s-ci-robot · 2023-07-03T21:56:33Z

@djdongjin: GitHub didn't allow me to request PR reviews from the following users: kindly, ping.

Note that only containerd members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @thaJeztah @dcantah kindly ping
really appreciate any comments/feedback 🙏

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

remotes/docker/resolver.go

dcantah

Sorry for the delay. @thaJeztah and I were both trying to make sense of the resolver code in general as we'd never dug in, but obviously it's a rabbit hole. I think this is fine given what it fixes. As a follow up (for anyone) we should see if there's anything we can do here to avoid a bunch of copies and needing to keep remaking resolver objects, whether that be API or otherwise

djdongjin · 2023-07-04T01:27:17Z

/retest

djdongjin · 2023-07-06T16:06:35Z

Thanks @dcantah @thaJeztah for reviewing my PR! 🙏
@thaJeztah appreciate any other comment/suggestion that I can improve in the PR. 😁

thaJeztah · 2023-07-08T07:25:38Z

I think the changes in this PR are ok to mitigate against situations where a shared headers map was inadvertently passed, but shouldn't we also fix this at the call site? Perhaps these need to make a copy as well;

containerd/pkg/cri/sbserver/images/image_pull.go

Line 135 in 3c250cb

Headers: c.config.Registry.Headers,

Signed-off-by: Jin Dong <djdongjin95@gmail.com>

djdongjin · 2023-07-08T13:01:46Z

but shouldn't we also fix this at the call site? Perhaps these need to make a copy as well;

Good point, yeah I think in general we should .Clone() a header if (1) it's passed from somewhere (e.g., as an arg or from a global config) (2) we make write operations.

That being said, I think the call sites should be fine (https://github.com/search?q=repo%3Acontainerd%2Fcontainerd+%22docker.NewResolver%28%22&type=code), given now NewResolver only reads (no writes) the passed http.Header. Calling NewResolver is similar to other read ops (e.g. read a header key, or Clone the header which just reads all key-value pairs).

Also around these call sites (or any use of http.Header), we should check if it satisfies the above (1) and (2). If so, we need to .Clone() the header, like we did in NewResolver. Checking the functions containing these NewResolver call sites, they didn't make writer operations (except in some case, creating a new header and pass to NewResolver).

Some cases I found in the codebase where we did need to .Clone, similar to the fix, include:

containerd/remotes/docker/resolver.go

Lines 475 to 478 in 3c250cb

    
           header := r.header.Clone() 
        
           if header == nil { 
        
           	header = http.Header{} 
        
           }

(I changed this copy to using the builtin .Clone)

containerd/remotes/docker/resolver.go

Lines 546 to 549 in 3c250cb

    
           req.Header = http.Header{} // headers need to be copied to avoid concurrent map access 
        
           for k, v := range r.header { 
        
           	req.Header[k] = v 
        
           }

@thaJeztah please let me know if this makes any sense or I missed anything 😬

fuweid

LGTM

thaJeztah reviewed Jun 27, 2023

View reviewed changes

remotes/docker/resolver.go Show resolved Hide resolved

shuaichang reviewed Jun 28, 2023

View reviewed changes

remotes/docker/resolver.go Show resolved Hide resolved

djdongjin force-pushed the cri-header-race branch 2 times, most recently from 3f46047 to f11aa82 Compare June 28, 2023 16:36

shuaichang approved these changes Jun 28, 2023

View reviewed changes

k8s-ci-robot requested review from thaJeztah and dcantah July 3, 2023 21:56

dcantah reviewed Jul 3, 2023

View reviewed changes

remotes/docker/resolver.go Outdated Show resolved Hide resolved

dcantah reviewed Jul 4, 2023

View reviewed changes

djdongjin force-pushed the cri-header-race branch 2 times, most recently from fd98e3e to 086d968 Compare July 4, 2023 00:54

djdongjin requested a review from dcantah July 5, 2023 21:08

dcantah approved these changes Jul 6, 2023

View reviewed changes

djdongjin force-pushed the cri-header-race branch 2 times, most recently from 12bd529 to d0fb31e Compare July 8, 2023 03:12

djdongjin added 2 commits July 8, 2023 05:25

Resolve docker.NewResolver race condition

cdb153e

Signed-off-by: Jin Dong <djdongjin95@gmail.com>

Change http.Header copy to builtin Clone

83ff030

Signed-off-by: Jin Dong <djdongjin95@gmail.com>

djdongjin force-pushed the cri-header-race branch from d0fb31e to 83ff030 Compare July 8, 2023 13:27

fuweid approved these changes Jul 10, 2023

View reviewed changes

fuweid added cherry-pick/1.6.x Change to be cherry picked to release/1.6 branch cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels Jul 10, 2023

estesp approved these changes Jul 10, 2023

View reviewed changes

estesp merged commit 97f2e3b into containerd:main Jul 10, 2023

This was referenced Jul 10, 2023

[release/1.7] Resolve docker.NewResolver race condition #8799

Merged

[release/1.6] Resolve docker.NewResolver race condition #8800

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve docker.NewResolver race condition #8748

Resolve docker.NewResolver race condition #8748

Resolve docker.NewResolver race condition #8748

Resolve docker.NewResolver race condition #8748

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment