Bug 177052 – [WPE][GTK] Further cleanups in ProcessLauncherGLib.cpp

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKitGTK

[Michael Catanzaro]

In order to use a more complex argv for the child process, we're going to need to clean up how we construct the argv vector. Let's use a Vector object so we don't need to guess the size of argv in advance. (I actually did this first using GPtrArray before I realized that was stupid.)

Also, tighten the scopes of some variables and further improve the error message.

[Michael Catanzaro]

Created attachment 321042 [details]
Patch

[Michael Catanzaro]

Created attachment 321043 [details]
Patch

[Michael Catanzaro]

This is super wrong, it relies on a bunch of stack use-after-frees that I didn't notice because it just happens to work in practice. I need to be more careful.

[Michael Catanzaro]

Created attachment 321051 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 321051 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321051&action=review

I think we are complicating the code to avoid just a few copies/allocations. I think it would be easier to take the opposite approach, and make the argv take the ownership of the strings. We could use a GPtrArray instead of a vector.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:61
> +    // Footgun warning: the argv vector does not own the strings it contains, so
> +    // the owning containers need to stay in scope. Seems safest to declare them
> +    // all way up here.

This could happen if argv takes the ownership of strings.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:107
>          m_launchOptions.processCmdPrefix.split(' ', splitArgs);

Here we could use g_strsplit and pass the strings to the array, only freeing the container.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:109
> +            argv.append(arg.utf8().data());

This is still super-wrong. utf8() returns a temporary, so you can't save the data() here, it will be freed right after the append.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:112
> +    argv.reserveInitialCapacity(4);

If we keep using a Vector, it's better to pre-allocate this in the stack i think. Vector<const char*, 4>. If we use a GPtrArray we could use g_ptr_array_new_full() to provide this as initial size and g_free as free func.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:114
> +    realExecutablePath = fileSystemRepresentation(executablePath).data();

This is the only copy we would do, we can either keep using fileSystemRepresentation() adn g_strdup() the .data(), or use g_uri_unescape_string() directly, which is what fileSystemRepresentation() does in the end.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:117
> +    webkitSocket.reset(g_strdup_printf("%d", socketPair.client));

This would be added to the array directly.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:121
> +        wpeSocket.reset(g_strdup_printf("%d", wpe_renderer_host_create_client()));

Ditto.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:126
> +    argv.append(realPluginPath.data());

And here again we could dup or use g_uri_unescape_string().

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #5)
> Comment on attachment 321051 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=321051&action=review
> 
> I think we are complicating the code to avoid just a few copies/allocations.
> I think it would be easier to take the opposite approach, and make the argv
> take the ownership of the strings. We could use a GPtrArray instead of a
> vector.

I actually tried that approach, but ended up rejecting it. The problem is that to use bubblewrap I need to pass dozens and dozens of arguments that are mostly string literals, so that would mean complicating the code with tons of g_strdup()s that aren't really needed.

On the other hand, the strdup()s could easily be done by an addArguments() helper function, and having a few dozen unnecessary strdup()s is hardly a big deal either, and that really would simplify the code.
 
> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:109
> > +            argv.append(arg.utf8().data());
> 
> This is still super-wrong. utf8() returns a temporary, so you can't save the
> data() here, it will be freed right after the append.

Thanks for catching this bug. It's frustrating that the code works fine and there's no warning. :/

You're probably right, not using strdup() is too hard. And it does indeed complicate the code.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #6)
> You're probably right, not using strdup() is too hard. And it does indeed
> complicate the code.

Yeah, I'm definitely going to change it to take ownership. Not sure if I'll do that in an update to this patch now or later. Thanks for using r- when needed.

[Michael Catanzaro]

GPtrArray is probably best. I'll do this later in a different bug.