encoding: fix UnsafeConvertStringToBytes to work with large input strings

[ecwall]

Fixes #111626

The previous impl assumed input string length <= math.MaxInt32. Go 1.20 added unsafe.StringData (https://pkg.go.dev/unsafe#StringData) which properly handles longer strings. This changes the impl to use unsafe.StringData and adds a unit test.

Release note (bug fix): Fixed a panic that could occur if a query uses a string
larger than 2^31-1 bytes.

[cockroach-teamcity]

This change is 

[cucaroach]

If the go docs are to be believed I think we need to leave in the zero length check. https://pkg.go.dev/unsafe#StringData

[ecwall]

Ok, added this back.

[cucaroach]

Thanks for the quick fix!

[cucaroach]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ecwall and @j82w)

---

pkg/util/encoding/encoding.go line 999 at r2 (raw file):

	// Next we treat the string data as a maximally sized array which we
	// slice. This usage is safe because the pointer value remains in the string.
	arg := (*[0x7fffffff]byte)(unsafe.Pointer(hdr.Data))[:len(s):len(s)]

We should fix this one too

[ecwall]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ecwall and @j82w)

pkg/util/encoding/encoding.go line 999 at r2 (raw file):

	// Next we treat the string data as a maximally sized array which we
	// slice. This usage is safe because the pointer value remains in the string.
	arg := (*[0x7fffffff]byte)(unsafe.Pointer(hdr.Data))[:len(s):len(s)]

We should fix this one too

Good catch.

[j82w]

pkg/util/encoding/encoding_test.go line 2648 at r3 (raw file):

		t.Run(tc.desc, func(t *testing.T) {
			actual := UnsafeConvertStringToBytes(tc.input)
			require.Equal(t, tc.expected, actual)

I think the test might be failing because the t is overloaded, and it's using the outer t from the original test call. I would suggest renaming it to avoid the ambiguity to make sure it use the t from the function.

[ecwall]

pkg/util/encoding/encoding_test.go line 2648 at r3 (raw file):

		t.Run(tc.desc, func(t *testing.T) {
			actual := UnsafeConvertStringToBytes(tc.input)
			require.Equal(t, tc.expected, actual)

I think the test might be failing because the t is overloaded, and it's using the outer t from the original test call. I would suggest renaming it to avoid the ambiguity to make sure it use the t from the function.

The inner t shadows the outer one which is what we want because calling require.Equal(t, ...) with the outer one when inside a subtest will fail the test with a useless error message like

testing.go:1471: test executed panic(nil) or runtime.Goexit: subtest may have called FailNow on a parent test

I think the test needs to be skipped under stress because of the large input.

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach, @ecwall, and @j82w)

---

-- commits line 11 at r4:
nit: the public facing release notes should not reference an internal function name in our code base. can this be rephrased so something more high level? like "Fixed a panic that could occur if a query uses a string larger than 2^32 bytes."

[ecwall]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach, @j82w, and @rafiss)

---

-- commits line 11 at r4:

Previously, rafiss (Rafi Shamim) wrote…

nit: the public facing release notes should not reference an internal function name in our code base. can this be rephrased so something more high level? like "Fixed a panic that could occur if a query uses a string larger than 2^32 bytes."

The function name appears in the error output because of the panic so I thought it would make sense to reference it here so customers can identify if they had this problem. I don't have a strong opinion though so I will change it (also, it is 2^31-1 bytes).

[ecwall]

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)