Fixes potential cgo.Handle panic

[ibeckermayer]

The rdpclient.Client in Go has a cgo.Handle field, which is a special type cgo provides to deal with on of the idiosyncrasies of working between Go and C:

Handle provides a way to pass values that contain Go pointers (pointers to memory allocated by Go) between Go and C without breaking the cgo pointer passing rules. A Handle is an integer value that can represent any Go value. A Handle can be passed through C and back to Go, and Go code can use the Handle to retrieve the original Go value.

The underlying object of a cgo.Handle can be retrieved by calling handle.Value(), like we do in handle_bitmap:

teleport/lib/srv/desktop/rdp/rdpclient/client.go

Lines 393 to 396 in ad841e0

	//export handle_bitmap
	func handle_bitmap(handle C.uintptr_t, cb *C.CGOBitmap) C.CGOErrCode {
	return cgo.Handle(handle).Value().(*Client).handleBitmap(cb)
	}

and its memory must be explicitly released after use by calling handle.Delete(). Previously, we were calling handle.Delete() in Client.Close()

teleport/lib/srv/desktop/rdp/rdpclient/client.go

Lines 445 to 456 in 8ce4e6b

	// Close shuts down the client and closes any existing connections.
	// It is safe to call multiple times, from multiple goroutines.
	// Calls other than the first one are no-ops.
	func (c *Client) Close() {
	c.closeOnce.Do(func() {
	c.handle.Delete()
	if err := C.close_rdp(c.rustClient); err != C.ErrCodeSuccess {
	c.cfg.Log.Warningf("failed to close the RDP client")
	}
	})
	}

which was deferred in both the video output streaming goroutine (which calls handle_bitmap repeatedly)

teleport/lib/srv/desktop/rdp/rdpclient/client.go

Lines 239 to 245 in 8ce4e6b

	// Video output streaming worker goroutine.
	c.wg.Add(1)
	go func() {
	defer c.wg.Done()
	defer c.Close()
	defer c.cfg.Log.Info("RDP output streaming finished")

and in the user input streaming goroutine:

teleport/lib/srv/desktop/rdp/rdpclient/client.go

Lines 259 to 264 in 8ce4e6b

	// User input streaming worker goroutine.
	c.wg.Add(1)
	go func() {
	defer c.wg.Done()
	defer c.Close()
	defer c.cfg.Log.Info("TDP input streaming finished")

.

When a user closes a tab like in #13407, the input streaming goroutine returns

teleport/lib/srv/desktop/rdp/rdpclient/client.go

Lines 268 to 270 in 8ce4e6b

	msg, err := c.cfg.Conn.InputMessage()
	if errors.Is(err, io.EOF) {
	return

meaning the defer c.Close() gets called and calls c.handle.Delete(). Therefore, its possible that the video output streaming goroutine continues running for long enough to call handle_bitmap again, which attempts to call c.handle.Value() on an invalid handle (the one that's been Delete()ed), which causes the panic. The precise mechanics of how this happens, and why it happens more often when smartcard negotiation is taking place but not in other circumstances, is not entirely clear to me (though I have the beginnings of a hypothesis). However with these changes, such a scenario is now impossible, and so the panic should never occur.

Closes #13407

[zmb3]

Looks like you've definitely tracked down the source of the issue, but I'd like to clean up the solution a bit.

[zmb3]

This is a confusing API and I would like to avoid it.

How is someone supposed to know if "something fails between New() and Wait()" anyway?

Is there some way we could perhaps:

• unexport cleanup()
• make it a required convention to always call Wait() after new, to ensure cleanup happens
• make Wait() just perform cleanup and return immediately if something failed with New

[ibeckermayer]

I agree its a confusing API. What I was trying to express was what we have going on here:

teleport/lib/srv/desktop/windows_server.go

Line 878 in ad841e0

rdpc.Cleanup()

where we create a New() and then use it for things that might fail before calling Wait().

I think I found a way to refactor this so that the API makes more sense though, see f61a89f

[zmb3]

Looks much better. A few nits but otherwise LGTM.

[zmb3]

Suggested change

	// call StartAndWait()
	// call Run()

[github-actions]

@ibeckermayer See the table below for backport results.

Branch	Result
branch/v10	Create PR
branch/v9	Failed

[zmb3]

@ibeckermayer don't forget to open backports :-)

[ibeckermayer]

v9 backport fixed: #13590