From eaedadbed01219f6d074e739574f40ad7d72b126 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 4 Nov 2022 09:05:39 +0100 Subject: [PATCH] replace strings.Split(N) for strings.Cut() or alternatives Go 1.18 and up now provides a strings.Cut() which is better suited for splitting key/value pairs (and similar constructs), and performs better: ```go func BenchmarkSplit(b *testing.B) { b.ReportAllocs() data := []string{"12hello=world", "12hello=", "12=hello", "12hello"} for i := 0; i < b.N; i++ { for _, s := range data { _ = strings.SplitN(s, "=", 2)[0] } } } func BenchmarkCut(b *testing.B) { b.ReportAllocs() data := []string{"12hello=world", "12hello=", "12=hello", "12hello"} for i := 0; i < b.N; i++ { for _, s := range data { _, _, _ = strings.Cut(s, "=") } } } ``` BenchmarkSplit BenchmarkSplit-10 8244206 128.0 ns/op 128 B/op 4 allocs/op BenchmarkCut BenchmarkCut-10 54411998 21.80 ns/op 0 B/op 0 allocs/op While looking at occurrences of `strings.Split()`, I also updated some for alternatives, or added some constraints; for cases where an specific number of items is expected, I used `strings.SplitN()` with a suitable limit. This prevents (theoretical) unlimited splits. Signed-off-by: Sebastiaan van Stijn --- cmd/containerd-stress/density.go | 10 ++++---- cmd/ctr/commands/commands.go | 15 +++++------- cmd/ctr/commands/leases/leases.go | 8 ++---- cmd/ctr/commands/run/run.go | 6 ++--- cmd/ctr/commands/run/run_unix.go | 15 ++++++------ cmd/ctr/commands/run/run_windows.go | 8 +++--- cmd/gen-manpages/main.go | 5 ++-- contrib/apparmor/template.go | 17 ++++++------- images/mediatypes.go | 31 ++++++++++++++++-------- integration/container_log_test.go | 8 +++--- integration/main_test.go | 8 +++--- integration/sandbox_run_rollback_test.go | 7 +----- metadata/gc.go | 19 ++++++--------- oci/spec_opts.go | 12 ++++----- pkg/cap/cap_linux.go | 9 +++---- pkg/cri/opts/spec_windows.go | 8 +++--- pkg/cri/sbserver/image_pull.go | 5 ++-- pkg/cri/server/image_pull.go | 5 ++-- services/server/config/config.go | 6 ++--- 19 files changed, 96 insertions(+), 106 deletions(-) diff --git a/cmd/containerd-stress/density.go b/cmd/containerd-stress/density.go index 8006a6dda..0ca34c293 100644 --- a/cmd/containerd-stress/density.go +++ b/cmd/containerd-stress/density.go @@ -206,13 +206,13 @@ func parseStat(data string) (stat Stat, err error) { return stat, fmt.Errorf("invalid stat data: %q", data) } - parts := strings.SplitN(data[:i], "(", 2) - if len(parts) != 2 { + val, name, ok := strings.Cut(data[:i], "(") + if !ok { return stat, fmt.Errorf("invalid stat data: %q", data) } - stat.Name = parts[1] - _, err = fmt.Sscanf(parts[0], "%d", &stat.PID) + stat.Name = name + _, err = fmt.Sscanf(val, "%d", &stat.PID) if err != nil { return stat, err } @@ -220,7 +220,7 @@ func parseStat(data string) (stat Stat, err error) { // parts indexes should be offset by 3 from the field number given // proc(5), because parts is zero-indexed and we've removed fields // one (PID) and two (Name) in the paren-split. - parts = strings.Split(data[i+2:], " ") + parts := strings.Split(data[i+2:], " ") fmt.Sscanf(parts[22-3], "%d", &stat.StartTime) fmt.Sscanf(parts[4-3], "%d", &stat.PPID) return stat, nil diff --git a/cmd/ctr/commands/commands.go b/cmd/ctr/commands/commands.go index 4b8b9cf20..c0ac131a7 100644 --- a/cmd/ctr/commands/commands.go +++ b/cmd/ctr/commands/commands.go @@ -230,13 +230,10 @@ func ObjectWithLabelArgs(clicontext *cli.Context) (string, map[string]string) { func LabelArgs(labelStrings []string) map[string]string { labels := make(map[string]string, len(labelStrings)) for _, label := range labelStrings { - parts := strings.SplitN(label, "=", 2) - key := parts[0] - value := "true" - if len(parts) > 1 { - value = parts[1] + key, value, ok := strings.Cut(label, "=") + if !ok { + value = "true" } - labels[key] = value } @@ -247,11 +244,11 @@ func LabelArgs(labelStrings []string) map[string]string { func AnnotationArgs(annoStrings []string) (map[string]string, error) { annotations := make(map[string]string, len(annoStrings)) for _, anno := range annoStrings { - parts := strings.SplitN(anno, "=", 2) - if len(parts) != 2 { + key, value, ok := strings.Cut(anno, "=") + if !ok { return nil, fmt.Errorf("invalid key=value format annotation: %v", anno) } - annotations[parts[0]] = parts[1] + annotations[key] = value } return annotations, nil } diff --git a/cmd/ctr/commands/leases/leases.go b/cmd/ctr/commands/leases/leases.go index d9f1025e5..a454370a6 100644 --- a/cmd/ctr/commands/leases/leases.go +++ b/cmd/ctr/commands/leases/leases.go @@ -128,12 +128,8 @@ var createCommand = cli.Command{ if len(labelstr) > 0 { labels := map[string]string{} for _, lstr := range labelstr { - l := strings.SplitN(lstr, "=", 2) - if len(l) == 1 { - labels[l[0]] = "" - } else { - labels[l[0]] = l[1] - } + k, v, _ := strings.Cut(lstr, "=") + labels[k] = v } opts = append(opts, leases.WithLabels(labels)) } diff --git a/cmd/ctr/commands/run/run.go b/cmd/ctr/commands/run/run.go index 77414b1ab..95e014be8 100644 --- a/cmd/ctr/commands/run/run.go +++ b/cmd/ctr/commands/run/run.go @@ -62,13 +62,11 @@ func parseMountFlag(m string) (specs.Mount, error) { } for _, field := range fields { - v := strings.SplitN(field, "=", 2) - if len(v) < 2 { + key, val, ok := strings.Cut(field, "=") + if !ok { return mount, fmt.Errorf("invalid mount specification: expected key=val") } - key := v[0] - val := v[1] switch key { case "type": mount.Type = val diff --git a/cmd/ctr/commands/run/run_unix.go b/cmd/ctr/commands/run/run_unix.go index ef0aa2266..337c76250 100644 --- a/cmd/ctr/commands/run/run_unix.go +++ b/cmd/ctr/commands/run/run_unix.go @@ -310,16 +310,16 @@ func NewContainer(ctx gocontext.Context, client *containerd.Client, context *cli joinNs := context.StringSlice("with-ns") for _, ns := range joinNs { - parts := strings.Split(ns, ":") - if len(parts) != 2 { + nsType, nsPath, ok := strings.Cut(ns, ":") + if !ok { return nil, errors.New("joining a Linux namespace using --with-ns requires the format 'nstype:path'") } - if !validNamespace(parts[0]) { - return nil, errors.New("the Linux namespace type specified in --with-ns is not valid: " + parts[0]) + if !validNamespace(nsType) { + return nil, errors.New("the Linux namespace type specified in --with-ns is not valid: " + nsType) } opts = append(opts, oci.WithLinuxNamespace(specs.LinuxNamespace{ - Type: specs.LinuxNamespaceType(parts[0]), - Path: parts[1], + Type: specs.LinuxNamespaceType(nsType), + Path: nsPath, })) } if context.IsSet("gpus") { @@ -457,7 +457,8 @@ func getNewTaskOpts(context *cli.Context) []containerd.NewTaskOpts { } func parseIDMapping(mapping string) (specs.LinuxIDMapping, error) { - parts := strings.Split(mapping, ":") + // We expect 3 parts, but limit to 4 to allow detection of invalid values. + parts := strings.SplitN(mapping, ":", 4) if len(parts) != 3 { return specs.LinuxIDMapping{}, errors.New("user namespace mappings require the format `container-id:host-id:size`") } diff --git a/cmd/ctr/commands/run/run_windows.go b/cmd/ctr/commands/run/run_windows.go index d774d0c6e..af97a0be3 100644 --- a/cmd/ctr/commands/run/run_windows.go +++ b/cmd/ctr/commands/run/run_windows.go @@ -152,14 +152,14 @@ func NewContainer(ctx gocontext.Context, client *containerd.Client, context *cli opts = append(opts, oci.WithWindowsCPUMaximum(uint16(cmax))) } for _, dev := range context.StringSlice("device") { - parts := strings.Split(dev, "://") - if len(parts) != 2 { + idType, devID, ok := strings.Cut(dev, "://") + if !ok { return nil, errors.New("devices must be in the format IDType://ID") } - if parts[0] == "" { + if idType == "" { return nil, errors.New("devices must have a non-empty IDType") } - opts = append(opts, oci.WithWindowsDevice(parts[0], parts[1])) + opts = append(opts, oci.WithWindowsDevice(idType, devID)) } } diff --git a/cmd/gen-manpages/main.go b/cmd/gen-manpages/main.go index 61ec6a3af..4c87c1c0b 100644 --- a/cmd/gen-manpages/main.go +++ b/cmd/gen-manpages/main.go @@ -43,11 +43,10 @@ func run() error { } dir := flag.Arg(1) - parts := strings.SplitN(flag.Arg(0), ".", 2) - if len(parts) != 2 { + name, section, ok := strings.Cut(flag.Arg(0), ".") + if !ok { return fmt.Errorf("invalid name '%s': name does not contain man page section", flag.Arg(0)) } - name, section := parts[0], parts[1] appName, ok := apps[name] if !ok { diff --git a/contrib/apparmor/template.go b/contrib/apparmor/template.go index ba613c3e7..7544db749 100644 --- a/contrib/apparmor/template.go +++ b/contrib/apparmor/template.go @@ -100,9 +100,7 @@ type data struct { func cleanProfileName(profile string) string { // Normally profiles are suffixed by " (enforce)". AppArmor profiles cannot // contain spaces so this doesn't restrict daemon profile names. - if parts := strings.SplitN(profile, " ", 2); len(parts) >= 1 { - profile = parts[0] - } + profile, _, _ = strings.Cut(profile, " ") if profile == "" { profile = "unconfined" } @@ -184,17 +182,18 @@ func parseVersion(output string) (int, error) { // Copyright (C) 1999-2008 Novell Inc. // Copyright 2009-2012 Canonical Ltd. - lines := strings.SplitN(output, "\n", 2) - words := strings.Split(lines[0], " ") - version := words[len(words)-1] + version, _, _ := strings.Cut(output, "\n") + if i := strings.LastIndex(version, " "); i >= 0 { + version = version[i+1:] + } // trim "-beta1" suffix from version="3.0.0-beta1" if exists - version = strings.SplitN(version, "-", 2)[0] + version, _, _ = strings.Cut(version, "-") // also trim tilde - version = strings.SplitN(version, "~", 2)[0] + version, _, _ = strings.Cut(version, "~") // split by major minor version - v := strings.Split(version, ".") + v := strings.SplitN(version, ".", 4) if len(v) == 0 || len(v) > 3 { return -1, fmt.Errorf("parsing version failed for output: `%s`", output) } diff --git a/images/mediatypes.go b/images/mediatypes.go index 671e160e1..067963bab 100644 --- a/images/mediatypes.go +++ b/images/mediatypes.go @@ -38,7 +38,9 @@ const ( MediaTypeDockerSchema2Config = "application/vnd.docker.container.image.v1+json" MediaTypeDockerSchema2Manifest = "application/vnd.docker.distribution.manifest.v2+json" MediaTypeDockerSchema2ManifestList = "application/vnd.docker.distribution.manifest.list.v2+json" + // Checkpoint/Restore Media Types + MediaTypeContainerd1Checkpoint = "application/vnd.containerd.container.criu.checkpoint.criu.tar" MediaTypeContainerd1CheckpointPreDump = "application/vnd.containerd.container.criu.checkpoint.predump.tar" MediaTypeContainerd1Resource = "application/vnd.containerd.container.resource.tar" @@ -47,9 +49,12 @@ const ( MediaTypeContainerd1CheckpointOptions = "application/vnd.containerd.container.checkpoint.options.v1+proto" MediaTypeContainerd1CheckpointRuntimeName = "application/vnd.containerd.container.checkpoint.runtime.name" MediaTypeContainerd1CheckpointRuntimeOptions = "application/vnd.containerd.container.checkpoint.runtime.options+proto" - // Legacy Docker schema1 manifest + + // MediaTypeDockerSchema1Manifest is the legacy Docker schema1 manifest MediaTypeDockerSchema1Manifest = "application/vnd.docker.distribution.manifest.v1+prettyjws" - // Encypted media types + + // Encrypted media types + MediaTypeImageLayerEncrypted = ocispec.MediaTypeImageLayer + "+encrypted" MediaTypeImageLayerGzipEncrypted = ocispec.MediaTypeImageLayerGzip + "+encrypted" ) @@ -93,16 +98,23 @@ func DiffCompression(ctx context.Context, mediaType string) (string, error) { // parseMediaTypes splits the media type into the base type and // an array of sorted extensions -func parseMediaTypes(mt string) (string, []string) { +func parseMediaTypes(mt string) (mediaType string, suffixes []string) { if mt == "" { return "", []string{} } + mediaType, ext, ok := strings.Cut(mt, "+") + if !ok { + return mediaType, []string{} + } - s := strings.Split(mt, "+") - ext := s[1:] - sort.Strings(ext) - - return s[0], ext + // Splitting the extensions following the mediatype "(+)gzip+encrypted". + // We expect this to be a limited list, so add an arbitrary limit (50). + // + // Note that DiffCompression is only using the last element, so perhaps we + // should split on the last "+" only. + suffixes = strings.SplitN(ext, "+", 50) + sort.Strings(suffixes) + return mediaType, suffixes } // IsNonDistributable returns true if the media type is non-distributable. @@ -118,8 +130,7 @@ func IsLayerType(mt string) bool { } // Parse Docker media types, strip off any + suffixes first - base, _ := parseMediaTypes(mt) - switch base { + switch base, _ := parseMediaTypes(mt); base { case MediaTypeDockerSchema2Layer, MediaTypeDockerSchema2LayerGzip, MediaTypeDockerSchema2LayerForeign, MediaTypeDockerSchema2LayerForeignGzip: return true diff --git a/integration/container_log_test.go b/integration/container_log_test.go index bfff289dd..7896ca4e6 100644 --- a/integration/container_log_test.go +++ b/integration/container_log_test.go @@ -141,10 +141,10 @@ func checkContainerLog(t *testing.T, log string, messages []string) { lines := strings.Split(strings.TrimSpace(log), "\n") require.Len(t, lines, len(messages), "log line number should match") for i, line := range lines { - parts := strings.SplitN(line, " ", 2) - require.Len(t, parts, 2) - _, err := time.Parse(time.RFC3339Nano, parts[0]) + ts, msg, ok := strings.Cut(line, " ") + require.True(t, ok) + _, err := time.Parse(time.RFC3339Nano, ts) assert.NoError(t, err, "timestamp should be in RFC3339Nano format") - assert.Equal(t, messages[i], parts[1], "log content should match") + assert.Equal(t, messages[i], msg, "log content should match") } } diff --git a/integration/main_test.go b/integration/main_test.go index dc07d6697..3250b07d8 100644 --- a/integration/main_test.go +++ b/integration/main_test.go @@ -509,14 +509,14 @@ func PidEnvs(pid int) (map[string]string, error) { res := make(map[string]string) for _, value := range values { - value := strings.TrimSpace(string(value)) + value = bytes.TrimSpace(value) if len(value) == 0 { continue } - parts := strings.SplitN(value, "=", 2) - if len(parts) == 2 { - res[parts[0]] = parts[1] + k, v, ok := strings.Cut(string(value), "=") + if ok { + res[k] = v } } return res, nil diff --git a/integration/sandbox_run_rollback_test.go b/integration/sandbox_run_rollback_test.go index 8a8c08825..9a152e164 100644 --- a/integration/sandbox_run_rollback_test.go +++ b/integration/sandbox_run_rollback_test.go @@ -349,12 +349,7 @@ func ensureCNIAddRunning(t *testing.T, sbName string) error { } for _, arg := range strings.Split(args, ";") { - kv := strings.SplitN(arg, "=", 2) - if len(kv) != 2 { - continue - } - - if kv[0] == "K8S_POD_NAME" && kv[1] == sbName { + if arg == "K8S_POD_NAME="+sbName { return true, nil } } diff --git a/metadata/gc.go b/metadata/gc.go index a5551380b..288ace95c 100644 --- a/metadata/gc.go +++ b/metadata/gc.go @@ -443,13 +443,10 @@ func (c *gcContext) references(ctx context.Context, tx *bolt.Tx, node gc.Node, f return c.sendLabelRefs(node.Namespace, bkt, fn) case ResourceSnapshot, resourceSnapshotFlat: - parts := strings.SplitN(node.Key, "/", 2) - if len(parts) != 2 { + ss, name, ok := strings.Cut(node.Key, "/") + if !ok { return fmt.Errorf("invalid snapshot gc key %s", node.Key) } - ss := parts[0] - name := parts[1] - bkt := getBucket(tx, bucketKeyVersion, []byte(node.Namespace), bucketKeyObjectSnapshots, []byte(ss), []byte(name)) if bkt == nil { // Node may be created from dead edge @@ -563,7 +560,7 @@ func (c *gcContext) scanAll(ctx context.Context, tx *bolt.Tx, fn func(ctx contex } c.all(func(n gc.Node) { - fn(ctx, n) + _ = fn(ctx, n) }) return nil @@ -598,14 +595,14 @@ func (c *gcContext) remove(ctx context.Context, tx *bolt.Tx, node gc.Node) error case ResourceSnapshot: sbkt := nsbkt.Bucket(bucketKeyObjectSnapshots) if sbkt != nil { - parts := strings.SplitN(node.Key, "/", 2) - if len(parts) != 2 { + ss, key, ok := strings.Cut(node.Key, "/") + if !ok { return fmt.Errorf("invalid snapshot gc key %s", node.Key) } - ssbkt := sbkt.Bucket([]byte(parts[0])) + ssbkt := sbkt.Bucket([]byte(ss)) if ssbkt != nil { - log.G(ctx).WithField("key", parts[1]).WithField("snapshotter", parts[0]).Debug("remove snapshot") - return ssbkt.DeleteBucket([]byte(parts[1])) + log.G(ctx).WithField("key", key).WithField("snapshotter", ss).Debug("remove snapshot") + return ssbkt.DeleteBucket([]byte(key)) } } case ResourceLease: diff --git a/oci/spec_opts.go b/oci/spec_opts.go index e13b9a498..a3502e9ca 100644 --- a/oci/spec_opts.go +++ b/oci/spec_opts.go @@ -187,23 +187,23 @@ func replaceOrAppendEnvValues(defaults, overrides []string) []string { cache := make(map[string]int, len(defaults)) results := make([]string, 0, len(defaults)) for i, e := range defaults { - parts := strings.SplitN(e, "=", 2) + k, _, _ := strings.Cut(e, "=") results = append(results, e) - cache[parts[0]] = i + cache[k] = i } for _, value := range overrides { // Values w/o = means they want this env to be removed/unset. - if !strings.Contains(value, "=") { - if i, exists := cache[value]; exists { + k, _, ok := strings.Cut(value, "=") + if !ok { + if i, exists := cache[k]; exists { results[i] = "" // Used to indicate it should be removed } continue } // Just do a normal set/update - parts := strings.SplitN(value, "=", 2) - if i, exists := cache[parts[0]]; exists { + if i, exists := cache[k]; exists { results[i] = value } else { results = append(results, value) diff --git a/pkg/cap/cap_linux.go b/pkg/cap/cap_linux.go index 26212573d..63fa104fb 100644 --- a/pkg/cap/cap_linux.go +++ b/pkg/cap/cap_linux.go @@ -80,15 +80,14 @@ func ParseProcPIDStatus(r io.Reader) (map[Type]uint64, error) { scanner := bufio.NewScanner(r) for scanner.Scan() { line := scanner.Text() - pair := strings.SplitN(line, ":", 2) - if len(pair) != 2 { + k, v, ok := strings.Cut(line, ":") + if !ok { continue } - k := strings.TrimSpace(pair[0]) - v := strings.TrimSpace(pair[1]) + k = strings.TrimSpace(k) switch k { case "CapInh", "CapPrm", "CapEff", "CapBnd", "CapAmb": - ui64, err := strconv.ParseUint(v, 16, 64) + ui64, err := strconv.ParseUint(strings.TrimSpace(v), 16, 64) if err != nil { return nil, fmt.Errorf("failed to parse line %q", line) } diff --git a/pkg/cri/opts/spec_windows.go b/pkg/cri/opts/spec_windows.go index 76598730d..9b964e748 100644 --- a/pkg/cri/opts/spec_windows.go +++ b/pkg/cri/opts/spec_windows.go @@ -243,15 +243,15 @@ func WithDevices(config *runtime.ContainerConfig) oci.SpecOpts { hostPath := device.HostPath if strings.HasPrefix(hostPath, "class/") { - hostPath = strings.Replace(hostPath, "class/", "class://", 1) + hostPath = "class://" + strings.TrimPrefix(hostPath, "class/") } - splitParts := strings.SplitN(hostPath, "://", 2) - if len(splitParts) != 2 { + idType, id, ok := strings.Cut(hostPath, "://") + if !ok { return fmt.Errorf("unrecognised HostPath format %v, must match IDType://ID", device.HostPath) } - o := oci.WithWindowsDevice(splitParts[0], splitParts[1]) + o := oci.WithWindowsDevice(idType, id) if err := o(ctx, client, c, s); err != nil { return fmt.Errorf("failed adding device with HostPath %v: %w", device.HostPath, err) } diff --git a/pkg/cri/sbserver/image_pull.go b/pkg/cri/sbserver/image_pull.go index 2151221fe..7e4e85335 100644 --- a/pkg/cri/sbserver/image_pull.go +++ b/pkg/cri/sbserver/image_pull.go @@ -233,11 +233,10 @@ func ParseAuth(auth *runtime.AuthConfig, host string) (string, string, error) { if err != nil { return "", "", err } - fields := strings.SplitN(string(decoded), ":", 2) - if len(fields) != 2 { + user, passwd, ok := strings.Cut(string(decoded), ":") + if !ok { return "", "", fmt.Errorf("invalid decoded auth: %q", decoded) } - user, passwd := fields[0], fields[1] return user, strings.Trim(passwd, "\x00"), nil } // TODO(random-liu): Support RegistryToken. diff --git a/pkg/cri/server/image_pull.go b/pkg/cri/server/image_pull.go index e885a95b6..3e0f151d0 100644 --- a/pkg/cri/server/image_pull.go +++ b/pkg/cri/server/image_pull.go @@ -232,11 +232,10 @@ func ParseAuth(auth *runtime.AuthConfig, host string) (string, string, error) { if err != nil { return "", "", err } - fields := strings.SplitN(string(decoded), ":", 2) - if len(fields) != 2 { + user, passwd, ok := strings.Cut(string(decoded), ":") + if !ok { return "", "", fmt.Errorf("invalid decoded auth: %q", decoded) } - user, passwd := fields[0], fields[1] return user, strings.Trim(passwd, "\x00"), nil } // TODO(random-liu): Support RegistryToken. diff --git a/services/server/config/config.go b/services/server/config/config.go index 3f17ce589..340c93b31 100644 --- a/services/server/config/config.go +++ b/services/server/config/config.go @@ -104,17 +104,17 @@ func (c *Config) ValidateV2() error { return nil } for _, p := range c.DisabledPlugins { - if len(strings.Split(p, ".")) < 4 { + if !strings.HasPrefix(p, "io.containerd.") || len(strings.SplitN(p, ".", 4)) < 4 { return fmt.Errorf("invalid disabled plugin URI %q expect io.containerd.x.vx", p) } } for _, p := range c.RequiredPlugins { - if len(strings.Split(p, ".")) < 4 { + if !strings.HasPrefix(p, "io.containerd.") || len(strings.SplitN(p, ".", 4)) < 4 { return fmt.Errorf("invalid required plugin URI %q expect io.containerd.x.vx", p) } } for p := range c.Plugins { - if len(strings.Split(p, ".")) < 4 { + if !strings.HasPrefix(p, "io.containerd.") || len(strings.SplitN(p, ".", 4)) < 4 { return fmt.Errorf("invalid plugin key URI %q expect io.containerd.x.vx", p) } }