tractordev / toolkit-go

MIT License
5 stars 4 forks source link

Memory Safety in purego #14

Open gysddn opened 1 week ago

gysddn commented 1 week ago

While investigating the canvas issue in env86, I have tried to compile and run the project in many different configurations and distros to simply run it with a lower webkit2gtk version but unfortunately I have run into many different problems while doing that. This simply raised a question of memory safety in general, especially given the nature of the project with a C-bridge.

The problems are of course, dereferenced NULL pointers and uninitialized memories. A typical example of this would be the case where an operation of window is called before the window is initialized or loaded.

This issue tries to list what can be done about it.

Test case

The test case below demonstrates a case where there can be arguments that cannot be null with arguments that can.

    WebkitWebViewEvaluateJavascript func(
        webView unsafe.Pointer,  // Has to be non-null and initialized
        script string, // purego takes care of this char*
        length int32,

               /* Nullable arguments */
        worldName unsafe.Pointer,
        sourceUri unsafe.Pointer,
        cancellable unsafe.Pointer,
        callback unsafe.Pointer,
        userData unsafe.Pointer,
    )

Approaches

Wrapper function before every C call
WebkitWebViewEvaluateJavascript  -- calls --> webkitWebViewEvaluateJavascript

The wrapper will call the internal purego function after doing all the checks and purego bindings themselves will not exposed. But this is tedious to add and maintain for every single function since there are no macros.

Non-Null type (If possible)
WebkitWebViewEvaluateJavascript(webView unsafe.Pointer, ...)  ----> WebkitWebViewEvaluateJavascript(webView NonNull, ...)

This is only possible if there is an inline cast to unsafe.Pointer or uintptr in go, otherwise if only option is to use struct wrapper like:

type NonNull struct {
    Pointer unsafe.Pointer
}

/* Build NonNull api here */

This would require a signature change in every purego binding, which might or might not break the bridge. Even though one might assume the struct above would have the same size as a pointer and memory read to to it would be the same, it is not entirely certain without testing.

Safe function to call on parameters
WebkitWebViewEvaluateJavascript(webView.Handle, ...)  ----> WebkitWebViewEvaluateJavascript(SafeArg(webView.Handle), ...)

This is a feasible option to add some null-checking that would provide some info on where the problem is in the go side. This would definitely make debugging a bit easier for the project. But it is tricky to type check the argument for uninitialized memory with this approach because the type of the argument will not be in the go side so there is nothing to check it against, WebView struct in this example.

Progressively add types and remove instances of unsafe.Pointer
WebkitWebViewEvaluateJavascript(webView unsafe.Pointer, ...)  ----> WebkitWebViewEvaluateJavascript(webView WebView*, ...)

This has the potential to take care of both null and type checking but it should really be the last resort because of how invasive and tricky it can be. Some types can require other types one of after another, even referencing some from other libraries not really included in the scope of the project. This is also tricky because of the macro's controlling the struct's definition, requiring it be an exact match to a version, architecture, build flags etc.

It also wouldn't bring much of a value on top of checks because of how well gtk encapsulates data, not really requiring much to interact with the struct.

One approach of course would be to use shallow structures, maybe checking only what's necessary but I believe that wouldn't be any less tedious.

Uninitialized memory and type checking

GLib has a type checking system and gtk uses that heavily internally, including webkitgtk, on critical arguments. When that's the case, there is the always the option of ignoring it.

When these libraries are built with debug flags, they primarily handle failures by printing error messages to stderr and returning control to the caller. This is the main failure-handling mechanism. Most of the failure functions are internal and not exposed to the language bindings, meaning there is no direct, programmatic way to check whether a function call failed or to determine the specific reason for the failure. From initial observations, it appears that these functions simply return to the caller without providing additional error details for external handling.

But this is an option that can be explored further and could provide some value if it's possible to cleanly import this interface to go.

progrium commented 1 week ago

Typically we would generate bindings from a schema or API definition that would allow us to create more idiomatic Go functions to wrap the direct purego calls. However, I usually don't do that unless I know we need most of the API. We're only using a subset of webkit/gtk and the platform bindings for the desktop package are already an indirection, so users don't use those APIs directly. In general though, create wrappers if necessary.