Bug 142980 - [Seccomp] Should be easier to debug blocked syscalls
Summary: [Seccomp] Should be easier to debug blocked syscalls
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Enhancement
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on: 150172
Blocks: 140072
  Show dependency treegraph
 
Reported: 2015-03-23 13:45 PDT by Michael Catanzaro
Modified: 2015-10-15 08:52 PDT (History)
3 users (show)

See Also:


Attachments
[Linux] Seccomp Filters: Make it easier to debug blocked syscalls (8.27 KB, patch)
2015-03-23 13:54 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
[Linux] Seccomp Filters: Add helper functions for common directories (6.50 KB, patch)
2015-03-23 14:07 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
[Linux] Seccomp Filters: Make it easier to debug blocked syscalls (8.33 KB, patch)
2015-03-23 14:24 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (6.76 KB, patch)
2015-07-19 16:12 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (6.74 KB, patch)
2015-07-20 12:03 PDT, Michael Catanzaro
zan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2015-03-23 13:45:24 PDT
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.
Comment 1 Michael Catanzaro 2015-03-23 13:54:30 PDT
Created attachment 249264 [details]
[Linux] Seccomp Filters: Make it easier to debug blocked syscalls
Comment 2 WebKit Commit Bot 2015-03-23 13:56:54 PDT
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.
Comment 3 Michael Catanzaro 2015-03-23 14:07:35 PDT
Created attachment 249265 [details]
[Linux] Seccomp Filters: Add helper functions for common directories
Comment 4 Michael Catanzaro 2015-03-23 14:10:36 PDT
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: mcatanzaro@igalia.com
Logging in as mcatanzaro@igalia.com...
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
Comment 5 Michael Catanzaro 2015-03-23 14:24:30 PDT
Created attachment 249274 [details]
[Linux] Seccomp Filters: Make it easier to debug blocked syscalls
Comment 6 Michael Catanzaro 2015-07-19 16:12:29 PDT
Created attachment 257067 [details]
Patch
Comment 7 Zan Dobersek 2015-07-19 23:05:41 PDT
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.
Comment 8 Michael Catanzaro 2015-07-20 06:02:30 PDT
(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).
Comment 9 Michael Catanzaro 2015-07-20 12:03:07 PDT
Created attachment 257114 [details]
Patch
Comment 10 Zan Dobersek 2015-07-20 12:08:37 PDT
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.
Comment 11 Michael Catanzaro 2015-07-20 13:06:02 PDT
Thanks, I'll go with "unknown action".
Comment 12 Michael Catanzaro 2015-07-20 13:07:46 PDT
Committed r187030: <http://trac.webkit.org/changeset/187030>