Add guards around nativeProps usage to prevent race conditions#52646
Add guards around nativeProps usage to prevent race conditions#52646OrfeasZ wants to merge 1 commit into
Conversation
|
Hi @OrfeasZ! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
meta-cla
Bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
facebook-github-bot
added
the
Shared with Meta
Some third-party libraries, like react-native-reanimated, can clone nodes in a different thread while react-native is calling setNativeProps_DEPRECATED. This results in a race condition, where a stale pointer to nativeProps_DEPRECATED can be accessed, resulting in a crash.
OrfeasZ
force-pushed
the
fix/clone-prop-update-race-condition
branch
from
67241d7 to
b350148
Compare
|
rn0.81.1 still has this issue, when will we merge it? |
|
@cipolleschi Hi! Could we have someone look at this? I think it could also be a problem for c++ animated. |
|
Hello, have any of you had any problems with the patch so far? [Edited - 22 Sep] |
|
I patched locally and released it to my users so far, so great, no more crashes 🎉 |
| * architecture and will be removed in the future. | ||
| */ | ||
| mutable std::unique_ptr<folly::dynamic> nativeProps_DEPRECATED; | ||
| mutable std::recursive_mutex nativePropsMutex; |
There was a problem hiding this comment.
Any reason this needs to be a recursive mutex instead of a plain one?
Does this need to be ShadowNodeFamily scoped, or would a single global mutex on UIManager be sufficient?
There was a problem hiding this comment.
I don't know enough about the internals of react native, so took the safe approach of using a recursive one to prevent deadlocks in the case the critical path ends up trying to acquire the lock again recursively. That said, if that can't happen, it could be replaced with a regular mutex.
As for scoping, I'm not sure (again due to lack of internals knowledge). The main issue this tries to address is concurrent writes / reads to the nativeProps_DEPRECATED field, which is why I added it there, trying to keep the locking exactly to where it's needed.
There was a problem hiding this comment.
Let's stick with a normal mutex here - I don't see any way this could be re-entrant.
|
@javache Could we get an update on this? Is there anything I should change so we can get this merged? |
|
@javache Could we get an update on this? We need this fix for our project. |
7 participants
Summary:
Some third-party libraries, like react-native-reanimated, can clone nodes in a different thread while react-native is calling
setNativeProps_DEPRECATED. This results in a race condition, where a stale pointer tonativeProps_DEPRECATEDcan be accessed, resulting in a crash. This usually manifests as aEXC_BAD_ACCESScrash on iOS. On Android it seems more rare. We've added a lock around accesses to nativeProps_DEPRECATED, but alternative options of fixing this can be considered too.For more information see software-mansion/react-native-reanimated#7666
Changelog:
[INTERNAL] [FIXED] - Fixed crashes caused by race conditions when third-party libraries clone the shadow dom from a different thread
Test Plan:
Due to this being a race condition that only manifests in rare circumstances, it's very difficult to create a reliable reproduction case. The issue mentioned above contains ThreadSanitizer logs that demonstrate this issue. TSan no longer complains with this patch applied, and we've not seen any additional issues from it after deploying it in production over the past week.