net/http: Server panic if listener returns connection with RemoteAddr() == nil #23022

azavorotnii · 2017-12-07T01:16:30Z

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

1.9.2

Does this issue reproduce with the latest release?

Yes.

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

Any.

What did you do?

In very specific situation listener returns net.Conn object with "RemoteAddr" that is "nil".

	listener, err := net.ListenTCP("tcp", &net.TCPAddr{Port: 8080})
	if err != nil {
		log.Fatal(err)
	}
	
	server := &http.Server{}
	for {
		if err := server.Serve(listener); err != nil {
			log.Println(err)
		}
	}

What did you expect to see?

Server returns error from server.Serve() or ignore issue.

What did you see instead?

Panic:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x2fcaf4]

goroutine 207644 [running]:
net/http.(*conn).serve(0x15ee4120, 0x31fbbc0, 0x154af2c0)
        /usr/local/go/src/net/http/server.go:1691 +0x34
created by net/http.(*Server).Serve
        /usr/local/go/src/net/http/server.go:2720 +0x208

Related code lines:

func (c *conn) serve(ctx context.Context) {
	c.remoteAddr = c.rwc.RemoteAddr().String()
	...

Field "c.remoteAddr" is used only in couple places, so should not be harmful to do instead:

func (c *conn) serve(ctx context.Context) {
	if raddr := c.rwc.RemoteAddr(); raddr != nil {
		c.remoteAddr = raddr.String()
	}
	...

The text was updated successfully, but these errors were encountered:

bradfitz · 2017-12-07T01:19:14Z

In very specific situation

What situation?

If you're implementing your own net.Listener or net.Conn, you need to implement the full interface.

If a concrete implementation in the standard library is returning nil, how?

azavorotnii · 2017-12-07T01:27:41Z

By very specific situation I mean that we didn't find stable way to reproduce this situation. Our listener wraps "keep alive" logic:

type keepAliveListener struct {
	*net.TCPListener
}

func (l keepAliveListener) Accept() (_ net.Conn, rerr error) {
	c, err := l.TCPListener.AcceptTCP()
	if err != nil {
		return nil, err
	}
	defer func() {
		if err := c.Close(); err != nil && rerr == nil {
			rerr = err
		}
	}()

	f, err := c.File()
	if err != nil {
		return nil, err
	}
	defer func() {
		if err := f.Close(); err != nil && rerr == nil {
			rerr = err
		}
	}()

	fc, err := net.FileConn(f) //See https://github.com/golang/go/issues/7605 and https://github.com/golang/go/issues/5052
	if err != nil {
		return nil, err
	}
	defer func() {
		if rerr != nil {
			_ = fc.Close() //ignore, rerr already non nil
		}
	}()

	c2 := fc.(*net.TCPConn) // <---- in very rare cases it has everything set correctly but "raddr".

	if err := c2.SetKeepAlive(true); err != nil {
		return nil, err
	}
	if err := c2.SetKeepAlivePeriod(30*time.Second); err != nil {
		return nil, err
	}

	return c2, nil
}

bradfitz · 2017-12-07T02:04:07Z

c.File() dups the fd. Maybe that messes it up.

@ianlancetaylor?

ianlancetaylor · 2017-12-07T02:28:41Z

I don't know why the dup would cause the network address to be missing, but I also don't know why you are doing it at all.

It's not obvious to me that this is a net/http problem. Can you give us a small standalone test case that shows you getting a net.TCPConn with no address? Thanks.

azavorotnii · 2017-12-07T04:32:31Z

Unfortunately, I don't have test to reproduce this behavior, it happens rarely on running server.
I could narrow issue, FileConn() makes call to newFileFD:

func newFileFD(f *os.File) (*netFD, error) {
	s, err := dupSocket(f)
	if err != nil {
		return nil, err
	}
	family := syscall.AF_UNSPEC
	sotype, err := syscall.GetsockoptInt(s, syscall.SOL_SOCKET, syscall.SO_TYPE)
	if err != nil {
		poll.CloseFunc(s)
		return nil, os.NewSyscallError("getsockopt", err)
	}
	lsa, _ := syscall.Getsockname(s)
	rsa, _ := syscall.Getpeername(s) 
	...

where syscall.Getpeername(s) returns <nil>, transport endpoint is not connected, but original TCPConn have remote address before dup.

frostyplanet · 2017-12-19T17:55:39Z

I have the same issue, it seems it will be trigger by some request, which I encountered a couple of times but cannot reproduce the bug by myself.
The http server is listening unix socket.
The problem also exists in go 1.8.
Sometimes it will just happen once, sometimes it will be trigger massive crash on multiple nodes.

frostyplanet · 2017-12-19T18:01:18Z

Our listener wrapper, in order to implement graceful restart feature.

func Listen(proto, addr string) (*ListenerWrap, error) {
    var isUnix bool
    var l net.Listener
    var err error
    var addr_dup string
    if proto == "unix" {
        isUnix = true
        if _, err := os.Stat(addr); !os.IsNotExist(err) {
            os.Remove(addr)
        }
        addr_dup = addr + "_dup"
        if _, err := os.Stat(addr_dup); !os.IsNotExist(err) {
            os.Remove(addr_dup)
        }
    } else if proto == "tcp" {
        // XXX I do not know how to convert ipv4 to ipv6.
        // And net.Listen("tcp", addr) will only listen on ipv6.
        proto = "tcp4"
    }
    if proto == "unix" {
        l, err = net.Listen(proto, addr_dup)
        if err != nil {
            return nil, err
        }
        // Refer to https://github.com/golang/go/issues/11826
        // We don't want Close() delete the file,
        // use hard link to make a duplicated entry,
        // deleting "addr_dup" does not affect "addr"
        err = os.Link(addr_dup, addr)
    } else {
        l, err = net.Listen(proto, addr)
    }
    if err != nil {
        return nil, err
    }
    lw := &ListenerWrap{addr: addr, addrDup: addr_dup, isUnix: isUnix}
    lw.l = l
    return lw, nil
}

func (lw *ListenerWrap) Close() error {
    if lw.stopped {
        return nil
    }
    lw.stopped = true
    return lw.l.Close()
}

func (lw *ListenerWrap) Accept() (c net.Conn, err error) {
    if lw.stopped {
        return
    }
    c, err = lw.l.Accept()
    return
}

func (lw *ListenerWrap) Addr() net.Addr {
    return lw.l.Addr()
}

agnivade · 2018-08-10T05:21:31Z

@azavorotnii - Are you still experiencing this issue ? Have you had a chance to try with 1.11beta3 and see if it resolves the issue ?

yonderblue · 2018-09-18T23:13:02Z

~~@ianlancetaylor the dup is to be able to set all 3 of the values for keep alive on the conn using its descriptor, which according to this #7605 needed a dup?~~
Looks like the new Control func on https://golang.org/pkg/net/#ListenConfig to set any connection options are now the preferred way, which likely bypasses this issue.

gopherbot · 2018-10-07T07:17:30Z

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

dstiliadis · 2018-10-28T18:28:26Z

Just FYI: Hit the same problem with go 1.10.3. Very rare to happen, but it showed up in a machine exposed to the Internet. conn.RemoteAddr() returned nil. Don't have data to see what triggered it.

bradfitz added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Dec 7, 2017

bradfitz added this to the Unplanned milestone Dec 7, 2017

bradfitz added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Dec 7, 2017

frostyplanet mentioned this issue Dec 20, 2017

Prevent rare case crash in conn.serve when fetching remote address #23187

Closed

agnivade added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Sep 7, 2018

gopherbot closed this as completed Oct 7, 2018

dstiliadis mentioned this issue Oct 28, 2018

Fix crash when connection doesn't have a remote address aporeto-inc/trireme-lib#673

Merged

golang locked and limited conversation to collaborators Oct 28, 2019

gopherbot added the FrozenDueToAge label Oct 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net/http: Server panic if listener returns connection with RemoteAddr() == nil #23022

net/http: Server panic if listener returns connection with RemoteAddr() == nil #23022

azavorotnii commented Dec 7, 2017

bradfitz commented Dec 7, 2017

azavorotnii commented Dec 7, 2017

bradfitz commented Dec 7, 2017

ianlancetaylor commented Dec 7, 2017

azavorotnii commented Dec 7, 2017 •

edited

frostyplanet commented Dec 19, 2017 •

edited

frostyplanet commented Dec 19, 2017 •

edited

agnivade commented Aug 10, 2018

yonderblue commented Sep 18, 2018 •

edited

gopherbot commented Oct 7, 2018

dstiliadis commented Oct 28, 2018

net/http: Server panic if listener returns connection with RemoteAddr() == nil #23022

net/http: Server panic if listener returns connection with RemoteAddr() == nil #23022

Comments

azavorotnii commented Dec 7, 2017

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

bradfitz commented Dec 7, 2017

azavorotnii commented Dec 7, 2017

bradfitz commented Dec 7, 2017

ianlancetaylor commented Dec 7, 2017

azavorotnii commented Dec 7, 2017 • edited

frostyplanet commented Dec 19, 2017 • edited

frostyplanet commented Dec 19, 2017 • edited

agnivade commented Aug 10, 2018

yonderblue commented Sep 18, 2018 • edited

gopherbot commented Oct 7, 2018

dstiliadis commented Oct 28, 2018

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

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

azavorotnii commented Dec 7, 2017 •

edited

frostyplanet commented Dec 19, 2017 •

edited

frostyplanet commented Dec 19, 2017 •

edited

yonderblue commented Sep 18, 2018 •

edited