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

[isbang]

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]

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

[bcmills]

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]

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]

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]

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]

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]

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]

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]

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

Done.