go/ir: incorrect control flow due to compiler intrinsic stub

[FiloSottile]

In crypto code we often use

// sliceForAppend takes a slice and a requested number of bytes. It returns a
// slice with the contents of the given slice followed by that many bytes and a
// second slice that aliases into it and contains only the extra bytes. If the
// original slice has sufficient capacity then no allocation is performed.
func sliceForAppend(in []byte, n int) (head, tail []byte) {
	if total := len(in) + n; cap(in) >= total {
		head = in[:total]
	} else {
		head = make([]byte, total)
		copy(head, in)
	}
	tail = head[len(in):]
	return
}

to get a slice to operate on, and the value to return from an append-style API.

This causes SA4006 false positives, for example on the first line of

func bitPack18(buf []byte, r ringElement) []byte {
	out, v := sliceForAppend(buf, 18*n/8)
	const b = 1 << 17
	for i := 0; i < n; i += 4 {
		// b - [−2¹⁷+1, 2¹⁷] = [0, 2²⁸-1]
		w0 := b - fieldCenteredMod(r[i])
		v[0] = byte(w0 << 0)
		v[1] = byte(w0 >> 8)
		v[2] = byte(w0 >> 16)
		w1 := b - fieldCenteredMod(r[i+1])
		v[2] |= byte(w1 << 2)
		v[3] = byte(w1 >> 6)
		v[4] = byte(w1 >> 14)
		w2 := b - fieldCenteredMod(r[i+2])
		v[4] |= byte(w2 << 4)
		v[5] = byte(w2 >> 4)
		v[6] = byte(w2 >> 12)
		w3 := b - fieldCenteredMod(r[i+3])
		v[6] |= byte(w3 << 6)
		v[7] = byte(w3 >> 2)
		v[8] = byte(w3 >> 10)
		v = v[4*18/8:]
	}
	return out
}

SA4006 has definitely found real bugs for me, so I'm willing to tolerate a bit of false positives, but maybe sliceForAppend is an easy enough pattern to see through?

[dominikh]

I'm failing to reproduce this, both with master and the latest release. Can you provide a minimal example that compiles and shows the issue? I tried with https://play.golang.org/p/ZstojESUF0z but that wouldn't trigger SA4006.

[FiloSottile]

Sorry, I should have pointed to the CL. This should work in any environment.

$ go install golang.org/dl/gotip@latest
$ gotip download 717121
$ gotip run honnef.co/go/tools/cmd/staticcheck@latest crypto/internal/fips140/mldsa
sdk/gotip/src/crypto/internal/fips140/mldsa/cast.go:12:5: var fipsSelfTest is unused (U1000)
sdk/gotip/src/crypto/internal/fips140/mldsa/field.go:532:2: this value of v is never used (SA4006)
sdk/gotip/src/crypto/internal/fips140/mldsa/field.go:563:2: this value of v is never used (SA4006)
exit status 1

(The U1000 is a true positive.)

[dominikh]

This is actually unrelated to aliasing and closer to #787 and #807. However, in this case, it's not caused by build tags, but compiler magic. Your loop indirectly calls crypto/internal/constanttime.boolToUint8, which we see as an unconditional panic because it's implemented as

func boolToUint8(b bool) uint8 {
	panic("unreachable; must be intrinsicified")
}

This causes the loop to abort before any read of v occurs.

A quick(ish) solution would be for Staticcheck to special case that function and treat it as having no function body.

[FiloSottile]

Ah! Makes sense.

It is probably good to special-case it. It's unlikely to change and I doubt we'll end up with more shaped like it.