Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
xiemaisi opened this issue Jun 7, 2019 · 4 comments
Closed

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

xiemaisi opened this issue Jun 7, 2019 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@xiemaisi
Copy link

xiemaisi commented Jun 7, 2019

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++.
@gopherbot gopherbot added this to the Unreleased milestone Jun 7, 2019
@ALTree
Copy link
Member

ALTree commented Jun 7, 2019

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
Copy link
Author

xiemaisi commented Jun 7, 2019

Oh, excellent point.

@gopherbot
Copy link

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

@mdempsky
Copy link
Member

mdempsky commented Jun 7, 2019

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).

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 10, 2019
@golang golang locked and limited conversation to collaborators Mar 2, 2021
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
@ianlancetaylor
openpgp/packet: ensure that first partial packet is 512 bytes
8d87c18 
This requirement is from RFC 4880 4.2.2.4.

Also simplify the partialLengthWriter loop. The old code worked but
was written in a confusing way, with a loop whose terminating condition
didn't make sense and was never true in practice.
Rewrite it to more clearly do a set of partial writes of decreasing size.

Fixes golang/go#32474

Change-Id: Ia53ceb39a34f1d6f2ea7c60190d52948bb0db59b
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/181121
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@ianlancetaylor
openpgp/packet: ensure that first partial packet is 512 bytes
8554c5d 
This requirement is from RFC 4880 4.2.2.4.

Also simplify the partialLengthWriter loop. The old code worked but
was written in a confusing way, with a loop whose terminating condition
didn't make sense and was never true in practice.
Rewrite it to more clearly do a set of partial writes of decreasing size.

Fixes golang/go#32474

Change-Id: Ia53ceb39a34f1d6f2ea7c60190d52948bb0db59b
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/181121
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@ianlancetaylor
openpgp/packet: ensure that first partial packet is 512 bytes
4554dda 
This requirement is from RFC 4880 4.2.2.4.

Also simplify the partialLengthWriter loop. The old code worked but
was written in a confusing way, with a loop whose terminating condition
didn't make sense and was never true in practice.
Rewrite it to more clearly do a set of partial writes of decreasing size.

Fixes golang/go#32474

Change-Id: Ia53ceb39a34f1d6f2ea7c60190d52948bb0db59b
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/181121
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
@ianlancetaylor
openpgp/packet: ensure that first partial packet is 512 bytes
a2fa4bc 
This requirement is from RFC 4880 4.2.2.4.

Also simplify the partialLengthWriter loop. The old code worked but
was written in a confusing way, with a loop whose terminating condition
didn't make sense and was never true in practice.
Rewrite it to more clearly do a set of partial writes of decreasing size.

Fixes golang/go#32474

Change-Id: Ia53ceb39a34f1d6f2ea7c60190d52948bb0db59b
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/181121
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
@ianlancetaylor
openpgp/packet: ensure that first partial packet is 512 bytes
a5bf3fd 
This requirement is from RFC 4880 4.2.2.4.

Also simplify the partialLengthWriter loop. The old code worked but
was written in a confusing way, with a loop whose terminating condition
didn't make sense and was never true in practice.
Rewrite it to more clearly do a set of partial writes of decreasing size.

Fixes golang/go#32474

Change-Id: Ia53ceb39a34f1d6f2ea7c60190d52948bb0db59b
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/181121
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@mdempsky @xiemaisi @dmitshur @ALTree @gopherbot