[Webkit-unassigned] [Bug 193571] [GTK][WPE] Add API to add paths to sandbox

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=193571

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #359484|review?, commit-queue?      |review-, commit-queue-
              Flags|                            |

--- Comment #8 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 359484
  --> https://bugs.webkit.org/attachment.cgi?id=359484
Patch

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

Good!

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1182
> + * Adds a path to be mounted in the sandbox. @path must exist before any web process
> + * has been created otherwise it will be silently ignored. It is a fatal error to

Maybe we should attempt a g_mkdir_with_parents(), so we don't have to worry about this if the directory is writable? Bad idea?

We could add a flags parameter to control the behavior?

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1189
> +void webkit_web_context_add_path_to_sandbox(WebKitWebContext* context, const char* path, gboolean read_only)

Add an enum. Join the inquisition against boolean parameters!

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1197
> +    context->priv->processPool->appendSandboxPaths(String(path), read_only);

I think you can just pass path and let it be implicitly be converted toString.

> Source/WebKit/UIProcess/WebProcessPool.h:474
> +    void appendSandboxPaths(const String& path, bool readOnly) { m_extraSandboxPaths.set(path, readOnly); };

bool isn't OK here either; there should be a different new enum (since this is below the API layer and can't use the new enum you'll add to the API).

There really is an inquisition, you know. Be the change you want to see in function signatures!

> Source/WebKit/UIProcess/WebProcessPool.h:733
> +    HashMap<String, bool> m_extraSandboxPaths;

Then you can use the new enum here too. The code really will be more readable.

> Source/WebKit/UIProcess/glib/WebProcessProxyGLib.cpp:56
> +    String extraPaths;
> +    for (const auto& entry : m_processPool->sandboxPaths()) {
> +        if (!extraPaths.isEmpty())
> +            extraPaths.append('\0');
> +        extraPaths.append(entry.key);
> +        extraPaths.append(entry.value ? ":ro" : ":rw");
> +    }
> +
> +    launchOptions.extraInitializationData.add("extra-sandbox-paths", extraPaths);

Why are you doing this instead of just adding a second HashMap to struct LaunchOptions? Would be much simpler, right?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190121/afbb06fd/attachment-0001.html>