diff --git a/cmd/skopeo/copy.go b/cmd/skopeo/copy.go index 52b269f8..6f1452a4 100644 --- a/cmd/skopeo/copy.go +++ b/cmd/skopeo/copy.go @@ -110,7 +110,7 @@ func parseMultiArch(multiArch string) (copy.ImageListSelection, error) { } } -func (opts *copyOptions) run(args []string, stdout io.Writer) error { +func (opts *copyOptions) run(args []string, stdout io.Writer) (retErr error) { if len(args) != 2 { return errorShouldDisplayUsage{errors.New("Exactly two arguments expected")} } @@ -125,7 +125,11 @@ func (opts *copyOptions) run(args []string, stdout io.Writer) error { if err != nil { return fmt.Errorf("Error loading trust policy: %v", err) } - defer policyContext.Destroy() + defer func() { + if err := policyContext.Destroy(); err != nil { + retErr = fmt.Errorf("(error tearing down policy context: %v): %w", err, retErr) + } + }() srcRef, err := alltransports.ParseImageName(imageNames[0]) if err != nil { diff --git a/cmd/skopeo/proxy.go b/cmd/skopeo/proxy.go index 14c14135..919b55eb 100644 --- a/cmd/skopeo/proxy.go +++ b/cmd/skopeo/proxy.go @@ -98,7 +98,7 @@ const maxMsgSize = 32 * 1024 // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER // We hard error if the input JSON numbers we expect to be // integers are above this. -const maxJSONFloat = float64(1<<53 - 1) +const maxJSONFloat = float64(uint64(1)<<53 - 1) // request is the JSON serialization of a function call type request struct { @@ -664,7 +664,14 @@ func proxyCmd(global *globalOptions) *cobra.Command { // processRequest dispatches a remote request. // replyBuf is the result of the invocation. // terminate should be true if processing of requests should halt. -func (h *proxyHandler) processRequest(req request) (rb replyBuf, terminate bool, err error) { +func (h *proxyHandler) processRequest(readBytes []byte) (rb replyBuf, terminate bool, err error) { + var req request + + // Parse the request JSON + if err = json.Unmarshal(readBytes, &req); err != nil { + err = fmt.Errorf("invalid request: %v", err) + return + } // Dispatch on the method switch req.Method { case "Initialize": @@ -717,18 +724,15 @@ func (opts *proxyOptions) run(args []string, stdout io.Writer) error { } return fmt.Errorf("reading socket: %v", err) } - // Parse the request JSON readbuf := buf[0:n] - var req request - if err := json.Unmarshal(readbuf, &req); err != nil { - rb := replyBuf{} - rb.send(conn, fmt.Errorf("invalid request: %v", err)) - } - rb, terminate, err := handler.processRequest(req) + rb, terminate, err := handler.processRequest(readbuf) if terminate { return nil } - rb.send(conn, err) + + if err := rb.send(conn, err); err != nil { + return fmt.Errorf("writing to socket: %w", err) + } } } diff --git a/cmd/skopeo/sync.go b/cmd/skopeo/sync.go index 159dfecd..a7dd4814 100644 --- a/cmd/skopeo/sync.go +++ b/cmd/skopeo/sync.go @@ -500,7 +500,7 @@ func imagesToCopy(source string, transport string, sourceCtx *types.SystemContex return descriptors, nil } -func (opts *syncOptions) run(args []string, stdout io.Writer) error { +func (opts *syncOptions) run(args []string, stdout io.Writer) (retErr error) { if len(args) != 2 { return errorShouldDisplayUsage{errors.New("Exactly two arguments expected")} } @@ -510,7 +510,11 @@ func (opts *syncOptions) run(args []string, stdout io.Writer) error { if err != nil { return errors.Wrapf(err, "Error loading trust policy") } - defer policyContext.Destroy() + defer func() { + if err := policyContext.Destroy(); err != nil { + retErr = fmt.Errorf("(error tearing down policy context: %v): %w", err, retErr) + } + }() // validate source and destination options contains := func(val string, list []string) (_ bool) { diff --git a/cmd/skopeo/utils.go b/cmd/skopeo/utils.go index 15450562..2e7b65b6 100644 --- a/cmd/skopeo/utils.go +++ b/cmd/skopeo/utils.go @@ -34,7 +34,7 @@ func commandAction(handler func(args []string, stdout io.Writer) error) func(cmd return func(c *cobra.Command, args []string) error { err := handler(args, c.OutOrStdout()) if _, ok := err.(errorShouldDisplayUsage); ok { - c.Help() + return c.Help() } return err } diff --git a/integration/check_test.go b/integration/check_test.go index 64046004..1ff25649 100644 --- a/integration/check_test.go +++ b/integration/check_test.go @@ -36,12 +36,12 @@ func (s *SkopeoSuite) SetUpSuite(c *check.C) { func (s *SkopeoSuite) TearDownSuite(c *check.C) { if s.regV2 != nil { - s.regV2.Close() + s.regV2.tearDown(c) } if s.regV2WithAuth != nil { //cmd := exec.Command("docker", "logout", s.regV2WithAuth) //c.Assert(cmd.Run(), check.IsNil) - s.regV2WithAuth.Close() + s.regV2WithAuth.tearDown(c) } } diff --git a/integration/copy_test.go b/integration/copy_test.go index 13f2020f..c3a55a5c 100644 --- a/integration/copy_test.go +++ b/integration/copy_test.go @@ -86,10 +86,10 @@ func (s *CopySuite) TearDownSuite(c *check.C) { os.RemoveAll(s.gpgHome) } if s.registry != nil { - s.registry.Close() + s.registry.tearDown(c) } if s.s1Registry != nil { - s.s1Registry.Close() + s.s1Registry.tearDown(c) } if s.cluster != nil { s.cluster.tearDown(c) diff --git a/integration/openshift.go b/integration/openshift.go index 4c7d9e6c..0264e9f1 100644 --- a/integration/openshift.go +++ b/integration/openshift.go @@ -258,12 +258,16 @@ func (cluster *openshiftCluster) relaxImageSignerPermissions(c *check.C) { // tearDown stops the cluster services and deletes (only some!) of the state. func (cluster *openshiftCluster) tearDown(c *check.C) { for i := len(cluster.processes) - 1; i >= 0; i-- { - cluster.processes[i].Process.Kill() + // It’s undocumented what Kill() returns if the process has terminated, + // so we couldn’t check just for that. This is running in a container anyway… + _ = cluster.processes[i].Process.Kill() } if cluster.workingDir != "" { - os.RemoveAll(cluster.workingDir) + err := os.RemoveAll(cluster.workingDir) + c.Assert(err, check.IsNil) } if cluster.dockerDir != "" { - os.RemoveAll(cluster.dockerDir) + err := os.RemoveAll(cluster.dockerDir) + c.Assert(err, check.IsNil) } } diff --git a/integration/proxy_test.go b/integration/proxy_test.go index 490277a8..3a7fc11c 100644 --- a/integration/proxy_test.go +++ b/integration/proxy_test.go @@ -57,7 +57,7 @@ type pipefd struct { fd *os.File } -func (self *proxy) call(method string, args []interface{}) (rval interface{}, fd *pipefd, err error) { +func (p *proxy) call(method string, args []interface{}) (rval interface{}, fd *pipefd, err error) { req := request{ Method: method, Args: args, @@ -66,7 +66,7 @@ func (self *proxy) call(method string, args []interface{}) (rval interface{}, fd if err != nil { return } - n, err := self.c.Write(reqbuf) + n, err := p.c.Write(reqbuf) if err != nil { return } @@ -76,7 +76,7 @@ func (self *proxy) call(method string, args []interface{}) (rval interface{}, fd } oob := make([]byte, syscall.CmsgSpace(1)) replybuf := make([]byte, maxMsgSize) - n, oobn, _, _, err := self.c.ReadMsgUnix(replybuf, oob) + n, oobn, _, _, err := p.c.ReadMsgUnix(replybuf, oob) if err != nil { err = fmt.Errorf("reading reply: %v", err) return @@ -119,9 +119,9 @@ func (self *proxy) call(method string, args []interface{}) (rval interface{}, fd return } -func (self *proxy) callNoFd(method string, args []interface{}) (rval interface{}, err error) { +func (p *proxy) callNoFd(method string, args []interface{}) (rval interface{}, err error) { var fd *pipefd - rval, fd, err = self.call(method, args) + rval, fd, err = p.call(method, args) if err != nil { return } @@ -132,9 +132,9 @@ func (self *proxy) callNoFd(method string, args []interface{}) (rval interface{} return rval, nil } -func (self *proxy) callReadAllBytes(method string, args []interface{}) (rval interface{}, buf []byte, err error) { +func (p *proxy) callReadAllBytes(method string, args []interface{}) (rval interface{}, buf []byte, err error) { var fd *pipefd - rval, fd, err = self.call(method, args) + rval, fd, err = p.call(method, args) if err != nil { return } @@ -150,7 +150,7 @@ func (self *proxy) callReadAllBytes(method string, args []interface{}) (rval int err: err, } }() - _, err = self.callNoFd("FinishPipe", []interface{}{fd.id}) + _, err = p.callNoFd("FinishPipe", []interface{}{fd.id}) if err != nil { return } @@ -241,7 +241,7 @@ func runTestGetManifestAndConfig(p *proxy, img string) error { } imgid := uint32(imgidv) - v, manifestBytes, err := p.callReadAllBytes("GetManifest", []interface{}{imgid}) + _, manifestBytes, err := p.callReadAllBytes("GetManifest", []interface{}{imgid}) if err != nil { return err } @@ -250,7 +250,7 @@ func runTestGetManifestAndConfig(p *proxy, img string) error { return err } - v, configBytes, err := p.callReadAllBytes("GetFullConfig", []interface{}{imgid}) + _, configBytes, err := p.callReadAllBytes("GetFullConfig", []interface{}{imgid}) if err != nil { return err } @@ -269,7 +269,7 @@ func runTestGetManifestAndConfig(p *proxy, img string) error { } // Also test this legacy interface - v, ctrconfigBytes, err := p.callReadAllBytes("GetConfig", []interface{}{imgid}) + _, ctrconfigBytes, err := p.callReadAllBytes("GetConfig", []interface{}{imgid}) if err != nil { return err } @@ -285,6 +285,9 @@ func runTestGetManifestAndConfig(p *proxy, img string) error { } _, err = p.callNoFd("CloseImage", []interface{}{imgid}) + if err != nil { + return err + } return nil } diff --git a/integration/registry.go b/integration/registry.go index 2a510351..e2ec22c5 100644 --- a/integration/registry.go +++ b/integration/registry.go @@ -126,7 +126,10 @@ func (t *testRegistryV2) Ping() error { return nil } -func (t *testRegistryV2) Close() { - t.cmd.Process.Kill() - os.RemoveAll(t.dir) +func (t *testRegistryV2) tearDown(c *check.C) { + // It’s undocumented what Kill() returns if the process has terminated, + // so we couldn’t check just for that. This is running in a container anyway… + _ = t.cmd.Process.Kill() + err := os.RemoveAll(t.dir) + c.Assert(err, check.IsNil) } diff --git a/integration/sync_test.go b/integration/sync_test.go index 2465a2ff..d7102c17 100644 --- a/integration/sync_test.go +++ b/integration/sync_test.go @@ -100,7 +100,7 @@ func (s *SyncSuite) TearDownSuite(c *check.C) { os.RemoveAll(s.gpgHome) } if s.registry != nil { - s.registry.Close() + s.registry.tearDown(c) } if s.cluster != nil { s.cluster.tearDown(c) @@ -278,7 +278,8 @@ func (s *SyncSuite) TestYamlUntagged(c *check.C) { // sync to the local registry yamlFile := path.Join(tmpDir, "registries.yaml") - ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + err = ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + c.Assert(err, check.IsNil) assertSkopeoSucceeds(c, "", "sync", "--scoped", "--src", "yaml", "--dest", "docker", "--dest-tls-verify=false", yamlFile, v2DockerRegistryURL) // sync back from local registry to a folder os.Remove(yamlFile) @@ -289,7 +290,8 @@ func (s *SyncSuite) TestYamlUntagged(c *check.C) { %s: [] `, v2DockerRegistryURL, imagePath) - ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + err = ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + c.Assert(err, check.IsNil) assertSkopeoSucceeds(c, "", "sync", "--scoped", "--src", "yaml", "--dest", "dir", yamlFile, dir1) sysCtx = types.SystemContext{ @@ -334,7 +336,8 @@ k8s.gcr.io: c.Assert(nTags, check.Not(check.Equals), 0) yamlFile := path.Join(tmpDir, "registries.yaml") - ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + err = ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + c.Assert(err, check.IsNil) assertSkopeoSucceeds(c, "", "sync", "--scoped", "--src", "yaml", "--dest", "dir", yamlFile, dir1) nManifests := 0 @@ -365,7 +368,8 @@ k8s.gcr.io: - sha256:59eec8837a4d942cc19a52b8c09ea75121acc38114a2c68b98983ce9356b8610 ` yamlFile := path.Join(tmpDir, "registries.yaml") - ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + err = ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + c.Assert(err, check.IsNil) assertSkopeoSucceeds(c, "", "sync", "--scoped", "--src", "yaml", "--dest", "dir", yamlFile, dir1) nManifests := 0 @@ -417,7 +421,8 @@ quay.io: c.Assert(nTags, check.Not(check.Equals), 0) yamlFile := path.Join(tmpDir, "registries.yaml") - ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + err = ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + c.Assert(err, check.IsNil) assertSkopeoSucceeds(c, "", "sync", "--scoped", "--src", "yaml", "--dest", "dir", yamlFile, dir1) nManifests := 0 @@ -481,7 +486,8 @@ func (s *SyncSuite) TestYamlTLSVerify(c *check.C) { for _, cfg := range testCfg { yamlConfig := fmt.Sprintf(yamlTemplate, v2DockerRegistryURL, cfg.tlsVerify, image, tag) yamlFile := path.Join(tmpDir, "registries.yaml") - ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + err = ioutil.WriteFile(yamlFile, []byte(yamlConfig), 0644) + c.Assert(err, check.IsNil) cfg.checker(c, cfg.msg, "sync", "--scoped", "--src", "yaml", "--dest", "dir", yamlFile, dir1) os.Remove(yamlFile)