search

cretz / bine

Go library for accessing and embedding Tor clients and servers
MIT License
761 stars 71 forks source link

panic when closing tor while an onion service is being published #57

Open hkparker opened 2 years ago

hkparker commented 2 years ago

The context here is an application that publishes an onion service, but the user closes the app while everything is still being initialized. I want to close things down as gracefully as possible in that situation.

If tor.Start has already returned and we've got a reference to a *Tor we can Close() it, but if t.Listen has not returned yet, we don't have an *OnionService we can Close() first, even though we're in the process of publishing one. So we try to t.Close() while the onion service is still being published and occasionally we see:

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

goroutine 8 [running]:
github.com/cretz/bine/control.(*Conn).debugEnabled(...)
        /home/hkparker/Development/Go/pkg/mod/github.com/cretz/bine@v0.2.0/control/conn.go:91
github.com/cretz/bine/control.(*Conn).SendRequest(0x0, {0x1018d1c, 0xc00038b6c0}, {0xc00038b718, 0xc00038b728, 0x1})
        /home/hkparker/Development/Go/pkg/mod/github.com/cretz/bine@v0.2.0/control/conn.go:56 +0x5e
github.com/cretz/bine/control.(*Conn).sendRequestIgnoreResponse(...)
        /home/hkparker/Development/Go/pkg/mod/github.com/cretz/bine@v0.2.0/control/conn.go:47
github.com/cretz/bine/control.(*Conn).DelOnion(...)
        /home/hkparker/Development/Go/pkg/mod/github.com/cretz/bine@v0.2.0/control/cmd_onion.go:200
github.com/cretz/bine/tor.(*OnionService).Close(0xc00019e310)
        /home/hkparker/Development/Go/pkg/mod/github.com/cretz/bine@v0.2.0/tor/listen.go:325 +0x13b
github.com/cretz/bine/tor.(*Tor).Listen(0xc0003c6000, {0x114e930, 0xc00013c000}, 0xc00038bb78)
        /home/hkparker/Development/Go/pkg/mod/github.com/cretz/bine@v0.2.0/tor/listen.go:291 +0x141e
...

I tried using a cancelable context passed into t.Listen, and calling the cancel function before closing tor, but that hung for reasons unknown.

I think what's going on here is that (*Tor).Close assigns the control socket to nil, then somewhere in (*Tor).Listen there's an error, which at the end, results in an attempt to close the onion service. But by this point tor is closed and the control port is nil, so the onion service's closer is probably hitting the nil pointer dereference here. I thought about just PRing a nil check there, however I noticed there were other calls in the (*Tor).Listen function that might have issues.

So maybe the actual solution here is just to not assign the control port to nil, and just let all the other calls check and early return on "use of closed connection" errors. That's probably the most concurrency safe option, but I wanted to discuss it a little before submitting a PR. Alternatively I suppose the cancel function should really be what's used, and perhaps that's what I need to fix (potentially user error, I didn't spend to long on it) and a panic when t.Listen isn't canceled properly is expected.

Let me know what you make of this and if there's anything you'd like me to submit.

cretz commented 2 years ago

I haven't worked on this library much lately. As you've guessed, it appears the "cleanup on error" logic in Listen cannot safely handle closing its receiver before it has completed (specifically when you Tor.Close, it removes Tor.Control which OnionService.Close needs which is called on cleanup inside of Listen).

The safest route here is to not call Tor.Close while any methods are still running on the Tor object. If you need to bail out of a listen, cancel the context passed as the parameter and make sure the Listen call completes before closing.

hkparker commented 2 years ago

I tried using a cancelable context passed into t.Listen, and calling the cancel function before closing tor, but that hung for reasons unknown.

Maybe I just didn't wait long enough? Looked around the code a bit more and I'm not seeing why this would be the case. Either way, what do you think about not assigning Tor.Control to nil? Seems like the safer bet, errors trying to use the closed socket would be handled cleanly. If no I'll just keep deferring recover, not a huge deal, just doesn't feel good.

cretz commented 2 years ago

Maybe I just didn't wait long enough?

Did you make sure Listen returned before attempting to close? I think that is the key.