encoding/hex: fix runtime error index out of range in hex.Decode

[isbang]

this error occured when dst buffer is not enough in hex.Decode

fix #34982

[gopherbot]

This PR (HEAD: faba54f) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/201957 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

[gopherbot]

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/201957.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Gert Cuykens:

Patch Set 1:

You can maybe include a test in "/usr/local/go/test/fixedbugs"?

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/201957.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Gert Cuykens:

Patch Set 1:

Also maybe compare with the other encodings to be sure they all behave the same. For example, if the others panic too then maybe it's better to let this one panic too.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/201957.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from 일섭 방:

Patch Set 1:

This is my first contribute so I'm not sure how to do test in /usr/local/go/test/fixedbugs. Please let me know how to do.

Other public methods in encoding has length check or use index safe function like copy(). But base64:Encoding.Encode doesn't has dst length check, and binary:ByteOrder.PutUxxx has only bound check code like _ := buff[max]. Also in base64:varint.go#L40 there is comment "If the buffer is too small, PutUvarint will panic" on PutUvarint function.

Most of encoding package has dst length check and I strongly think those package should not occur unexpected panic.
So hex package need dst length check.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/201957.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Gert Cuykens:

Patch Set 1:

Am not a expert either XD but I guess adding a file issue34982.go and do a single function test in it will do.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/201957.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Tobias Klauser:

Patch Set 1:

Patch Set 1:

You can maybe include a test in "/usr/local/go/test/fixedbugs"?

$GOROOT/test contains tests of the Go tool chain and runtime, but this CL fixes an issue in the encoding/hex package. Thus I'd suggest to add a test to hex_test.go ($GOROOT/src/encoding/hex/hex_test.go) instead.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/201957.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Bryan C. Mills:

Patch Set 1: Code-Review-1

(2 comments)

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/201957.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from 일섭 방:

Patch Set 1:

Patch Set 1: Code-Review-1

(2 comments)

Thanks for your review.
Here is my reply for your comments.

Line 52: I think I should learn more english :) I just wanted to mention that 'if dst is not enough, src will not fully decoded'

Line 61: I found this from my api server panic log. My server use hex string token in header, and I had to parse using hex.Decode. Because token size was fixed 32bytes, I used array not slice. And my co-worker sent 33 bytes by mistake, server raised pannic. Of course I couldn't handle this panic because nothing returned.

I think this code should be changed because this panic cannot be handled. Even there is no explanation that this method can occur panic when dst space is not enough. But I agree with your comments that extra comparison needlessly slows the loop.

What do you think about checking len(dst) >= hex.DecodedLen(len(src)) and dst==nil before loop and return error that dst has not enough space size?

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/201957.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Bryan C. Mills:

Patch Set 1:

Patch Set 1:

Patch Set 1: Code-Review-1

(2 comments)

Thanks for your review.
Here is my reply for your comments.

Line 52: I think I should learn more english :) I just wanted to mention that 'if dst is not enough, src will not fully decoded'

Line 61: I found this from my api server panic log. My server use hex string token in header, and I had to parse using hex.Decode. Because token size was fixed 32bytes, I used array not slice. And my co-worker sent 33 bytes by mistake, server raised pannic. Of course I couldn't handle this panic because nothing returned.

The panic does not need to be handled: it can be avoided using a simple check on the caller side. You can prevent the panic from happening in the first place by checking that len(dst) is equal to hex.DecodedLen(len(src)).

I think this code should be changed because this panic cannot be handled. Even there is no explanation that this method can occur panic when dst space is not enough. But I agree with your comments that extra comparison needlessly slows the loop.

What do you think about checking len(dst) >= hex.DecodedLen(len(src)) and dst==nil before loop and return error that dst has not enough space size?

That still has the problem of suppressing bugs that should have been detected during testing and/or code review.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/201957.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

This PR is being closed because golang.org/cl/201957 has been abandoned.

Closing per discussion on linked issue.