x/net/http2: suspicious uint32→int conversion in http2 frame implementation

[sivukhin]

Go version

analyzed net library from commit: f12db26b1c9293fa3eb95c936e548d2c1fba4ba9

What operating system and processor architecture are you using (go env)?

Analyzed source code with linter - so the env is irrelevant.

What did you do?

I ran custom govanish linter against golang/net GitHub repo and it found that following statements were removed from compiled binary:

2024/01/04 22:56:02 it seems like your code vanished from compiled binary: func=[maxHeaderStringLen], file=[/home/sivukhin/code/go-net/http2/frame.go], lines=[1519-1519], snippet:
	return 0

func (fr *Framer) maxHeaderStringLen() int {                             // 1512
	v := fr.maxHeaderListSize()                                      // 1513
	if uint32(int(v)) == v {                                         // 1514
		return int(v)                                            // 1515
	}                                                                // 1516
	// They had a crazy big number for MaxHeaderBytes anyway,        // 1517
	// so give them unlimited header lengths:                        // 1518
	return 0                                                         // 1519 <- this return is not found in attributed assembly
}                                                                        // 1520

This is ok because my machine is 64 bit, but when I inspected this code I found it weird because conversion of types uint32 -> int32 -> uint32 on 32bit architectures should results in noop because there is no loss of precision.

I implemented explicit conversion funcs with int32 / int64 types to prove this statement:

func ConvertInt32(v uint32) int32 {
	if uint32(int32(v)) != v {
		return 0
	}
	return int32(v)
}

func ConvertInt64(v uint32) int64 {
	if uint32(int64(v)) != v {
		return 0
	}
	return int64(v)
}

func TestConvert(t *testing.T) {
	v := uint32(math.MaxUint32)
	t.Log(v)
	t.Log(ConvertInt32(v))
	t.Log(ConvertInt64(v))
}
/*
=== RUN   TestName
    ast_test.go:90: 4294967295
    ast_test.go:91: -1
    ast_test.go:92: 4294967295
--- PASS: TestName (0.00s)
PASS
*/

Godbolt link with compiled functions against x86 gc (tip) target: https://godbolt.org/z/688bf4jEs

I suspect that this part of the code will produce negative MaxStringLen header. Not sure if this will results in any bad consequences but I think it's better to fix this issue and implement more straightforward check.

What did you expect to see?

Conversion from uint32 to int should always produce non-negative numbers

What did you see instead?

Conversion from uint32 to int can produce negative numbers

[bcmills]

(attn @neild)

[bcmills]

@sivukhin, want to send a fix?
(https://go.dev/doc/contribute)

At the very least, this code ought to be written more clearly.

[sivukhin]

Yes, sure, will prepare patch tomorrow.

[sivukhin]

@bcmills, done: https://go-review.googlesource.com/c/net/+/554235

Also, what do you think about adding helpers in stdlib which will simplify integer type conversions to native int/uint?
I saw similar issues in other repositories (for example here: https://github.com/sajari/docconv/blob/master/snappy/decode.go#L33). It will be great to have more explicit and less error-prone way to perform such conversion through API like func TryConvertUint32ToInt(value uint32) (int, bool) { ... }

[gopherbot]

Change https://go.dev/cl/554275 mentions this issue: x/net: replace suspicious uint32 -> v conversion in http2 frame code with more robust approach

[gopherbot]

Change https://go.dev/cl/554235 mentions this issue: x/net: replace suspicious uint32 -> v conversion in http2 frame code with more robust approach