Skip to content

Possible handle leak: the six RemoveXDevice exports return before dh.Delete() when RemoveDeviceByID fails #40

Description

@OvOhao

Possible handle leak: the six RemoveXDevice exports return before dh.Delete() when RemoveDeviceByID fails

Each device type repeats the same body: resolve the handle, call
RemoveDeviceByID, return false on error, and only then drop the handle.

lib/viiper/xbox360.go:186

func RemoveXbox360Device(handle C.Xbox360DeviceHandle) bool {
	dh := cgo.Handle(handle)
	dhw, ok := dh.Value().(*deviceHandleWrapper)
	if !ok {
		return false
	}
	if err := dhw.usbServer.s.RemoveDeviceByID(dhw.exportMeta.BusID, fmt.Sprintf("%d", dhw.exportMeta.DevID)); err != nil {
		return false
	}
	...
	dh.Delete()
	return true
}

The same early return and the same trailing Delete appear in all six copies:

lib/viiper/dualsense.go:330 and :342
lib/viiper/dualshock4.go:263 and :275
lib/viiper/keyboard.go:301 and :313
lib/viiper/mouse.go:149 and :161
lib/viiper/ns2pro.go:259 and :271
lib/viiper/xbox360.go:192 and :204

RemoveDeviceByID fails for a device the server has already removed: it returns
bus %d not found (internal/server/usb/server.go:259) or the bus-level error at
:263. The HTTP disconnect timeout produces exactly that state by removing the
device from the bus (internal/server/api/server.go:299) without touching the
library's deviceHandles map. The handle then stays in shw.deviceHandles and
pins a deviceHandleWrapper (lib/viiper/viiper.go:33), which holds usbServer,
so the whole usbServerHandleWrapper graph stays reachable.

CloseUSBServer shows the ordering that works: it deletes every device handle
(lib/viiper/server.go:121) before checking hw.s.Close() for an error
(server.go:129).

Suggested fix: drop the handle and its deviceHandles entry whether or not
RemoveDeviceByID succeeded, while still returning false to the caller.

If you could credit me as a reporter for my contributions to security advisory I will be thankful.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions