frida/frida-go

FError pattern is a potential leak

Closed this issue · 2 comments

I noticed today while spelunking around that most frida operations return GError. Which we then wrap with FError. I noticed that the only time GError in this case is freed is when it's .Error() func is called. In many cases it's reasonable to not always called .Error() for instance (the reverse is also true here when calling Error twice we end up with an empty string):

func main() {
	d := frida.NewDeviceManager()
	d.EnumerateDevices()
	fridaDevice, err := d.FindDeviceByID("00008101-0005190C3EA0001E")
	if err != nil {
		log.Fatal(err)
	}
	_, err = fridaDevice.Attach(0, nil)
	fmt.Println(err.Error())
	fmt.Println(err.Error())

	err = errors.New("hello")
	fmt.Println(err.Error())
	fmt.Println(err.Error())
}
FError: The Frida system session is not available on jailed iOS
FError: 
hello
hello

I have many patterns where I just check if err != nil and then I handle the error without ever calling .Error(). I think if this is the pattern that will be supported in the future it would need some better documentation. FError would need to provide a Clean func which could lead to some pretty nasty code for the caller. Something like:

	_, err = fridaDevice.Attach(0, nil)
	if err, ok := err.(frida.FError); ok {
		err.Clean()
	}

Or whatever flavor of errors.Is/As works best here.

I personally think the bindings should handle the copying of the error into a GoString and freeing it for the caller, and the caller should just receive a regular golang error so they aren't concerned with freeing anything. For instance much of the bindings code would turn into:

func handleGError(gErr *C.GError) error {
	defer clean(unsafe.Pointer(gErr), unrefGError)
	return fmt.Errorf("FError: %s", C.GoString(gErr.message))
}

...
                s := C.frida_device_attach_sync(d.device, C.guint(pid), opt, nil, &err)
		return &Session{s}, handleGError(err)
...

Another large lift. This is also would be an implicit api change. What are your thoughts? If you like this. I would tackle it.

Cheers!

Hey there @dylanhitt, another great idea and thank you for digging into this. I will say that I absolutely love the idea and the handleGError function you have here.

Nice, I'll start on it sometime this week.