Repository: opencontainers/runc
Pull request #3460 (2022-04-15 18:11:00)

Do not use regexp
Don't get me wrong, I love regular expressions. But the regexp package is somewhat big and slow, and we can do just fine without.

Remove two last uses of regexp in runc code (modulo tests).

**This saves about 200K (2.3%) from the resulting binary, without losing any of the functionality.**

~~Currently a draft pending~~
 - [x] #3514 
 - [x] #3484 (which has https://github.com/urfave/cli/pull/1383)
 - [x] #3491 (which has https://github.com/cilium/ebpf/pull/642)
 - [x] #3543 (which has https://go-review.googlesource.com/c/protobuf/+/402774)
 - [x] #3585 (which has https://github.com/docker/go-units/pull/40)


kolyshkin: Fedora CI failed with
```
[04] run rootless tests ... (idmap+cgroup)
tests/rootless.sh: line 44: /etc/subuid.tmp: Read-only file system
make: *** [Makefile:103: localrootlessintegration] Error 1
make: Leaving directory '/vagrant'
```
which is pretty weird. Perhaps it's the problem with the underlying fs and so the kernel remounted it read-only.

Restarted.
AkihiroSuda: > slow

Yes, but not the bottleneck of runc AFAICS.
I don’t see necessity to remove regexp.
kolyshkin: A few of runc dependencies use regexp as well.
 * github.com/docker/go-units: fixed by https://github.com/docker/go-units/pull/40
 * github.com/cilium/ebpf: fixed by https://github.com/cilium/ebpf/pull/642
 * github.com/urfave/cli (via github.com/russross/blackfriday): being fixed by TODO (backport of https://github.com/urfave/cli/pull/1375)
 * google.golang.org/protobuf: being fixed by https://go-review.googlesource.com/c/protobuf/+/402774
 
 Once (and if) all of these are merged, I am going to bring this out of draft, as it will save us 200K (130K stripped) from the runc binary size (which is about 5%, comparing the stripped binaries).
kolyshkin: Currently saves 531K from the stripped binary (which is 5.6% of binary size). Still very far from crun footprint, yet it seems like a worthwhile saving.
kolyshkin: ebpf bump moved to https://github.com/opencontainers/runc/pull/3491, will rebase once merged.
kolyshkin: `urfave_cli` bump moved to https://github.com/opencontainers/runc/pull/3484, will rebase once merged.
kolyshkin: Rebased, made a fresh comparison:

```console
[kir@kir-rhat runc]$ go version
go version go1.18.1 linux/amd64

[kir@kir-rhat runc]$ git describe HEAD
v1.1.0-239-g93ad6a85

[kir@kir-rhat runc]$ ls -l runc.main runc.no-regexp
-rwxrwxr-x. 1 kir kir 12999304 Jul 14 13:31 runc.main
-rwxrwxr-x. 1 kir kir 12708080 Jul 14 13:31 runc.no-regexp

[kir@kir-rhat runc]$ strip runc.main runc.no-regexp

[kir@kir-rhat runc]$ ls -l runc.main runc.no-regexp
-rwxrwxr-x. 1 kir kir 9310880 Jul 14 13:33 runc.main
-rwxrwxr-x. 1 kir kir 9100352 Jul 14 13:33 runc.no-regexp

[kir@kir-rhat runc]$ size runc.main runc.no-regexp
   text	   data	    bss	    dec	    hex	filename
5645422	3657744	 229832	9532998	 917646	runc.main
5503086	3587520	 229416	9320022	 8e3656	runc.no-regexp
```

So, the saving is about 200K for a stripped binary.
kolyshkin: @thaJeztah addressed your comments, PTAL

Patch:
diff --git a/libcontainer/cgroups/systemd/common.go b/libcontainer/cgroups/systemd/common.go
index 0c3562e6..2f0767b2 100644
--- a/libcontainer/cgroups/systemd/common.go
+++ b/libcontainer/cgroups/systemd/common.go
@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"math"
 	"os"
-	"regexp"
 	"strconv"
 	"strings"
 	"sync"
@@ -231,18 +230,22 @@ func systemdVersion(cm *dbusConnManager) int {
 	return version
 }
 
-func systemdVersionAtoi(verStr string) (int, error) {
-	// verStr should be of the form:
-	// "v245.4-1.fc32", "245", "v245-1.fc32", "245-1.fc32" (without quotes).
-	// The result for all of the above should be 245.
-	// Thus, we unconditionally remove the "v" prefix
-	// and then match on the first integer we can grab.
-	re := regexp.MustCompile(`v?([0-9]+)`)
-	matches := re.FindStringSubmatch(verStr)
-	if len(matches) < 2 {
-		return 0, fmt.Errorf("can't parse version %s: incorrect number of matches %v", verStr, matches)
+// systemdVersionAtoi extracts a numeric systemd version from the argument.
+// The argument should be of the form: "v245.4-1.fc32", "245", "v245-1.fc32",
+// "245-1.fc32" (with or without quotes). The result for all of the above
+// should be 245.
+func systemdVersionAtoi(str string) (int, error) {
+	// Unconditionally remove the leading prefix ("v).
+	str = strings.TrimLeft(str, `"v`)
+	// Match on the first integer we can grab.
+	for i := 0; i < len(str); i++ {
+		if str[i] < '0' || str[i] > '9' {
+			// First non-digit: cut the tail.
+			str = str[:i]
+			break
+		}
 	}
-	ver, err := strconv.Atoi(matches[1])
+	ver, err := strconv.Atoi(str)
 	if err != nil {
 		return -1, fmt.Errorf("can't parse version: %w", err)
 	}
diff --git a/libcontainer/factory_linux.go b/libcontainer/factory_linux.go
index cd39ca38..b8d2d9c2 100644
--- a/libcontainer/factory_linux.go
+++ b/libcontainer/factory_linux.go
@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"os"
-	"regexp"
 	"runtime/debug"
 	"strconv"
 
@@ -27,8 +26,6 @@ const (
 	execFifoFilename = "exec.fifo"
 )
 
-var idRegex = regexp.MustCompile(`^[\w+-\.]+$`)
-
 // Create creates a new container with the given id inside a given state
 // directory (root), and returns a Container object.
 //
@@ -260,8 +257,47 @@ func loadState(root string) (*State, error) {
 	return state, nil
 }
 
+// validateID checks if the supplied container ID is valid, returning
+// the ErrInvalidID in case it is not.
+//
+// The format of valid ID was never formally defined, instead the code
+// was modified to allow or disallow specific characters.
+//
+// Currently, a valid ID is a non-empty string consisting only of
+// the following characters:
+// - uppercase (A-Z) and lowercase (a-z) Latin letters;
+// - digits (0-9);
+// - underscore (_);
+// - plus sign (+);
+// - minus sign (-);
+// - period (.).
+//
+// In addition, IDs that can't be used to represent a file name
+// (such as . or ..) are rejected.
+
 func validateID(id string) error {
-	if !idRegex.MatchString(id) || string(os.PathSeparator)+id != utils.CleanPath(string(os.PathSeparator)+id) {
+	if len(id) < 1 {
+		return ErrInvalidID
+	}
+
+	// Allowed characters: 0-9 A-Z a-z _ + - .
+	for i := 0; i < len(id); i++ {
+		c := id[i]
+		switch {
+		case c >= 'a' && c <= 'z':
+		case c >= 'A' && c <= 'Z':
+		case c >= '0' && c <= '9':
+		case c == '_':
+		case c == '+':
+		case c == '-':
+		case c == '.':
+		default:
+			return ErrInvalidID
+		}
+
+	}
+
+	if string(os.PathSeparator)+id != utils.CleanPath(string(os.PathSeparator)+id) {
 		return ErrInvalidID
 	}
 


Test patch:
diff --git a/libcontainer/cgroups/systemd/systemd_test.go b/libcontainer/cgroups/systemd/systemd_test.go
index d280a309..40584f78 100644
--- a/libcontainer/cgroups/systemd/systemd_test.go
+++ b/libcontainer/cgroups/systemd/systemd_test.go
@@ -35,8 +35,11 @@ func TestSystemdVersion(t *testing.T) {
 		{`"v245.4-1.fc32"`, 245, false},
 		{`"241-1"`, 241, false},
 		{`"v241-1"`, 241, false},
-		{"NaN", 0, true},
-		{"", 0, true},
+		{`333.45"`, 333, false},
+		{`v321-0`, 321, false},
+		{"NaN", -1, true},
+		{"", -1, true},
+		{"v", -1, true},
 	}
 	for _, sdTest := range systemdVersionTests {
 		ver, err := systemdVersionAtoi(sdTest.verStr)