x/crypto/openpgp: Strange loop in packet/packet.go

[xiemaisi]

What version of Go are you using (go version)?

$ go version
go version go1.12.5 linux/amd64

Does this issue reproduce with the latest release?

N/A, this issue was found by a static analysis tool.

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

Probably not important.

What did you do?

I noticed this strange-looking loop:

for power := uint(14); power < 32; power-- {
  // ...
}

Note that the loop variable power starts from 14 and is decremented in every iteration. The loop condition power < 32, on the other hand, bounds the variable upwards.

What did you expect?

If the present behaviour of counting power down from 14 to 0 and then terminating is correct, the loop condition should be replaced with power >= 0 to clarify this intention.

If the intention is to count from 14 up to 31, then power-- should be power++.

[ALTree]

Thanks for the report.

the loop condition should be replaced with power >= 0 to clarify this intention.

power >= 0 is always true since power is unsigned, so this wouldn't be correct. power-- does not make power negative when it's zero, it makes it wrap around to 4294967295.

[xiemaisi]

Oh, excellent point.

[gopherbot]

Change https://golang.org/cl/181121 mentions this issue: openpgp/packet: simplify partialLengthWriter loop

[mdempsky]

While people are looking at this code, I see RFC 4880 4.2.2.4 says "The first partial length MUST be at least 512 octets long." However, I don't see any code in openpgp/packet to enforce this.

In particular, it appears that both SerializeCompressed and SerializeLiteral will violate this: they both create a partialLengthWriter via serializeStreamHeader, and then they write a 1- or 2-byte slice, which I think will immediately get written as a 1- or 2-octet partial (i.e., less than 512).