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

[iOS] Static counters lead to data races #44715

Closed
hakonk opened this issue May 30, 2024 · 0 comments
Closed

[iOS] Static counters lead to data races #44715

hakonk opened this issue May 30, 2024 · 0 comments
Labels
Platform: iOS iOS applications. Resolution: Fixed A PR that fixes this issue has been merged.

Comments

@hakonk
Copy link
Contributor

hakonk commented May 30, 2024

Description

Some places there are static counter variables in the iOS code base. This can be problematic when different threads increment the counters. One such example is getNextImageRequestCount of RCTImageLoader:

static uint64_t getNextImageRequestCount(void)
{
  static uint64_t requestCounter = 0;
  return requestCounter++;
}

Another example is the initializer of RCTNetworkTask:

- (instancetype)initWithRequest:(NSURLRequest *)request
                        handler:(id<RCTURLRequestHandler>)handler
                  callbackQueue:(dispatch_queue_t)callbackQueue
{
  RCTAssertParam(request);
  RCTAssertParam(handler);
  RCTAssertParam(callbackQueue);

  static NSUInteger requestID = 0;

  if ((self = [super init])) {
    _requestID = @(requestID++); // <- problematic
    _request = request;
    _handler = handler;
    _callbackQueue = callbackQueue;
    _status = RCTNetworkTaskPending;

    dispatch_queue_set_specific(callbackQueue, (__bridge void *)self, (__bridge void *)self, NULL);
  }
  return self;
}

Other places in the code, this issue seems to have been addressed, e.g. RCTModuleData:

int32_t getUniqueId()
{
  static std::atomic<int32_t> counter{0};
  return counter++;
}

As shared counters seem to be employed several places in the code base, perhaps a common implementation should reside in the utils? C++ is not my forte, but perhaps one could do something like this:

template <typename T>
auto inline createSharedAtomicCounter(T initialValue) {
    static_assert(std::is_integral<T>::value, "T must be an integral type");
    auto value = std::make_shared<std::atomic<T>>(initialValue);
    return [value]() -> T {
        return (*value)++;
    };
}

This can be employed everywhere a shared counter is needed, and since it is generic, one can preserve the specific primitive types needed for each counter case.

Steps to reproduce

NB! Reproduction is tricky as other data races may occur:

  1. clone reproducer
  2. cd /Users/user/Documents/Dev/react-native/packages/rn-tester && npm i && cd ios && bundle exec pod install
  3. Open iOS workspace and enable TSan for scheme
  4. Enable runtime issue breakpoint
  5. Build and run the tester app and open the Image component from the list

React Native Version

0.74.1

Affected Platforms

Runtime - iOS

Output of npx react-native info

System:
  OS: macOS 14.5
  CPU: (12) arm64 Apple M2 Pro
  Memory: 381.03 MB / 32.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 20.13.1
    path: ~/.nvm/versions/node/v20.13.1/bin/node
  Yarn:
    version: 1.22.21
    path: ~/.nvm/versions/node/v20.13.1/bin/yarn
  npm:
    version: 10.5.2
    path: ~/.nvm/versions/node/v20.13.1/bin/npm
  Watchman:
    version: 2024.05.06.00
    path: /opt/homebrew/bin/watchman
Managers:
  CocoaPods:
    version: 1.14.2
    path: /Users/user/.rvm/gems/ruby-2.7.2/bin/pod
SDKs:
  iOS SDK:
    Platforms:
      - DriverKit 23.5
      - iOS 17.5
      - macOS 14.5
      - tvOS 17.5
      - visionOS 1.2
      - watchOS 10.5
  Android SDK:
    API Levels:
      - "26"
      - "33"
      - "34"
    Build Tools:
      - 26.0.3
      - 30.0.3
      - 33.0.1
      - 34.0.0
      - 35.0.0
    System Images:
      - android-26 | Google APIs Intel x86_64 Atom
      - android-34 | Google APIs ARM 64 v8a
    Android NDK: Not Found
IDEs:
  Android Studio: 2023.3 AI-233.14808.21.2331.11709847
  Xcode:
    version: 15.4/15F31d
    path: /usr/bin/xcodebuild
Languages:
  Java:
    version: 20.0.1
    path: /usr/bin/javac
  Ruby:
    version: 2.7.2
    path: /Users/user/.rvm/rubies/ruby-2.7.2/bin/ruby
npmPackages:
  "@react-native-community/cli": Not Found
  react: Not Found
  react-native: Not Found
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: false
  newArchEnabled: false
iOS:
  hermesEnabled: true
  newArchEnabled: true

Stacktrace or Logs

Unfortunately, I have some trouble reproducing this issue as other data races are caught. However, I believe the code examples I refer to are problematic in an of themselves since they demonstrate concurrent access of shared resources in code that is executed concurrently.

Reproducer

https://github.com/hakonk/react-native

Screenshots and Videos

No response

@github-actions github-actions bot added the Platform: iOS iOS applications. label May 30, 2024
facebook-github-bot pushed a commit that referenced this issue Jun 28, 2024
…tomic counters (#45114)

Summary:
In order to fix the data races described in #44715, I propose a simple solution by leveraging shared counter functions wherein `std::atomic` is the backing for the integer values.

## Changelog:

[iOS] [Fixed] - Implement shared atomic counters and replace static integers in `RCTImageLoader` and `RCTNetworkTask` that were accessed concurrently, which in some cases lead to data races.

Pull Request resolved: #45114

Test Plan: Added unit tests for the counters in `RCTSharedCounterTests`.

Reviewed By: cipolleschi

Differential Revision: D59155076

Pulled By: javache

fbshipit-source-id: f73afce6a816ad3226ed8c123cb2ccf4183549a0
@hakonk hakonk closed this as completed Jul 4, 2024
@cortinico cortinico added Resolution: Fixed A PR that fixes this issue has been merged. and removed Needs: Triage 🔍 labels Jul 4, 2024
blakef pushed a commit that referenced this issue Jul 15, 2024
…tomic counters (#45114)

Summary:
In order to fix the data races described in #44715, I propose a simple solution by leveraging shared counter functions wherein `std::atomic` is the backing for the integer values.

## Changelog:

[iOS] [Fixed] - Implement shared atomic counters and replace static integers in `RCTImageLoader` and `RCTNetworkTask` that were accessed concurrently, which in some cases lead to data races.

Pull Request resolved: #45114

Test Plan: Added unit tests for the counters in `RCTSharedCounterTests`.

Reviewed By: cipolleschi

Differential Revision: D59155076

Pulled By: javache

fbshipit-source-id: f73afce6a816ad3226ed8c123cb2ccf4183549a0
Titozzz pushed a commit that referenced this issue Jul 22, 2024
…tomic counters (#45114)

Summary:
In order to fix the data races described in #44715, I propose a simple solution by leveraging shared counter functions wherein `std::atomic` is the backing for the integer values.

## Changelog:

[iOS] [Fixed] - Implement shared atomic counters and replace static integers in `RCTImageLoader` and `RCTNetworkTask` that were accessed concurrently, which in some cases lead to data races.

Pull Request resolved: #45114

Test Plan: Added unit tests for the counters in `RCTSharedCounterTests`.

Reviewed By: cipolleschi

Differential Revision: D59155076

Pulled By: javache

fbshipit-source-id: f73afce6a816ad3226ed8c123cb2ccf4183549a0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: iOS iOS applications. Resolution: Fixed A PR that fixes this issue has been merged.
Projects
None yet
Development

No branches or pull requests

2 participants