search

FactomProject / factomd

Factom Daemon
https://www.factomprotocol.org/
Other
201 stars 92 forks source link

p2p/peer.go/Peer.LocationFromAddress() panics when Peer.Address contains an interface name as returned by Linux 4.14.36 #463

Open AdamSLevy opened 6 years ago

AdamSLevy commented 6 years ago

When Peer.Address contains an address like "fe80::f6:ecff:fe05:5ffc%eth0", Peer.LocationFromAddress panics due to index out of range.

The full panic is:

panic: runtime error: index out of range
goroutine 52 [running]:
github.com/FactomProject/factomd/p2p.(*Peer).LocationFromAddress(0xc4200b9a00, 0x15)
        /build/factomd/src/src/github.com/FactomProject/factomd/p2p/peer.go:141 +0x489
github.com/FactomProject/factomd/p2p.(*Peer).Init(0xc4200b9a00, 0xc420130c9b, 0x15, 0xc420130cb1, 0x4, 0x100000000, 0x0, 0x0)
        /build/factomd/src/src/github.com/FactomProject/factomd/p2p/peer.go:65 +0x333
github.com/FactomProject/factomd/p2p.(*Controller).parseSpecialPeers(0xc4202046e0, 0xc420130c80, 0x35, 0x1, 0x44bea8, 0xab72a411278d, 0x800000000c321a26)
        /build/factomd/src/src/github.com/FactomProject/factomd/p2p/controller.go:386 +0x2b6
github.com/FactomProject/factomd/p2p.(*Controller).initSpecialPeers(0xc4202046e0, 0xcbbd22, 0x6, 0xc4201a6130, 0x4, 0xc42001cf30, 0x2b, 0x100feedbeef, 0xc420238150, 0x60, ...)
        /build/factomd/src/src/github.com/FactomProject/factomd/p2p/controller.go:363 +0x78
github.com/FactomProject/factomd/p2p.(*Controller).Init(0xc4202046e0, 0xcbbd22, 0x6, 0xc4201a6130, 0x4, 0xc42001cf30, 0x2b, 0x100feedbeef, 0xc420238150, 0x60, ...)
        /build/factomd/src/src/github.com/FactomProject/factomd/p2p/controller.go:190 +0x819
github.com/FactomProject/factomd/engine.NetStart(0xc4200f0000, 0x12d09c0, 0x0)
        /build/factomd/src/src/github.com/FactomProject/factomd/engine/NetStart.go:378 +0x3f3f
created by github.com/FactomProject/factomd/engine.Factomd
        /build/factomd/src/src/github.com/FactomProject/factomd/engine/factomd.go:60 +0x34f

This can occur on Linux with an interface with an ipv6 address when a user specifies their own hostname as a special peer. In this situation net.LookupHost(address) in Peer.Init() on line 51 of p2p/peer.go returns an array of both the ipv6 and the ipv4 address as this custom debug output shows:

time="2018-05-02T16:54:29-08:00" level=debug msg="address: node2.entrycredits.io" address=node2.entrycredits.io component=networking package=p2p peerType=1 port=8108 subpack=peer
time="2018-05-02T16:54:29-08:00" level=debug msg="net.LookupHost(p.Address) returned ipAddress: [fe80::f6:ecff:fe05:5ffc%eth0 10.0.0.210] err: <nil>" address=node2.entrycredits.io component=networking package=p2p peerType=1 port=8108 subpack=peer

As you can see, the resolver for Linux is returning an ipv6 address with the interface appended: fe80::f6:ecff:fe05:5ffc%eth0.

Next, on line 65 Peer.LocationFromAddress() is called and this again calls net.LookupHost(p.Address). But since this relies on the same Linux resolver, this does not return an error and just returns the same IP address it was given, still including the appended %eth0. Now when net.ParseIP(p.Address) is called again, it returns nil because it does not handle the appended interface name. Again, this custom debug output shows this:

time="2018-05-02T16:54:29-08:00" level=debug msg="p.Address: fe80::f6:ecff:fe05:5ffc%eth0" address=node2.entrycredits.io component=networking package=p2p peerType=1 port=8108 subpack=peer
time="2018-05-02T16:54:29-08:00" level=debug msg="net.LookupHost(p.Address) returned ipAddress: [fe80::f6:ecff:fe05:5ffc%eth0] err: <nil>" address=node2.entrycredits.io component=networking package=p2p peerType=1 port=8108 subpack=peer
time="2018-05-02T16:54:29-08:00" level=debug msg="net.ParseIP(p.Address) returned ip: <nil>" address=node2.entrycredits.io component=networking package=p2p peerType=1 port=8108 subpack=peer

This is not checked for, and we proceed to deference the nil pointer on line 141, causing the panic.

Solutions

The simplest thing to do here is just check if ip == nil after line 135 and return 0 if so.

More niche things could be done to handle this like stripping out everything after % from addresses, but this is an edge case that doesn't matter since we don't need to peer with ourselves anyway.

stackdump commented 4 years ago

we added logging of all recover() calls - so if this is happening - I'll get that PR merged