From 0539b7073edda2c58929754efab6a87352e715ce Mon Sep 17 00:00:00 2001
From: Sebastiaan van Stijn <github@gone.nl>
Date: Sun, 20 Oct 2024 16:39:29 +0200
Subject: [PATCH] pkg/stringid: optimize GenerateRandomID

GenerateRandomID has a check to verify if the generated ID was numeric. This
check was added because a container's short-ID is used as default hostname for
containers, which isn't allowed to be consisting of only numbers (see [moby#3869]
and https://bugzilla.redhat.com/show_bug.cgi?id=1059122.

Producing an random ID with only numbers is a rare corner-case, but the check
would always be executed and wasn't optimized.

This patch applies some optimizations:

- The code was using `strconv.ParseUInt`, which has additional checks for
  signs ("+" or "-"); `hex.EncodeToString` would never produce these, so
  we can use `strconv.ParseInt` instead (which doesn't have these checks).
- The code was using `TruncateID(id)` to get the short-ID. The `TruncateID`
  function is designed to also handle digests, and for that checks for
  the given ID to contain colons (`:`), which it would split to remove
  the algorithm (`sha256:`) before truncating to the short-ID length.
  That check wasn't needed either, because those would not be produced
  by `hex.EncodeToString`, so instead, we can just truncate the ID.
- Finally, all we _really_ need to check for is if the ID consists of only
  numeric characters (`0-9`) so, let's do just that; if any non-numeric
  value is found, the ID is valid, and we can terminate the loop.

I did some basic benchmark to compare all of the above in isolation;

- BenchmarkParseInt: `strconv.ParseInt(TruncateID(id), 10, 64)`
- BenchmarkParseUInt: `strconv.ParseUint(TruncateID(id), 10, 64)`
- BenchmarkParseUIntNoTrunc: `strconv.ParseUint(id[:shortLen], 10, 64)`
- BenchmarkAllNum: `allNum(id[:shortLen])`

Results of the above:

    BenchmarkParseInt-10                1713937       691.0 ns/op     480 B/op      18 allocs/op
    BenchmarkParseIntNoTrunc-10         3385483       356.1 ns/op     480 B/op      18 allocs/op
    BenchmarkParseUInt-10               2112538       567.7 ns/op     384 B/op      12 allocs/op
    BenchmarkParseUIntNoTrunc-10        4325847       266.7 ns/op     384 B/op      12 allocs/op
    BenchmarkAllNum-10                 77277264        15.29 ns/op      0 B/op       0 allocs/op

Difference for `GenerateRandomID` as a whole is less dramatic, as in most
cases `ParseInt` would bail out early, but still saves some allocations, and
performance is ~14% better:

    BenchmarkGenerateRandomID-10        2807764       424.5 ns/op     240 B/op       6 allocs/op
    BenchmarkGenerateRandomIDNew-10     3288866       366.6 ns/op     160 B/op       3 allocs/op

[moby#3869]: https://github.com/moby/moby/issues/3869

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
---
 pkg/stringid/stringid.go      | 29 +++++++++++++++++++++++------
 pkg/stringid/stringid_test.go | 30 ++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/pkg/stringid/stringid.go b/pkg/stringid/stringid.go
index 6b0c29cbd5..a79c96728e 100644
--- a/pkg/stringid/stringid.go
+++ b/pkg/stringid/stringid.go
@@ -4,7 +4,6 @@ package stringid // import "github.com/docker/docker/pkg/stringid"
 import (
 	"crypto/rand"
 	"encoding/hex"
-	"strconv"
 	"strings"
 )
 
@@ -27,7 +26,10 @@ func TruncateID(id string) string {
 	return id
 }
 
-// GenerateRandomID returns a unique id.
+// GenerateRandomID returns a unique, 64-character ID consisting of a-z, 0-9.
+// It guarantees that the ID, when truncated ([TruncateID]) does not consist
+// of numbers only, so that the truncated ID can be used as hostname for
+// containers.
 func GenerateRandomID() string {
 	b := make([]byte, 32)
 	for {
@@ -35,12 +37,27 @@ func GenerateRandomID() string {
 			panic(err) // This shouldn't happen
 		}
 		id := hex.EncodeToString(b)
-		// if we try to parse the truncated for as an int and we don't have
-		// an error then the value is all numeric and causes issues when
-		// used as a hostname. ref #3869
-		if _, err := strconv.ParseInt(TruncateID(id), 10, 64); err == nil {
+
+		// make sure that the truncated ID does not consist of only numeric
+		// characters, as it's used as default hostname for containers.
+		//
+		// See:
+		// - https://github.com/moby/moby/issues/3869
+		// - https://bugzilla.redhat.com/show_bug.cgi?id=1059122
+		if allNum(id[:shortLen]) {
+			// all numbers; try again
 			continue
 		}
 		return id
 	}
 }
+
+// allNum checks whether id consists of only numbers (0-9).
+func allNum(id string) bool {
+	for _, c := range []byte(id) {
+		if c > '9' || c < '0' {
+			return false
+		}
+	}
+	return true
+}
diff --git a/pkg/stringid/stringid_test.go b/pkg/stringid/stringid_test.go
index 454e59476c..a0bbbfa82c 100644
--- a/pkg/stringid/stringid_test.go
+++ b/pkg/stringid/stringid_test.go
@@ -43,3 +43,33 @@ func TestShortenIdInvalid(t *testing.T) {
 		t.Fatalf("Id returned is incorrect: truncate on %s returned %s", id, truncID)
 	}
 }
+
+func TestAllNum(t *testing.T) {
+	tests := []struct {
+		doc, id  string
+		expected bool
+	}{
+		{
+			doc:      "mixed letters and numbers",
+			id:       "4e38e38c8ce0",
+			expected: false,
+		},
+		{
+			doc:      "letters only",
+			id:       "deadbeefcafe",
+			expected: false,
+		},
+		{
+			doc:      "numbers only",
+			id:       "012345678912",
+			expected: true,
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.doc, func(t *testing.T) {
+			if actual := allNum(tc.id); actual != tc.expected {
+				t.Errorf("expected %q to be %t, got %t, ", tc.id, !tc.expected, actual)
+			}
+		})
+	}
+}