Bug 142980 – [Seccomp] Should be easier to debug blocked syscalls

• Component: WebKit2
• Status: RESOLVED FIXED

[Michael Catanzaro]

Let's print a warning when a syscall is blocked, even in release builds. Otherwise problems caused by the sandbox will be basically impossible to debug.

[Michael Catanzaro]

Created attachment 249264 [details]
[Linux] Seccomp Filters: Make it easier to debug blocked syscalls

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/Shared/linux/SeccompFilters/SeccompBroker.cpp:308:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit2/Shared/linux/SeccompFilters/SyscallPolicy.cpp:103:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebKit2/Shared/linux/SeccompFilters/Syscall.cpp:78:  msg_needed is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit2/Shared/linux/SeccompFilters/Syscall.cpp:81:  write_uint is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit2/Shared/linux/SeccompFilters/Syscall.cpp:86:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 5 in 5 files


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

[Michael Catanzaro]

Created attachment 249265 [details]
[Linux] Seccomp Filters: Add helper functions for common directories

[Michael Catanzaro]

Comment on attachment 249265 [details]
[Linux] Seccomp Filters: Add helper functions for common directories

$ webkit-patch post-commits 7f1dedab84e00f593d6618 --no-review
bugs.webkit.org login: [e-mail]
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/show_bug.cgi?id=142980&ctype=xml&excludefield=attachmentdata
Obsoleting 1 old patch on bug 142980
Obsoleting attachment: 249264
Adding patch "[Linux] Seccomp Filters: Add helper functions for common directories" to https://bugs.webkit.org/show_bug.cgi?id=142980

Looks like a bug in webkit-patch, since the URL in the patch is clearly https://bugs.webkit.org/show_bug.cgi?id=142982

[Michael Catanzaro]

Created attachment 249274 [details]
[Linux] Seccomp Filters: Make it easier to debug blocked syscalls

[Michael Catanzaro]

Created attachment 257067 [details]
Patch

[Zan Dobersek]

Comment on attachment 257067 [details]
Patch

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

> Source/WebKit2/Shared/linux/SeccompFilters/SyscallPolicy.cpp:277
> +    CRASH();

ASSERT_NOT_REACHED() maybe, with a default return value? The compiler might also be smart and verbose enough to throw a warning/error when all the enum values are not accounted for in a switch statement with no default label.

[Michael Catanzaro]

(In reply to comment #7)
> Comment on attachment 257067 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257067&action=review
> 
> > Source/WebKit2/Shared/linux/SeccompFilters/SyscallPolicy.cpp:277
> > +    CRASH();
> 
> ASSERT_NOT_REACHED() maybe, with a default return value?

OK

> The compiler might
> also be smart and verbose enough to throw a warning/error when all the enum
> values are not accounted for in a switch statement with no default label.

It will do so if we use -Wswitch (we do; it's implied by -Wall), and it will do so _even with a default label_ if we use -Wswitch-enum (we don't, thank goodness).

[Michael Catanzaro]

Created attachment 257114 [details]
Patch

[Zan Dobersek]

Comment on attachment 257114 [details]
Patch

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

> Source/WebKit2/Shared/linux/SeccompFilters/SyscallPolicy.cpp:279
> +    return "";

I'd prefer 'unknown' or similar just so that the message doesn't get confusing and discarded because of an empty permission name.

[Michael Catanzaro]

Thanks, I'll go with "unknown action".

[Michael Catanzaro]

Committed r187030: <http://trac.webkit.org/changeset/187030>