Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reformat_libyuv: Add ignore CFI annotation #1507

Merged
merged 1 commit into from
Aug 18, 2023
Merged

reformat_libyuv: Add ignore CFI annotation #1507

merged 1 commit into from
Aug 18, 2023

Conversation

vigneshvg
Copy link
Collaborator

libyuv is a C++ library and defines custom types (struct, enum, etc) in a namespace when conditionally compiled with a C++ compiler. When accessed from a C library like libavif, via a function pointer, this leads to signature mismatches in the CFI sanitizers since the C++ header file defines the types within the namespace and the C code has the types without the namespace. In order to avoid this CFI error, we tag some functions as an exception when being compiled with CFI enabled.

For more details on clang's CFI see:
https://clang.llvm.org/docs/ControlFlowIntegrity.html.

For a simpler example of this bug, please see:
https://github.com/vigneshvg/cpp_c_potential_cfi_bug

Sample CFI failure in Chromium: crbug.com/1473228

Copy link
Collaborator

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I suggest some changes to the comments.

Note: Another simple solution would be to compile src/reformat_libyuv.c as C++ code. Then this file and libyuv itself will see libyuv's types in the libyuv namespace, and CFI will be happy. We will need to make sure this does not add a dependency on the C++ runtime library.

src/reformat_libyuv.c Outdated Show resolved Hide resolved
src/reformat_libyuv.c Outdated Show resolved Hide resolved
@vigneshvg
Copy link
Collaborator Author

LGTM. I suggest some changes to the comments.

Note: Another simple solution would be to compile src/reformat_libyuv.c as C++ code. Then this file and libyuv itself will see libyuv's types in the libyuv namespace, and CFI will be happy. We will need to make sure this does not add a dependency on the C++ runtime library.

Ack. I agree that's another solution, but would rather get away with the annotation than meddling around the compilation settings for this file alone in various environments.

Copy link
Collaborator

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

libyuv is a C++ library and defines custom types (struct, enum, etc) in
a namespace when conditionally compiled with a C++ compiler. When
accessed from a C library like libavif, via a function pointer, this
leads to signature mismatches in the CFI sanitizers since the C++ header
file defines the types within the namespace and the C code has the types
without the namespace. In order to avoid this CFI error, we tag some
functions as an exception when being compiled with CFI enabled.

For more details on clang's CFI see:
https://clang.llvm.org/docs/ControlFlowIntegrity.html.

For a simpler example of this bug, please see:
https://github.com/vigneshvg/cpp_c_potential_cfi_bug

Sample CFI failure in Chromium: crbug.com/1473228
@vigneshvg vigneshvg merged commit 152cbde into AOMediaCodec:main Aug 18, 2023
14 checks passed
@vigneshvg vigneshvg deleted the cl_ignore_cfi branch August 18, 2023 22:13
aarongable pushed a commit to chromium/chromium that referenced this pull request Aug 21, 2023
Re-landing https://crrev.com/c/4763864

The underlying bug that was causing the CFI failures was fixed in
libavif here: AOMediaCodec/libavif#1507

It was included in Chromium DEPS here: https://crrev.com/c/4794456

Also remove "-fsanitize-cfi-icall-generalize-pointers" from the
libavif BUILD.gn since that is no longer necessary as the underlying
libavif fix does it better by disabling cfi-icall checks for only
one specific function.

Test: blink_platform_unittests pass with CFI enabled.
Bug: 1473228
Change-Id: I2453fcc077fa3235874ae660da17a2e9f86ca7c1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4795633
Reviewed-by: Wan-Teh Chang <[email protected]>
Commit-Queue: Vignesh Venkat <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1185915}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants