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.

Sign up for GitHub

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/net/icmp: ListenPacket shouldn't panic for invalid network #32933

Closed
songjiayang opened this issue Jul 4, 2019 · 6 comments
Closed

x/net/icmp: ListenPacket shouldn't panic for invalid network #32933

songjiayang opened this issue Jul 4, 2019 · 6 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@songjiayang
Copy link

I write some test for x/net/icmp, there is my code:

package main

import (
	"log"

	"golang.org/x/net/icmp"
)

func main() {
	conn, err := icmp.ListenPacket("ip4", "0.0.0.0")
	if err != nil {
		log.Panic(err)
	}
}

When I run the code, get error message like:

panic: runtime error: slice bounds out of range

goroutine 1 [running]:
golang.org/x/net/icmp.ListenPacket(0x113c872, 0x3, 0x113cd87, 0x7, 0x0, 0xc0000ae381, 0x123aa60)
	/Users/songjiayang/go/pkg/mod/golang.org/x/net@v0.0.0-20190628185345-da137c7871d7/icmp/listen_posix.go:53 +0x872

I known the reason is because I use an invalid network, it should be ip4:1 rather than ip4. But I think it should raise a specific error rather than a panic runtime error.
@gopherbot gopherbot added this to the Unreleased milestone Jul 4, 2019
@bcmills
Copy link
Contributor

bcmills commented Jul 8, 2019

The bug is a missing range check here:
https://github.com/golang/net/blob/da137c7871d730100384dbcf36e6f8fa493aef5b/icmp/listen_posix.go#L52-L53

		i := last(network, ':')
		switch network[:i] {

@bcmills bcmills added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Jul 8, 2019
@bcmills bcmills changed the title x/net: ICMP listen shouldn't panic for invalid network x/net/icmp: ListenPacket shouldn't panic for invalid network Jul 8, 2019
@songjiayang
Copy link
Author

songjiayang commented Jul 9, 2019 •
edited

The bug is a missing range check here:
https://github.com/golang/net/blob/da137c7871d730100384dbcf36e6f8fa493aef5b/icmp/listen_posix.go#L52-L53

		i := last(network, ':')
		switch network[:i] {

Yes, I try to fix the last help function and make it return the length if not find the target character.

@gopherbot
Copy link

Change https://golang.org/cl/185317 mentions this issue: x/net/icmp: ListenPacket panic error.

@ivenk ivenk mentioned this issue Jul 24, 2019
Open
@ivenk
Copy link

ivenk commented Jul 24, 2019

Since it has been a couple of days, i took a swing at it.
So far i did the following :

  • changed the helper function last() to return -1 when no match is found. Similar to what strings.Index does.
  • Later i check for -1 and return a new error i created in message.go

Now instead of a panic an error is returned.
Let me know if i should change anything.

@songjiayang
Copy link
Author

HI @ivenk, thanks for your PR.

I have make a same PR for this bug, but it blocked at code review, you can check it with link https://golang.org/cl/185317

@ivenk
Copy link

ivenk commented Jul 25, 2019

Oh okay i didn't know about that. Will get fixed eventually then.

@golang golang locked and limited conversation to collaborators Nov 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@songjiayang @bcmills @gopherbot @ivenk