1477722 - Create tests that verify CFI is present and working

Status: NEW

(Firefox Build System :: General, enhancement, P3)

Description

[Tom Ritter [:tjr]]

We should intentionally crash the browser in the various ways we are CFI enforcing to ensure CFI stays enabled.

Comment 1

[Tom Ritter [:tjr]]

Attachment: Bug 1477722 Beginning of CFI tests

Review commit: https://reviewboard.mozilla.org/r/258822/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258822/

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8994213 [details]
Bug 1477722 Beginning of CFI tests

https://reviewboard.mozilla.org/r/258822/#review265708


Code analysis found 3 defects in this patch:
 - 3 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: xpcom/base/nsDebugImpl.cpp:634
(Diff revision 1)
> +  __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop();
> +  __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop();
> +  __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop(); __nop();
> +
> +  longjmp(env, 1);
> +  return;

Warning: Redundant return statement at the end of a function with a void return type [clang-tidy: readability-redundant-control-flow]

  return;
~~^~~~~~~

::: xpcom/base/nsDebugImpl.cpp:709
(Diff revision 1)
> +  if (aType & nsCFIType::ICALL_INVALID_SIGNATURE) {
> +    MOZ_RELEASE_ASSERT(true, "ICALL_INVALID_SIGNATURE");
> +    int_arg_fn get_down_now_yall = (int_arg_fn)int_arg;
> +
> +    MOZ_RELEASE_ASSERT(true, "Should not crash");
> +    int ret = get_down_now_yall(5);

Warning: Value stored to 'ret' during its initialization is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

    int ret = get_down_now_yall(5);
        ^

::: xpcom/base/nsDebugImpl.cpp:713
(Diff revision 1)
> +    MOZ_RELEASE_ASSERT(true, "Should not crash");
> +    int ret = get_down_now_yall(5);
> +
> +    MOZ_RELEASE_ASSERT(true, "Should crash");
> +    get_down_now_yall = (int_arg_fn)float_arg;
> +    ret = get_down_now_yall(5);

Warning: Value stored to 'ret' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

    ret = get_down_now_yall(5);
    ^

Comment 3

[Tom Ritter [:tjr]]

This is currently blocked on what I think is a compiler bug here: https://bugs.llvm.org/show_bug.cgi?id=38200

Comment 4

[Tom Ritter [:tjr]]

Attachment: Bug 1477722 Beginning of CFI tests r=Alex_Gaynor

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The following patch is waiting for review from an inactive reviewer:

ID	Title	Author	Reviewer Status
D3039	Bug 1477722 Beginning of CFI tests r=Alex_Gaynor	tjr	Alex_Gaynor: Inactive

:tjr, could you please find another reviewer or abandon the patch if it is no longer relevant?

For more information, please visit auto_nag documentation.