Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

v0.9.0 panics on OpenBSD #8211

Closed
qbit opened this issue Jun 23, 2021 · 5 comments · Fixed by multiformats/go-multiaddr#155 or #8215
Closed

v0.9.0 panics on OpenBSD #8211

qbit opened this issue Jun 23, 2021 · 5 comments · Fixed by multiformats/go-multiaddr#155 or #8215
Labels
kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization

Comments

@qbit
Copy link

qbit commented Jun 23, 2021

Version information:

go-ipfs version: 0.9.0
Repo version: 11
System version: amd64/openbsd
Golang version: go1.16.5

Description:

After running for a period of time, ipfs daemon panics with the following:

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

goroutine 270 [running]:
github.com/multiformats/go-multiaddr/net.(*maListener).Accept(0xc0006b49a0, 0xc00287de18, 0xb23a5d, 0xc000019980, 0xc008136a40)
        /build/pobj/go-ipfs-0.9.0/go/pkg/mod/github.com/multiformats/go-multiaddr@v0.3.2/net/net.go:259 +0x112
github.com/libp2p/go-tcp-transport.(*tcpListener).Accept(0xc0007af950, 0xc00287de70, 0xc00287de78, 0x20, 0xc0007d8a80)
        /build/pobj/go-ipfs-0.9.0/go/pkg/mod/github.com/libp2p/go-tcp-transport@v0.2.2/tcp.go:66 +0x37
github.com/libp2p/go-tcp-transport.(*tracingListener).Accept(0xc0001636f0, 0x0, 0x0, 0xc001438fc0, 0x1adbf60)
        /build/pobj/go-ipfs-0.9.0/go/pkg/mod/github.com/libp2p/go-tcp-transport@v0.2.2/metrics.go:226 +0x37
github.com/libp2p/go-libp2p-transport-upgrader.(*listener).handleIncoming(0xc001438fc0)
        /build/pobj/go-ipfs-0.9.0/go/pkg/mod/github.com/libp2p/go-libp2p-transport-upgrader@v0.4.2/listener.go:77 +0xf7
created by github.com/libp2p/go-libp2p-transport-upgrader.(*Upgrader).UpgradeListener
        /build/pobj/go-ipfs-0.9.0/go/pkg/mod/github.com/libp2p/go-libp2p-transport-upgrader@v0.4.2/upgrader.go:49 +0x18d

The daemon also produces a plethora of this error while running:

2021-06-23T06:01:16.574-0600    ERROR   tcp-tpt go-tcp-transport@v0.2.2/tcp.go:45       Failed set keepalive period: set tcp4 REDACTED:1777->REDACTED:30504: protocol not available
@qbit qbit added kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization labels Jun 23, 2021
@qbit
Copy link
Author

qbit commented Jun 23, 2021

The error from the keepalive bit seems to be from https://github.com/golang/go/blob/2ebe77a2fda1ee9ff6fd9a3e08933ad1ebaea039/src/net/tcpsockopt_openbsd.go#L13

So that might not be the issue.

@marten-seemann
Copy link
Member

Agreed, this is definitely not related to the socket option.

The problem is in https://github.com/multiformats/go-multiaddr/blob/2f4fae4104af2df7036afcdd93d2075845e68ed0/net/net.go#L234-L267.
It looks like the net.Listener returns a net.Conn where the LocalAddr() is nil. It looks like the problem was introduced in multiformats/go-multiaddr#135, which was merged after the v0.3.1 release, so it makes sense that this only occurs now.

Now I'm not sure why a net.Conn would return nil in LocalAddr(), I would have expected it to return a non-nil value in every case. The easy fix is to guard against unexpected nil values, if we can't come up with a reasonable explanation here.

@qbit
Copy link
Author

qbit commented Jun 23, 2021

I figure this is more of a question for this thread:

Given libp2p/go-tcp-transport#80 (comment)

I guess OpenBSD users then have to live with a slightly suboptimal behavior then

It looks like go-tcp-transport just sets the keepalive to 30 seconds. I maintain the port of go-ipfs on OpenBSD and we have a doc that makes a number of tuning recommendations - Would recommending that users set their keepalive to 30 seconds be worth mentioning?

@marten-seemann
Copy link
Member

We probably should keep this open until the change is bubbled up here.

@marten-seemann
Copy link
Member

marten-seemann commented Jun 24, 2021

@qbit depending on the default value that OpenBSD uses, it might or might not be worth it.

@aschmahmann aschmahmann mentioned this issue Jul 16, 2021
18 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization
Projects
None yet
2 participants