Bug 177047 – [Cocoa] Upstream sandbox-related WebKitSystemInterface functions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andy Estes]

[Cocoa] Upstream sandbox-related WebKitSystemInterface functions

[Andy Estes]

Created attachment 321028 [details]
Patch

[Build Bot]

Attachment 321028 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:52:  qtn_proc_apply_to_self is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:53:  qtn_proc_init_with_self is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:54:  qtn_proc_set_flags is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:55:  qtn_proc_alloc is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:56:  qtn_proc_free is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Estes]

Created attachment 321029 [details]
Patch

[Build Bot]

Attachment 321029 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:52:  qtn_proc_apply_to_self is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:53:  qtn_proc_init_with_self is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:54:  qtn_proc_set_flags is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:55:  qtn_proc_alloc is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:56:  qtn_proc_free is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Estes]

Created attachment 321036 [details]
Patch

[Build Bot]

Attachment 321036 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:52:  qtn_proc_apply_to_self is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:53:  qtn_proc_init_with_self is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:54:  qtn_proc_set_flags is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:55:  qtn_proc_alloc is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:56:  qtn_proc_free is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 321036 [details]
Patch

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

> Source/WebKit/ChangeLog:12
> +        (): Deleted.

What was deleted? Please file a bug to fix prepare-ChangeLog.

> Source/WebKit/Shared/SandboxExtension.h:46
> +    enum class Type {

I take it you feel creating this Type namespace improves readability?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:86
> +    qtn_proc_t quarantineProperties = qtn_proc_alloc();

For your consideration I suggest we make this a std::unique_ptr with qtn_proc_free() as its custom deleter. Then we can simplify the implementation of this function because we can omit  the calls to qtn_proc_free() and this makes the implementation less error prone in the unlikely event that a new early return code path is added.

> Source/WebKit/Shared/mac/SandboxExtensionMac.mm:64
> +    bool consume()

Would it make sense to add an attribute to warn if the return value is unused?

> Source/WebKit/Shared/mac/SandboxExtensionMac.mm:76
> +        int error = sandbox_extension_release(m_handle);

We could use std::exchange(m_handle, 0) here and then reduce this function to one line.

> Source/WebKit/Shared/mac/SandboxExtensionMac.mm:81
> +    const char* getSerializedFormat(size_t& length)

Ditto.

> Source/WebKit/Shared/mac/SandboxExtensionMac.mm:92
> +            m_token = sandbox_extension_issue_file(APP_SANDBOX_READ, path, 0);

I am not near my Mac. I am assuming the right-hand side expression allocates and returns a string that you take ownership of.

> Source/WebKit/Shared/mac/SandboxExtensionMac.mm:103
> +    char* m_token { nullptr };

Can we make this a std::unique_ptr? Then we do not need an in-class data member initializer (as unique_ptr default constructs to nullptr) and we can remove the destructor.

[Daniel Bates]

(In reply to Daniel Bates from comment #7)
> 
> > Source/WebKit/Shared/mac/SandboxExtensionMac.mm:81
> > +    const char* getSerializedFormat(size_t& length)
> 
> Ditto.
> 

I meant to write:

Would it make sense to add an attribute to warn if the return value is unused?

[Andy Estes]

Created attachment 321134 [details]
Patch

[Andy Estes]

(In reply to Daniel Bates from comment #7)
> Comment on attachment 321036 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=321036&action=review
> 
> For your consideration I suggest we make this a std::unique_ptr with
> qtn_proc_free() as its custom deleter. Then we can simplify the
> implementation of this function because we can omit  the calls to
> qtn_proc_free() and this makes the implementation less error prone in the
> unlikely event that a new early return code path is added.

I used a ScopeGuard that calls qtn_proc_free(), which I think is a little cleaner than creating a custom deleter struct for unique_ptr.

> 
> > Source/WebKit/Shared/mac/SandboxExtensionMac.mm:64
> > +    bool consume()
> 
> Would it make sense to add an attribute to warn if the return value is
> unused?

Done.

> 
> > Source/WebKit/Shared/mac/SandboxExtensionMac.mm:76
> > +        int error = sandbox_extension_release(m_handle);
> 
> We could use std::exchange(m_handle, 0) here and then reduce this function
> to one line.

Done.

> 
> > Source/WebKit/Shared/mac/SandboxExtensionMac.mm:81
> > +    const char* getSerializedFormat(size_t& length)
> 
> Ditto.

Marked with WARN_UNUSED_RETURN.

> 
> > Source/WebKit/Shared/mac/SandboxExtensionMac.mm:92
> > +            m_token = sandbox_extension_issue_file(APP_SANDBOX_READ, path, 0);
> 
> I am not near my Mac. I am assuming the right-hand side expression allocates
> and returns a string that you take ownership of.

Yes.

> 
> > Source/WebKit/Shared/mac/SandboxExtensionMac.mm:103
> > +    char* m_token { nullptr };
> 
> Can we make this a std::unique_ptr? Then we do not need an in-class data
> member initializer (as unique_ptr default constructs to nullptr) and we can
> remove the destructor.

I left this as-is. I think having a destructor calling free() is simpler than creating a custom deleter struct. The class is designed to only assign to m_token during construction and free it during destruction, so there aren't weird lifetime issues that unique_ptr would help solve.

[WebKit Commit Bot]

Comment on attachment 321134 [details]
Patch

Clearing flags on attachment: 321134

Committed r222183: <http://trac.webkit.org/changeset/222183>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 321036 [details]
Patch

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

>>> Source/WebKit/Shared/mac/SandboxExtensionMac.mm:103
>>> +    char* m_token { nullptr };
>> 
>> Can we make this a std::unique_ptr? Then we do not need an in-class data member initializer (as unique_ptr default constructs to nullptr) and we can remove the destructor.
> 
> I left this as-is. I think having a destructor calling free() is simpler than creating a custom deleter struct. The class is designed to only assign to m_token during construction and free it during destruction, so there aren't weird lifetime issues that unique_ptr would help solve.

Would be safer if you deleted the assignment operator and copy constructor so we don’t accidentally make a copy of this.

[Radar WebKit Bug Importer]

<rdar://problem/34693377>