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

encoding: inconsistent panic behavior for {hex,base32,base64}.Decode #58391

Closed
dsnet opened this issue Feb 7, 2023 · 3 comments
Closed

encoding: inconsistent panic behavior for {hex,base32,base64}.Decode #58391

dsnet opened this issue Feb 7, 2023 · 3 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Go1.21

Comments

@dsnet
Copy link
Member

dsnet commented Feb 7, 2023 •
edited

CL 461958 introduced inconsistencies across two dimensions:

  • Encode still panics on output buffers too small, while Decode now treats such cases as an error. This inconsistency cannot be resolved since Encode does not report an error and could be an argument for why we should revert CL 461958.
  • The base32 and base64 packages share very similar API as hex, and their equivalent Decode functions still panic if the output buffer is too small. We should handle these cases in identical ways across packages.

Should we revert CL 461958 to maintain consistency between Encode and Decode?
Or should we roll forward with applying equivalent checks to base32 and base64?

I have a very minor preference for rolling back.

Related: the fact that Encode and Decode can panic is a sharp edge fixed by #53693.

\cc @bprosnitz @ianlancetaylor @bcmills
@bcmills
Copy link
Member

bcmills commented Feb 7, 2023 •
edited

I would be fine with reverting CL 461958, but I will defer to @ianlancetaylor if he has a preference.

@bcmills bcmills added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 7, 2023
@bcmills bcmills added this to the Go1.21 milestone Feb 7, 2023
@ianlancetaylor
Copy link
Contributor

The analogy to Encode does suggest rolling back. Sorry I didn't think of it.

@bprosnitz Any thoughts? Thanks.

@gopherbot
Copy link

Change https://go.dev/cl/469615 mentions this issue: Revert "hex: fix panic in Decode when len(src) > 2*len(dst)"

johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 22, 2023
@dsnet @johanbrandhorst
Revert "hex: fix panic in Decode when len(src) > 2*len(dst)"
3e8e7be 
This reverts CL 461958 and CL 465855.

Reason for revert: This introduced an irreconcilable inconsistency with Encode

Fixes golang#58391.

Change-Id: Ifd01a04d433b24c092b73e627b8149a5851c2bca
Reviewed-on: https://go-review.googlesource.com/c/go/+/469615
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
Auto-Submit: Joseph Tsai <joetsai@digital-static.net>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
4 participants
@ianlancetaylor @bcmills @dsnet @gopherbot