Skip to content

Commit 7ed17c9

Browse files
Miklós Fazekasmeta-codesync[bot]
authored andcommitted
fix(image): fix data race in imageURLLoaderForURL: and imageDataDecoderForData: (#57297)
Summary: `imageURLLoaderForURL:` had a broken double-checked lock on `_loaders`: the outer `if (!_loaders)` guard and the `for` loop iteration both ran without `_loadersMutex`, while the assignment ran inside it. `imageDataDecoderForData:` had no lock at all on `_decoders`. Both methods write the ivar in two steps (raw list then sorted), so a concurrent reader could observe the intermediate value and iterate a concurrently-released array, crashing with `EXC_BAD_ACCESS` in `objc_release` on a GCD worker thread. Verified with a reproducer — crashes 5/5 without the fix, 0/10 with it: https://github.com/mfazekas/rn-rctimageloader-dcl-repro ## Fix Build the loader and decoder lists once in `setUp` (alongside the URL request queue), under `_setupLock` and gated by the existing `_didSetup` atomic flag. Every entry point calls `setUp` before reading the now write-once `_loaders` and `_decoders`, so a reader can never observe a half-built or concurrently-released array. This replaces the broken double-checked lock where the outer `if (!_loaders)` guard and the `for` loop iteration ran outside the mutex. ## Changelog: [iOS] [Fixed] - Fix a data race in `RCTImageLoader` loader and decoder lazy initialization that could crash with `EXC_BAD_ACCESS` ## Related Fixes #57296 See also #46115, #46153 Pull Request resolved: #57297 Reviewed By: javache Differential Revision: D109404243 Pulled By: fabriziocucci fbshipit-source-id: 359d4c70e2cbe1bc70a7e6a1dadc3e3b89c59ff4
1 parent 10ec4ad commit 7ed17c9

1 file changed

Lines changed: 60 additions & 63 deletions

File tree

packages/react-native/Libraries/Image/RCTImageLoader.mm

Lines changed: 60 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,8 @@ - (void)setReactDecodedImageBytes:(NSInteger)bytes
6868
@end
6969

7070
@implementation RCTImageLoader {
71-
std::atomic<BOOL> _isLoaderSetup;
72-
std::mutex _loaderSetupLock;
71+
std::atomic<BOOL> _didSetup;
72+
std::mutex _setupLock;
7373
NSArray<id<RCTImageURLLoader>> * (^_loadersProvider)(RCTModuleRegistry *);
7474
NSArray<id<RCTImageDataDecoder>> * (^_decodersProvider)(RCTModuleRegistry *);
7575
NSArray<id<RCTImageURLLoader>> *_loaders;
@@ -82,7 +82,6 @@ @implementation RCTImageLoader {
8282
NSMutableArray *_pendingDecodes;
8383
NSInteger _scheduledDecodes;
8484
NSUInteger _activeBytes;
85-
std::mutex _loadersMutex;
8685
__weak id<RCTImageRedirectProtocol> _redirectDelegate;
8786
}
8887

@@ -108,7 +107,7 @@ - (instancetype)initWithRedirectDelegate:(id<RCTImageRedirectProtocol>)redirectD
108107
{
109108
if (self = [super init]) {
110109
_redirectDelegate = redirectDelegate;
111-
_isLoaderSetup = NO;
110+
_didSetup = NO;
112111
}
113112
return self;
114113
}
@@ -126,15 +125,56 @@ - (instancetype)initWithRedirectDelegate:(id<RCTImageRedirectProtocol>)redirectD
126125

127126
- (void)setUp
128127
{
129-
std::lock_guard<std::mutex> guard(_loaderSetupLock);
130-
if (!_isLoaderSetup) {
128+
std::lock_guard<std::mutex> guard(_setupLock);
129+
if (!_didSetup) {
131130
// Set defaults
132131
_maxConcurrentLoadingTasks = _maxConcurrentLoadingTasks ?: 4;
133132
_maxConcurrentDecodingTasks = _maxConcurrentDecodingTasks ?: 2;
134133
_maxConcurrentDecodingBytes = _maxConcurrentDecodingBytes ?: 30 * 1024 * 1024; // 30MB
135134

136135
_URLRequestQueue = dispatch_queue_create("com.facebook.react.ImageLoaderURLRequestQueue", DISPATCH_QUEUE_SERIAL);
137-
_isLoaderSetup = YES;
136+
137+
// Get loaders, sorted in reverse priority order (highest priority first)
138+
if (_loadersProvider != nil) {
139+
_loaders = _loadersProvider(self.moduleRegistry);
140+
} else {
141+
RCTAssert(_bridge, @"Trying to find RCTImageURLLoaders and bridge not set.");
142+
_loaders = [_bridge modulesConformingToProtocol:@protocol(RCTImageURLLoader)];
143+
}
144+
_loaders =
145+
[_loaders sortedArrayUsingComparator:^NSComparisonResult(id<RCTImageURLLoader> a, id<RCTImageURLLoader> b) {
146+
float priorityA = [a respondsToSelector:@selector(loaderPriority)] ? [a loaderPriority] : 0;
147+
float priorityB = [b respondsToSelector:@selector(loaderPriority)] ? [b loaderPriority] : 0;
148+
if (priorityA > priorityB) {
149+
return NSOrderedAscending;
150+
} else if (priorityA < priorityB) {
151+
return NSOrderedDescending;
152+
} else {
153+
return NSOrderedSame;
154+
}
155+
}];
156+
157+
// Get decoders, sorted in reverse priority order (highest priority first)
158+
if (_decodersProvider != nil) {
159+
_decoders = _decodersProvider(self.moduleRegistry);
160+
} else {
161+
RCTAssert(_bridge, @"Trying to find RCTImageDataDecoders and bridge not set.");
162+
_decoders = [_bridge modulesConformingToProtocol:@protocol(RCTImageDataDecoder)];
163+
}
164+
_decoders = [_decoders
165+
sortedArrayUsingComparator:^NSComparisonResult(id<RCTImageDataDecoder> a, id<RCTImageDataDecoder> b) {
166+
float priorityA = [a respondsToSelector:@selector(decoderPriority)] ? [a decoderPriority] : 0;
167+
float priorityB = [b respondsToSelector:@selector(decoderPriority)] ? [b decoderPriority] : 0;
168+
if (priorityA > priorityB) {
169+
return NSOrderedAscending;
170+
} else if (priorityA < priorityB) {
171+
return NSOrderedDescending;
172+
} else {
173+
return NSOrderedSame;
174+
}
175+
}];
176+
177+
_didSetup = YES;
138178
}
139179
}
140180

@@ -163,41 +203,17 @@ - (void)setImageCache:(id<RCTImageCache>)cache
163203

164204
- (id<RCTImageURLLoader>)imageURLLoaderForURL:(NSURL *)URL
165205
{
166-
if (!_isLoaderSetup) {
206+
if (!_didSetup) {
167207
[self setUp];
168208
}
169209

170-
if (!_loaders) {
171-
std::unique_lock<std::mutex> guard(_loadersMutex);
172-
if (!_loaders) {
173-
// Get loaders, sorted in reverse priority order (highest priority first)
174-
if (_loadersProvider) {
175-
_loaders = _loadersProvider(self.moduleRegistry);
176-
} else {
177-
RCTAssert(_bridge, @"Trying to find RCTImageURLLoaders and bridge not set.");
178-
_loaders = [_bridge modulesConformingToProtocol:@protocol(RCTImageURLLoader)];
179-
}
180-
181-
_loaders =
182-
[_loaders sortedArrayUsingComparator:^NSComparisonResult(id<RCTImageURLLoader> a, id<RCTImageURLLoader> b) {
183-
float priorityA = [a respondsToSelector:@selector(loaderPriority)] ? [a loaderPriority] : 0;
184-
float priorityB = [b respondsToSelector:@selector(loaderPriority)] ? [b loaderPriority] : 0;
185-
if (priorityA > priorityB) {
186-
return NSOrderedAscending;
187-
} else if (priorityA < priorityB) {
188-
return NSOrderedDescending;
189-
} else {
190-
return NSOrderedSame;
191-
}
192-
}];
193-
}
194-
}
210+
NSArray<id<RCTImageURLLoader>> *loaders = _loaders;
195211

196212
if (RCT_DEBUG) {
197213
// Check for handler conflicts
198214
float previousPriority = 0;
199215
id<RCTImageURLLoader> previousLoader = nil;
200-
for (id<RCTImageURLLoader> loader in _loaders) {
216+
for (id<RCTImageURLLoader> loader in loaders) {
201217
float priority = [loader respondsToSelector:@selector(loaderPriority)] ? [loader loaderPriority] : 0;
202218
if (previousLoader && priority < previousPriority) {
203219
return previousLoader;
@@ -224,7 +240,7 @@ - (void)setImageCache:(id<RCTImageCache>)cache
224240
}
225241

226242
// Normal code path
227-
for (id<RCTImageURLLoader> loader in _loaders) {
243+
for (id<RCTImageURLLoader> loader in loaders) {
228244
if ([loader canLoadImageURL:URL]) {
229245
return loader;
230246
}
@@ -236,39 +252,17 @@ - (void)setImageCache:(id<RCTImageCache>)cache
236252

237253
- (id<RCTImageDataDecoder>)imageDataDecoderForData:(NSData *)data
238254
{
239-
if (!_isLoaderSetup) {
255+
if (!_didSetup) {
240256
[self setUp];
241257
}
242258

243-
if (!_decoders) {
244-
// Get decoders, sorted in reverse priority order (highest priority first)
245-
246-
if (_decodersProvider) {
247-
_decoders = _decodersProvider(self.moduleRegistry);
248-
} else {
249-
RCTAssert(_bridge, @"Trying to find RCTImageDataDecoders and bridge not set.");
250-
_decoders = [_bridge modulesConformingToProtocol:@protocol(RCTImageDataDecoder)];
251-
}
252-
253-
_decoders = [_decoders
254-
sortedArrayUsingComparator:^NSComparisonResult(id<RCTImageDataDecoder> a, id<RCTImageDataDecoder> b) {
255-
float priorityA = [a respondsToSelector:@selector(decoderPriority)] ? [a decoderPriority] : 0;
256-
float priorityB = [b respondsToSelector:@selector(decoderPriority)] ? [b decoderPriority] : 0;
257-
if (priorityA > priorityB) {
258-
return NSOrderedAscending;
259-
} else if (priorityA < priorityB) {
260-
return NSOrderedDescending;
261-
} else {
262-
return NSOrderedSame;
263-
}
264-
}];
265-
}
259+
NSArray<id<RCTImageDataDecoder>> *decoders = _decoders;
266260

267261
if (RCT_DEBUG) {
268262
// Check for handler conflicts
269263
float previousPriority = 0;
270264
id<RCTImageDataDecoder> previousDecoder = nil;
271-
for (id<RCTImageDataDecoder> decoder in _decoders) {
265+
for (id<RCTImageDataDecoder> decoder in decoders) {
272266
float priority = [decoder respondsToSelector:@selector(decoderPriority)] ? [decoder decoderPriority] : 0;
273267
if (previousDecoder && priority < previousPriority) {
274268
return previousDecoder;
@@ -297,7 +291,7 @@ - (void)setImageCache:(id<RCTImageCache>)cache
297291
}
298292

299293
// Normal code path
300-
for (id<RCTImageDataDecoder> decoder in _decoders) {
294+
for (id<RCTImageDataDecoder> decoder in decoders) {
301295
if ([decoder canDecodeImageData:data]) {
302296
return decoder;
303297
}
@@ -621,7 +615,7 @@ - (RCTImageURLLoaderRequest *)_loadImageOrDataWithURLRequest:(NSURLRequest *)req
621615
}
622616

623617
// All access to URL cache must be serialized
624-
if (!_isLoaderSetup) {
618+
if (!_didSetup) {
625619
[self setUp];
626620
}
627621

@@ -1040,7 +1034,7 @@ - (RCTImageLoaderCancellationBlock)decodeImageData:(NSData *)data
10401034
});
10411035
};
10421036

1043-
if (!_isLoaderSetup) {
1037+
if (!_didSetup) {
10441038
[self setUp];
10451039
}
10461040
dispatch_async(_URLRequestQueue, ^{
@@ -1173,6 +1167,9 @@ - (BOOL)canHandleRequest:(NSURLRequest *)request
11731167
return NO;
11741168
}
11751169

1170+
if (!_didSetup) {
1171+
[self setUp];
1172+
}
11761173
for (id<RCTImageURLLoader> loader in _loaders) {
11771174
// Don't use RCTImageURLLoader protocol for modules that already conform to
11781175
// RCTURLRequestHandler as it's inefficient to decode an image and then

0 commit comments

Comments
 (0)