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

encoding/hex: panic on hex.Decode with not enough space of dst #34982

Closed
isbang opened this issue Oct 18, 2019 · 9 comments
Closed

encoding/hex: panic on hex.Decode with not enough space of dst #34982

isbang opened this issue Oct 18, 2019 · 9 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@isbang
Copy link

isbang commented Oct 18, 2019

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

$ go version
go version go1.13 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/bang/.cache/go-build"
GOENV="/home/bang/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY="*.toysmythiot.com"
GONOSUMDB="*.toysmythiot.com"
GOOS="linux"
GOPATH="/home/bang/go"
GOPRIVATE="*.toysmythiot.com"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build950007809=/tmp/go-build -gno-record-gcc-switches"
GOROOT/bin/go version: go version go1.13 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.13
uname -sr: Linux 5.0.0-29-generic
Distributor ID:	Ubuntu
Description:	Ubuntu 18.04.3 LTS
Release:	18.04
Codename:	bionic
/lib/x86_64-linux-gnu/libc.so.6: GNU C Library (Ubuntu GLIBC 2.27-3ubuntu1) stable release version 2.27.
gdb --version: GNU gdb (Ubuntu 8.1-0ubuntu3.1) 8.1.0.20180409-git

What did you do?

I tried hex.Decode with not enough space of dst

https://play.golang.org/p/suJblfGs0EQ

What did you expect to see?

returning error

What did you see instead?

panic


I belive this panic occured because of hex.Decode does not check length or capacity of dst

In src/encoding/hex/hex.go#L58

func Decode(dst, src []byte) (int, error) {
	i, j := 0, 1
	for ; j < len(src); j += 2 {
		a, ok := fromHexChar(src[j-1])
		if !ok {
			return i, InvalidByteError(src[j-1])
		}
		b, ok := fromHexChar(src[j])
		if !ok {
			return i, InvalidByteError(src[j])
		}
		dst[i] = (a << 4) | b // I belive dst[i] is problem
		i++
	}

and I suggest change this code into

func Decode(dst, src []byte) (int, error) {
	i, j := 0, 1
-	for ; j < len(src); j += 2 {
+	for ; j < len(src) && i < len(dst); j += 2 {
		a, ok := fromHexChar(src[j-1])
		if !ok {
			return i, InvalidByteError(src[j-1])
		}
		b, ok := fromHexChar(src[j])
		if !ok {
			return i, InvalidByteError(src[j])
		}
		dst[i] = (a << 4) | b
		i++
	}
@gopherbot
Copy link

Change https://golang.org/cl/201957 mentions this issue: encoding/hex: fix runtime error index out of range in hex.Decode

@isbang isbang changed the title Panic on hex.Decode with not enough space of dst encoding/hex: panic on hex.Decode with not enough space of dst Oct 18, 2019
@bcmills
Copy link
Contributor

bcmills commented Oct 18, 2019

The documentation for Decode says:

Decode decodes src into DecodedLen(len(src)) bytes

Did you ensure that len(dst) is at least DecodedLen(len(src))? If not, this is arguably a caller error rather than a bug in Decode: Go functions generally do not check what should be compile-time invariants, provided that failure to satisfy those invariants will be detected in some other way (such as the panic here).

The error returned by Decode is presumably for other kinds of errors, such as a src text that contains non-hexadecimal characters.

@bcmills bcmills added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 18, 2019
@bcmills bcmills added this to the Go1.14 milestone Oct 18, 2019
@bcmills
Copy link
Contributor

bcmills commented Oct 18, 2019

As noted on the CL: I don't think this would be a net improvement, regardless of whether Decode is changed to return a non-nil error or 0, nil as in the current draft of the change.

If the lengths of dst and src are not already known, is easy for the caller to check whether len(dst) >= hex.DecodedLen(len(src)) before calling hex.Decode.

If the lengths are already known, then an extra run-time comparison potentially slows the loop for no benefit — and, more importantly, masks bugs that would otherwise be caught during testing (as panics) by treating them as equivalent to invalid input strings.

@isbang
Copy link
Author

isbang commented Oct 18, 2019

The documentation for Decode says:

Decode decodes src into DecodedLen(len(src)) bytes

Did you ensure that len(dst) is at least DecodedLen(len(src))? If not, this is arguably a caller error rather than a bug in Decode: Go functions generally do not check what should be compile-time invariants, provided that failure to satisfy those invariants will be detected in some other way (such as the panic here).

The error returned by Decode is presumably for other kinds of errors, such as a src text that contains non-hexadecimal characters.

Thanks for your comments.

Like you commented, Decode says

Decode decodes src into DecodedLen(len(src)) bytes

But does not say

Decode can panic if DecodedLen(len(src)) is over len(dst)

In case of sending and receiveing 32 length data of hex string in the server client environment, the server will prepare make([]byte, DecodedLen(32)). Here, if the client sends 34 length, intentional or unintentional, the server will PANIC.

And actually I agree with your comment

extra run-time comparison potentially slows the loop

But I steel think hex.Decode need len(dst) check code. (maybe check before loop and return error?)

I hope golang official library doesn't panic without saying.

@elagergren-spideroak
Copy link

elagergren-spideroak commented Oct 18, 2019

In case of sending and receiveing 32 length data of hex string in the server client environment, the server will prepare make([]byte, DecodedLen(32))

You're describing using the API as such:

src := bytes.Repeat([]byte("A"), 34)
dst := make([]byte, hex.DecodedLen(32))
hex.Decode(dst, src)

which, per the function's docs, is incorrect:

Decode decodes src into **DecodedLen(len(src))** bytes...

I do think the docs could be more clear that the function expects len(dst) == DecodedLen(len(src)), perhaps explicitly calling out that the function will fail spectacularly if it doesn't hold true.

@bcmills
Copy link
Contributor

bcmills commented Oct 18, 2019

does not say

Decode can panic if DecodedLen(len(src)) is over len(dst)

Go APIs generally do not (and should not!) document what happens if you call them with arguments that do not satisfy their documented preconditions.

A function that panics for a given input today could be changed to accept that input, return an explicit error for it, report a race in -race mode, fail a vet check at compile time, or any number of other things.

(It should strive to always do something reasonable in order to help the user diagnose the problem, but beyond that it should not be constrained in what it does. In that regard, a panic with a useful stacktrace is “something reasonable”.)

@isbang
Copy link
Author

isbang commented Oct 26, 2019

If there is no more comments, is it ok to close this issue? I found out why this issue happened and how to fix it. Thanks @bcmills

@isbang
Copy link
Author

isbang commented Oct 26, 2019

If there is no more comments, is it ok to close this issue? I found out why this issue happened and how to fix it. Thanks @bcmills

and also #34983

@bcmills
Copy link
Contributor

bcmills commented Oct 28, 2019

If there is no more comments, is it ok to close this issue?

Done.

@bcmills bcmills closed this as completed Oct 28, 2019
@golang golang locked and limited conversation to collaborators Oct 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants