#2017 fix: Check if address is nil for IsValid() function.

[cty123]

#2017 Panic due to nil dereferencing

While I was investigating #2017, I saw the error log reported by @base510

2023/04/30 20:00:38 [Error] common/net: invalid IP format: []
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x982078]

goroutine 92630 [running]:
github.com/xtls/xray-core/app/proxyman/outbound.(*Handler).getUoTConnection(0xc0001d5ce0, {0x13eeb40, 0xc0008e5290}, {{0x0?, 0x0?}, 0x7f?, 0x0?})
github.com/xtls/xray-core/app/proxyman/outbound/uot.go:14 +0x58
github.com/xtls/xray-core/app/proxyman/outbound.(*Handler).Dial(0xc0001d5ce0, {0x13eeb40?, 0xc0008e5290?}, {{0x0, 0x0}, 0xb, 0x2})
github.com/xtls/xray-core/app/proxyman/outbound/handler.go:214 +0xa26
github.com/xtls/xray-core/proxy/freedom.(*Handler).Process.func1()
github.com/xtls/xray-core/proxy/freedom/freedom.go:140 +0x2c6
github.com/xtls/xray-core/common/retry.(*retryer).On(0xc00125dd08, 0xc00125dd60)
github.com/xtls/xray-core/common/retry/retry.go:27 +0xdb
github.com/xtls/xray-core/proxy/freedom.(*Handler).Process(0xc00021bce0, {0x13eeb40, 0xc0008e5290}, 0xc0008e2460, {0x13e6748, 0xc0001d5ce0})
github.com/xtls/xray-core/proxy/freedom/freedom.go:126 +0x4b0
github.com/xtls/xray-core/app/proxyman/outbound.(*Handler).Dispatch(0xc000c07920?, {0x13eeb40, 0xc0008e5290}, 0xc0008e2460)
github.com/xtls/xray-core/app/proxyman/outbound/handler.go:147 +0x211
github.com/xtls/xray-core/app/dispatcher.(*DefaultDispatcher).routedDispatch(0xc0001c7f80, {0x13eeb40, 0xc0008e5290}, 0xc0008e2460, {{0x0, 0x0}, 0xb, 0x2})
github.com/xtls/xray-core/app/dispatcher/default.go:492 +0xbfe
created by github.com/xtls/xray-core/app/dispatcher.(*DefaultDispatcher).Dispatch
github.com/xtls/xray-core/app/dispatcher/default.go:302 +0x4ea

The error message right before the segfault suggests the root cause of the application panic. What happened under the hood was that, getUoTConnection() has a de-referencing like dest.Address.Family(), while dest.Address could be nil.

func (h *Handler) getUoTConnection(ctx context.Context, dest net.Destination) (stat.Connection, error) {
	if !dest.Address.Family().IsDomain() {
		return nil, os.ErrInvalid
	}
	...
}

After some code searching, I noticed the real problem wasn't the function itself, but rather the outbound handler.

func (h *Handler) Process(ctx context.Context, link *transport.Link, dialer internet.Dialer) error {
	outbound := session.OutboundFromContext(ctx)
	if outbound == nil || !outbound.Target.IsValid() {
		return newError("target not specified.")
	}
       ...

Before proceeding with the traffic handling logic, the code first checks if the outbound target(which is the destination address, net.Destination) is valid, which does make sense. However, I believe the IsValid() function is broken, because it's possible that the Address field could be nil. See following code.

type Destination struct {
	Address Address
	Port    Port
	Network Network
}
...
func DestinationFromAddr(addr net.Addr) Destination {
	switch addr := addr.(type) {
	case *net.TCPAddr:
		return TCPDestination(IPAddress(addr.IP), Port(addr.Port))
       ...
}
...
func IPAddress(ip []byte) Address {
	switch len(ip) {
	case net.IPv4len:
       ...
	default:
		newError("invalid IP format: ", ip).AtError().WriteToLog()
		return nil
	}
}

Also from semantic perspective, for the destination of the proxy request, a valid destination should always include address, port and network type. In this case, Port is never nil, so I updated the function to also include address check.

[yuhan6665]

感谢详细描述

[base510]

@cty123 编译修改后的版本测试结果

panic: Dispatcher: Invalid destination.

goroutine 47593 [running]:
github.com/xtls/xray-core/app/dispatcher.(*DefaultDispatcher).Dispatch(0xc0002203c0, {0x13bf960, 0xc0002264b0}, {{0x0, 0x0}, 0xb, 0x2})
github.com/xtls/xray-core/app/dispatcher/default.go:287 +0x519
github.com/xtls/xray-core/common/mux.(*Server).Dispatch(0x0?, {0x13bf960?, 0xc0002264b0?}, {{0x0?, 0x0?}, 0x11?, 0x0?})
github.com/xtls/xray-core/common/mux/server.go:41 +0xcf
github.com/xtls/xray-core/proxy/shadowsocks_2022.(*MultiUserInbound).NewConnection(0xc0001c5500, {0x13bf960, 0xc0002263f0}, {0x13c5358, 0xc000224070}, {{0x120ab67, 0xb}, {{{0x0, 0xffff59745902}, 0xc000012120}, ...}, ...})
github.com/xtls/xray-core/proxy/shadowsocks_2022/inbound_multi.go:207 +0x3ef
github.com/sagernet/sing-shadowsocks/shadowaead_2022.(*MultiService[...]).newConnection(0x13cd6e0, {0x13bf960, 0xc000226210}, {0x13c6278, 0xc000014010}, {{0x120ab67, 0xb}, {{{0x0, 0xffff59745902}, 0xc000012120}, ...}, ...})
github.com/sagernet/sing-shadowsocks@v0.2.2/shadowaead_2022/service_multi.go:245 +0x11d3
github.com/sagernet/sing-shadowsocks/shadowaead_2022.(*MultiService[...]).NewConnection(0xc000ffb860?, {0x13bf960?, 0xc000226210?}, {0x13c6278?, 0xc000014010?}, {{0x0, 0x0}, {{{0x0, 0xffff59745902}, 0xc000012120}, ...}, ...})
github.com/sagernet/sing-shadowsocks@v0.2.2/shadowaead_2022/service_multi.go:118 +0x89
github.com/xtls/xray-core/proxy/shadowsocks_2022.(*MultiUserInbound).Process(0xc0001c5500, {0x13bf960, 0xc0002261e0}, 0x2, {0x13c6220?, 0xc000014010}, {0x13c1c00?, 0xc000209560})
github.com/xtls/xray-core/proxy/shadowsocks_2022/inbound_multi.go:167 +0x305
github.com/xtls/xray-core/app/proxyman/inbound.(*tcpWorker).callback(0xc000193c20, {0x13c6220, 0xc000014010})
github.com/xtls/xray-core/app/proxyman/inbound/worker.go:107 +0x6d7
created by github.com/xtls/xray-core/app/proxyman/inbound.(*tcpWorker).Start.func1
github.com/xtls/xray-core/app/proxyman/inbound/worker.go:121 +0x98