[RNTester][IOS] Enable Address and Undefined Behavior Sanitizers #36443

Saadnajmi · 2023-03-10T23:31:12Z

Summary

This change does two things:

Moves RNTester on iOS to use an .xctestplan to configure its tests. This moves the test configuration out of the .xcscheme file, and into a new json file that Xcode likes. It also allows you to run the same tests multiple times with different configurations. This was done by clicking the focused button below.

Note: this is partially me upstreaming a change from React Native macOS (microsoft#190), though at some point our Xcode project got messed up and we were no longer referencing them 🤷🏾.

Enables Address Sanitizer and Udefined Behavior Sanitizer through the xctestplan

This should help catch some extra vulnerabilities should they ever show up while our tests run.

Notably, I did not also add a configuration to run Thread Sanitizer, because tests start failing with that enabled 😅.

Changelog

[IOS] [SECURITY] - Enable Address and Undefined Behavior Sanitizers on RNTester

Test Plan

CI should pass

analysis-bot · 2023-03-11T00:11:48Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,519,189	+0
android	hermes	armeabi-v7a	7,834,780	+0
android	hermes	x86	8,999,116	+0
android	hermes	x86_64	8,854,116	+0
android	jsc	arm64-v8a	9,141,839	+0
android	jsc	armeabi-v7a	8,333,495	+0
android	jsc	x86	9,196,443	+0
android	jsc	x86_64	9,454,253	+0

Base commit: 346de2d
Branch: main

cipolleschi · 2023-03-11T09:20:46Z

Hi @Saadnajmi, thank you for the PR. I'll have a more thorough look at it on Monday, but currently the [RCTAnimationUtilsTests testColorInterpolation] test is failing consistently in CI. Could you have a look at it, please?

Saadnajmi · 2023-03-13T18:30:14Z

Hi @Saadnajmi, thank you for the PR. I'll have a more thorough look at it on Monday, but currently the [RCTAnimationUtilsTests testColorInterpolation] test is failing consistently in CI. Could you have a look at it, please?

Sure. I'll take a look in the next day or two. I think I reproed the same crash locally but wasn't sure if it was local or acutally because I enabled the new sanitizers. It'll be interesting to see if we can make that test better now :)

Saadnajmi · 2023-03-16T18:19:54Z

For reference, the error I'm seeing locally is as follows:

=================================================================
==56710==ERROR: AddressSanitizer: odr-violation (0x000123ef2a80):
  [1] size=56 'folly::exception_wrapper::uninit_' react-native/packages/rn-tester/Pods/RCT-Folly/folly/ExceptionWrapper.cpp:25:52
  [2] size=56 'folly::exception_wrapper::uninit_' react-native/packages/rn-tester/Pods/Flipper-Folly/folly/ExceptionWrapper.cpp:25:52
These globals were registered at these points:
...

This seems to be similar to https://github.com/google/sanitizers/wiki/AddressSanitizerOneDefinitionRuleViolation
It looks like ASAN doesn't like we pull in Folly from both Flipper-Folly and RCT-Folly. This also explains why I didn't see the error in React Native macOS, we don't enable flipper by default.
Perhaps we only enable the sanitizer if flipper is not compiled? Working on it..

Saadnajmi · 2023-03-16T21:03:34Z

Nevermind, it's not Address Sanitizer at all. It's actually Undefined Behavior Sanitizer. See the error:

This can be fixed by casting to uint32_t instead. I pushed a fix up :)

cipolleschi

Great job, thank you for spending time in fixing the tests! :D

Let's see if the internal infra is happy about these changes! 😅

facebook-github-bot · 2023-03-20T10:22:14Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cortinico

Great stuff @Saadnajmi 👍

facebook-github-bot · 2023-03-20T13:17:12Z

@cipolleschi merged this pull request in 65e61f3.

Summary: This change does two things: 1) Moves RNTester on iOS to use an `.xctestplan` to configure its tests. This moves the test configuration out of the `.xcscheme` file, and into a new json file that Xcode likes. It also allows you to run the same tests multiple times with different configurations. This was done by clicking the focused button below. ![image](https://user-images.githubusercontent.com/6722175/224447314-49d460e9-d2d6-4ec9-9767-64c2a1c449c0.png) Note: this is partially me upstreaming a change from React Native macOS (microsoft#190), though at some point our Xcode project got messed up and we were no longer referencing them ��. 2) Enables Address Sanitizer and Udefined Behavior Sanitizer through the xctestplan This should help catch some extra vulnerabilities should they ever show up while our tests run. Notably, I did not _also_ add a configuration to run Thread Sanitizer, because tests start failing with that enabled 😅. ## Changelog: [IOS] [SECURITY] - Enable Address and Undefined Behavior Sanitizers on RNTester Pull Request resolved: facebook#36443 Test Plan: CI should pass Reviewed By: cortinico Differential Revision: D44213517 Pulled By: cipolleschi fbshipit-source-id: 0646174c4b416413a563e8178aa2cfca230b5e66

… tests (#44642) Summary: On React Native macOS (I am not sure with the current state of React Native), the Xcode Unit and Integration tests are a bit flaky. Rather than set "retry on failure up to 3 times" through the pipeline config (in our case, Azure Pipelines), I realized my earlier PR to use Xcode test plans (#36443) means we can have Xcode retry the test. This should be faster than retrying it on the pipeline, because it retries just the failing test, not the entire "test" step. I did this on React Native macOS, so I'm doing it upstream so we can remove a diff. ## Changelog: [INTERNAL] [CHANGED] - Set `retryOnFailure` for Xcode Unit and Integration tests Pull Request resolved: #44642 Test Plan: CI should pass (faster) Reviewed By: cortinico Differential Revision: D57662523 Pulled By: cipolleschi fbshipit-source-id: 8de2ab0ea15ba4d38c3b5bf96108c0c7ff5e9f32

… tests (facebook#44642) Summary: On React Native macOS (I am not sure with the current state of React Native), the Xcode Unit and Integration tests are a bit flaky. Rather than set "retry on failure up to 3 times" through the pipeline config (in our case, Azure Pipelines), I realized my earlier PR to use Xcode test plans (facebook#36443) means we can have Xcode retry the test. This should be faster than retrying it on the pipeline, because it retries just the failing test, not the entire "test" step. I did this on React Native macOS, so I'm doing it upstream so we can remove a diff. ## Changelog: [INTERNAL] [CHANGED] - Set `retryOnFailure` for Xcode Unit and Integration tests Pull Request resolved: facebook#44642 Test Plan: CI should pass (faster) Reviewed By: cortinico Differential Revision: D57662523 Pulled By: cipolleschi fbshipit-source-id: 8de2ab0ea15ba4d38c3b5bf96108c0c7ff5e9f32

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Microsoft Partner: Microsoft Partner labels Mar 10, 2023

[RNTester][IOS] Enable Address and Undefined Behavior Sanitizers

865faec

Saadnajmi force-pushed the testplan branch from 16b170f to 865faec Compare March 10, 2023 23:50

Merge branch 'main' of github.com:facebook/react-native into testplan

8c19d27

Fix colorInterpolation test

b302f46

Saadnajmi added 2 commits March 16, 2023 14:48

convert to uint32_t

120841c

Merge branch 'main' of github.com:facebook/react-native into testplan

f9bfea6

cipolleschi approved these changes Mar 20, 2023

View reviewed changes

cortinico approved these changes Mar 20, 2023

View reviewed changes

facebook-github-bot closed this in 65e61f3 Mar 20, 2023

facebook-github-bot added the Merged This PR has been merged. label Mar 20, 2023

Saadnajmi deleted the testplan branch March 20, 2023 17:42

Saadnajmi mentioned this pull request Nov 7, 2023

[RN][Testing] Make integration tests work in the New Architecture #41362

Closed

Saadnajmi mentioned this pull request May 21, 2024

chore(ci): Set retryOnFailure for RNTester iOS Unit and Integration tests #44642

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RNTester][IOS] Enable Address and Undefined Behavior Sanitizers #36443

[RNTester][IOS] Enable Address and Undefined Behavior Sanitizers #36443

Saadnajmi commented Mar 10, 2023

analysis-bot commented Mar 11, 2023 •

edited

Loading

cipolleschi commented Mar 11, 2023

Saadnajmi commented Mar 13, 2023

Saadnajmi commented Mar 16, 2023 •

edited

Loading

Saadnajmi commented Mar 16, 2023 •

edited

Loading

cipolleschi left a comment

facebook-github-bot commented Mar 20, 2023

cortinico left a comment

facebook-github-bot commented Mar 20, 2023

[RNTester][IOS] Enable Address and Undefined Behavior Sanitizers #36443

[RNTester][IOS] Enable Address and Undefined Behavior Sanitizers #36443

Conversation

Saadnajmi commented Mar 10, 2023

Summary

Changelog

Test Plan

analysis-bot commented Mar 11, 2023 • edited Loading

cipolleschi commented Mar 11, 2023

Saadnajmi commented Mar 13, 2023

Saadnajmi commented Mar 16, 2023 • edited Loading

Saadnajmi commented Mar 16, 2023 • edited Loading

cipolleschi left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 20, 2023

cortinico left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 20, 2023

analysis-bot commented Mar 11, 2023 •

edited

Loading

Saadnajmi commented Mar 16, 2023 •

edited

Loading

Saadnajmi commented Mar 16, 2023 •

edited

Loading