GODRIVER-3240 Code hardening.

[qingyang-hu]

GODRIVER-3240

Summary

• Use functions from the "encoding/binary" package to replace bit-shifting logic as a follow-up of GODRIVER-3240 Replace all bit-shifting in wiremessage with encoding/binary calls. #1646.
• Update size check for integer conversions.
• Add allocation size check.
• Convert atlas test to a gotest.

Background & Motivation

[mongodb-drivers-pr-bot]

API Change Report

No changes found!

[matthewdale]

We can use ParseUint to limit the range of the parsed subtype value by using bitSize=8. I also recommend adding the parse error to returned error.

E.g.

i, err := strconv.ParseUint(val.v.(string), 16, 8)
if err != nil {
	return nil, 0, fmt.Errorf("invalid $binary subType string %q: %w", val.v.(string), err)
}
subType = byte(i)

[matthewdale]

Since we already apply the bounds check in the ParseUint call, it should be safe to assume that i is in the range [0, 255]. In that case, we can keep the existing uint64 -> byte conversion.

Suggested change

	b := []byte{0, 0, 0, 0, 0, 0, 0, 0}
	binary.LittleEndian.PutUint64(b, i)
	subType = b[0]
	subType = byte(i)

[matthewdale]

We can simplify the logic here by adding some additional data size validation. AFAIK there's no case where an X.509 key and cert should be anywhere near 64MiB, so we can avoid the integer overflow and check for reasonable data size at the same time.

Suggested change

	dataSize := len(keyData)
	certSize := len(certData)
	if math.MaxInt-dataSize < certSize {
	return "", errors.New("size overflow")
	}
	dataSize += certSize
	if math.MaxInt-dataSize < 1 {
	return "", errors.New("size overflow")
	}
	dataSize++
	keySize := len(keyData)
	if keySize > 64*1024*1024 {
	return "", errors.New("X.509 key must be less than 64 MiB")
	}
	certSize := len(certData)
	if certSize > 64*1024*1024 {
	return "", errors.New("X.509 certificate must be less than 64 MiB")
	}
	data := make([]byte, 0, keySize+certSize+1)

[matthewdale]

Use a more accurate error message.

Suggested change

	return emptyValue, fmt.Errorf("%d overflows uint", i64)
	return emptyValue, fmt.Errorf("%d underflows uint", i64)

[qingyang-hu]

It seems better to keep the error message consistent with other unsigned types.

[matthewdale]

Sounds reasonable. We can fix them all at the same time. Consider this comment resolved.

[qingyang-hu]

An additional fail-safe check.

[blink1073]

The TestDefaultValueDecoders test failures are relevant.

[matthewdale]

Looks good! 👍