util/cmpver: only consider ascii numerals (#9741)

Fixes #9740

Signed-off-by: Paul Scott <[email protected]>

util/cmpver/version.go

@@ -22,15 +22,20 @@ import (
 	"fmt"
 	"strconv"
 	"strings"
-	"unicode"
 )
 
+func isnum(r rune) bool {
+	return r >= '0' && r <= '9'
+}
+
+func notnum(r rune) bool {
+	return !isnum(r)
+}
+
 // Compare returns an integer comparing two strings as version
 // numbers. The result will be 0 if v1==v2, -1 if v1 < v2, and +1 if
 // v1 > v2.
 func Compare(v1, v2 string) int {
-	notNumber := func(r rune) bool { return !unicode.IsNumber(r) }
-
 	var (
 		f1, f2 string
 		n1, n2 uint64
@@ -38,16 +43,16 @@ func Compare(v1, v2 string) int {
 	)
 	for v1 != "" || v2 != "" {
 		// Compare the non-numeric character run lexicographically.
-		f1, v1 = splitPrefixFunc(v1, notNumber)
-		f2, v2 = splitPrefixFunc(v2, notNumber)
+		f1, v1 = splitPrefixFunc(v1, notnum)
+		f2, v2 = splitPrefixFunc(v2, notnum)
 
 		if res := strings.Compare(f1, f2); res != 0 {
 			return res
 		}
 
 		// Compare the numeric character run numerically.
-		f1, v1 = splitPrefixFunc(v1, unicode.IsNumber)
-		f2, v2 = splitPrefixFunc(v2, unicode.IsNumber)
+		f1, v1 = splitPrefixFunc(v1, isnum)
+		f2, v2 = splitPrefixFunc(v2, isnum)
 
 		// ParseUint refuses to parse empty strings, which would only
 		// happen if we reached end-of-string. We follow the Debian

util/cmpver/version_test.go

@@ -1,9 +1,13 @@
 // Copyright (c) Tailscale Inc & AUTHORS
 // SPDX-License-Identifier: BSD-3-Clause
 
-package cmpver
+package cmpver_test
 
-import "testing"
+import (
+	"testing"
+
+	"tailscale.com/util/cmpver"
+)
 
 func TestCompare(t *testing.T) {
 	tests := []struct {
@@ -87,6 +91,16 @@ func TestCompare(t *testing.T) {
 			v2:   "0.96-105",
 			want: 1,
 		},
+		{
+			// Though ۱ and ۲ both satisfy unicode.IsNumber, our previous use
+			// of strconv.ParseUint with these characters would have lead us to
+			// panic. We're now only looking at ascii numbers, so test these are
+			// compared as text.
+			name: "only ascii numbers",
+			v1:   "۱۱", // 2x EXTENDED ARABIC-INDIC DIGIT ONE
+			v2:   "۲",  // 1x EXTENDED ARABIC-INDIC DIGIT TWO
+			want: -1,
+		},
 
 		// A few specific OS version tests below.
 		{
@@ -147,17 +161,17 @@ func TestCompare(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			got := Compare(test.v1, test.v2)
+			got := cmpver.Compare(test.v1, test.v2)
 			if got != test.want {
 				t.Errorf("Compare(%v, %v) = %v, want %v", test.v1, test.v2, got, test.want)
 			}
 			// Reversing the comparison should reverse the outcome.
-			got2 := Compare(test.v2, test.v1)
+			got2 := cmpver.Compare(test.v2, test.v1)
 			if got2 != -test.want {
 				t.Errorf("Compare(%v, %v) = %v, want %v", test.v2, test.v1, got2, -test.want)
 			}
 			// Check that version comparison does not allocate.
-			if n := testing.AllocsPerRun(100, func() { Compare(test.v1, test.v2) }); n > 0 {
+			if n := testing.AllocsPerRun(100, func() { cmpver.Compare(test.v1, test.v2) }); n > 0 {
 				t.Errorf("Compare(%v, %v) got %v allocs per run", test.v1, test.v2, n)
 			}
 		})