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

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

Closed
azavorotnii opened this issue Dec 7, 2017 · 11 comments

Comments

Projects
None yet
8 participants
@azavorotnii
Copy link

commented Dec 7, 2017

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()
	}
	...
@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 7, 2017

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?

@bradfitz bradfitz added this to the Unplanned milestone Dec 7, 2017

@azavorotnii

This comment has been minimized.

Copy link
Author

commented Dec 7, 2017

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

This comment has been minimized.

Copy link
Member

commented Dec 7, 2017

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

@ianlancetaylor?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2017

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

This comment has been minimized.

Copy link
Author

commented Dec 7, 2017

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

This comment has been minimized.

Copy link

commented Dec 19, 2017

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

This comment has been minimized.

Copy link

commented Dec 19, 2017

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

This comment has been minimized.

Copy link
Member

commented Aug 10, 2018

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

@gaillard

This comment has been minimized.

Copy link

commented Sep 18, 2018

@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

This comment has been minimized.

Copy link

commented Oct 7, 2018

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

This comment has been minimized.

Copy link

commented Oct 28, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.