Skip to content

Commit b669184

Browse files
shoumikhinshoumikhin
authored
Use a local strong NSError for methods with nested autorelease pool (#10465)
Otherwise, the out `NSError` arg get assigned an autoreleasing `NSError` object that gets deallocated when the nested autorelease pool is drained and no error is reported: ```objectivec - (BOOL)foo:(NSError **)error { if (error) { // Creates an autoreleased error object. *error = [NSError errorWithDomain:@"DemoDomain" code:42 userInfo:nil]; } return NO; } - (BOOL)bar:(NSError **)error { @autoreleasepool { // foo: returns NO, sets *error to an autoreleased NSError. // But as soon as we leave this block, that NSError is released. return [self foo:error]; } } - (void)run { NSError *error = nil; if (![self bar:&error]) { // Crash here even before entering the if-statement below due to `objc_storeStrong` call on the autoreleased error out arg. // Even if we manage to get here by marking the error var as __autoreleasing above, // Error now points at freed memory, so we get another crash or garbage. NSLog(@"❌ Faulty: error code = %ld", (long)error.code); } } ``` We introduce a local error var to hold onto the autoreleased `NSError` created by some other API like `foo` and use it to initialize the out `NSError` arg: ```objectivec - (BOOL)bar:(NSError **)error { NSError *localError = nil; BOOL ok = NO; @autoreleasepool { ok = [self foo:&localError]; // ARC retains localError here when assigning the autoreleased object. } if (!ok && error) { *error = localError; // Safe - localError survived the pool drain. } return ok; } - (void)run { NSError *error = nil; if (![self bar:&error]) { if (error) { // Don't forget to still check the error var. // Now this reliably logs “42” NSLog(@"✅ Fixed: error code = %ld", (long)error.code); } } } ``` Another important observation is that generally it's required to check if the out `NSError` has been set despite the returned status. And a more trickier crash happens even before the control reaches error handling. The compiler assumes the error local var in the `run` method is `__strong`, so it generates `objc_storeStrong` call. Whereas in fact it points to an `__autoreleasing` object (that gets deallocated when the pool drains as we clarified), and thus such a call on it will lead to a crash regardless, we won't even reach the code that tries to access `code` on it. Unless we indeed introduce a local strong var inside `bar` to be assigned to the out `NSError` arg and so make the generated `objc_storeStrong` call a legit one. Although, that issue can technically be resolved differently - by marking `error` var in `run` as `__autoreleasing` to skip the `objc_storeStrong` call. But that still doesn't help reporting the actual error, since it's gonna be nil after the nested autorelease pool drainage inside `bar`. And apparently having the nested autorelease pool is important enough, so we can't avoid it and have to use such workarounds.
1 parent f3e8972 commit b669184

File tree

4 files changed

+98
-79
lines changed

4 files changed

+98
-79
lines changed

‎backends/apple/coreml/runtime/delegate/ETCoreMLModelManager.mm‎

Lines changed: 48 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -711,106 +711,109 @@ - (BOOL)executeModelWithHandle:(ModelHandle *)handle
711711
loggingOptions:(const executorchcoreml::ModelLoggingOptions&)loggingOptions
712712
eventLogger:(const executorchcoreml::ModelEventLogger* _Nullable)eventLogger
713713
error:(NSError * __autoreleasing *)error {
714+
BOOL result = NO;
714715
id<ETCoreMLModelExecutor> executor = [self executorWithHandle:handle];
715716
if (!executor) {
716717
ETCoreMLLogErrorAndSetNSError(error,
717718
ETCoreMLErrorInternalError,
718719
"Model is already unloaded.");
719-
return NO;
720+
return result;
720721
}
721-
722+
722723
ETCoreMLModel *model = executor.model;
723724
if (args.count != model.orderedInputNames.count + model.orderedOutputNames.count) {
724725
ETCoreMLLogErrorAndSetNSError(error,
725726
ETCoreMLErrorCorruptedModel,
726727
"Model is invalid, expected args count to be %lu but got %lu.",
727728
static_cast<unsigned long>(model.orderedInputNames.count + model.orderedOutputNames.count),
728729
args.count);
729-
return NO;
730+
return result;
730731
}
732+
NSError *localError = nil;
731733
@autoreleasepool {
732734
NSArray<MLMultiArray *> *inputs = [args subarrayWithRange:NSMakeRange(0, model.orderedInputNames.count)];
733735
NSArray<MLMultiArray *> *outputs = [args subarrayWithRange:NSMakeRange(model.orderedInputNames.count, args.count - model.orderedInputNames.count)];
734736
NSArray<MLMultiArray *> *outputBackings = @[];
735737
if (executor.ignoreOutputBackings == NO) {
736738
outputBackings = outputs;
737739
}
738-
739740
NSArray<MLMultiArray *> *modelOutputs = [self executeModelUsingExecutor:executor
740741
inputs:inputs
741742
outputBackings:outputBackings
742743
loggingOptions:loggingOptions
743744
eventLogger:eventLogger
744-
error:error];
745-
if (!modelOutputs) {
746-
return NO;
745+
error:&localError];
746+
if (modelOutputs) {
747+
::set_outputs(outputs, modelOutputs);
748+
result = YES;
747749
}
748-
749-
::set_outputs(outputs, modelOutputs);
750750
}
751-
752-
return YES;
751+
if (!result) {
752+
if (error) {
753+
*error = localError;
754+
}
755+
}
756+
return result;
753757
}
754758

755759
- (BOOL)executeModelWithHandle:(ModelHandle *)handle
756760
argsVec:(std::vector<executorchcoreml::MultiArray>&)argsVec
757761
loggingOptions:(const executorchcoreml::ModelLoggingOptions&)loggingOptions
758762
eventLogger:(const executorchcoreml::ModelEventLogger* _Nullable)eventLogger
759763
error:(NSError * __autoreleasing *)error {
764+
BOOL result = NO;
760765
id<ETCoreMLModelExecutor> executor = [self executorWithHandle:handle];
761766
if (!executor) {
762767
ETCoreMLLogErrorAndSetNSError(error,
763768
ETCoreMLErrorInternalError,
764769
"Model is already unloaded.");
765-
return NO;
770+
return result;
766771
}
767-
768772
ETCoreMLModel *model = executor.model;
769773
if (argsVec.size() != model.orderedInputNames.count + model.orderedOutputNames.count) {
770774
ETCoreMLLogErrorAndSetNSError(error,
771775
ETCoreMLErrorCorruptedModel,
772776
"Model is invalid, expected args count to be %lu but got %lu.",
773777
static_cast<unsigned long>(model.orderedInputNames.count + model.orderedOutputNames.count),
774778
argsVec.size());
775-
return NO;
779+
return result;
776780
}
777-
778781
std::vector<executorchcoreml::MultiArray> inputArgs(argsVec.begin(), argsVec.begin() + model.orderedInputNames.count);
779782
std::vector<executorchcoreml::MultiArray> outputArgs(argsVec.begin() + model.orderedInputNames.count, argsVec.end());
783+
NSError *localError = nil;
780784
@autoreleasepool {
781-
NSArray<MLMultiArray *> *inputs = [model prepareInputs:inputArgs error:error];
782-
if (!inputs) {
783-
return NO;
784-
}
785-
786-
NSArray<MLMultiArray *> *outputBackings = @[];
787-
if (executor.ignoreOutputBackings == NO) {
788-
outputBackings = [model prepareOutputBackings:outputArgs error:error];
789-
}
790-
791-
if (!outputBackings) {
792-
return NO;
793-
}
794-
795-
NSArray<MLMultiArray *> *modelOutputs = [self executeModelUsingExecutor:executor
796-
inputs:inputs
797-
outputBackings:outputBackings
798-
loggingOptions:loggingOptions
799-
eventLogger:eventLogger
800-
error:error];
801-
if (!modelOutputs) {
802-
return NO;
785+
NSArray<MLMultiArray *> *inputs = [model prepareInputs:inputArgs error:&localError];
786+
if (inputs) {
787+
NSArray<MLMultiArray *> *outputBackings = @[];
788+
if (executor.ignoreOutputBackings == NO) {
789+
outputBackings = [model prepareOutputBackings:outputArgs error:&localError];
790+
}
791+
if (outputBackings) {
792+
NSArray<MLMultiArray *> *modelOutputs = [self executeModelUsingExecutor:executor
793+
inputs:inputs
794+
outputBackings:outputBackings
795+
loggingOptions:loggingOptions
796+
eventLogger:eventLogger
797+
error:&localError];
798+
if (modelOutputs) {
799+
// Resize for dynamic shapes
800+
for (int i = 0; i < outputArgs.size(); i++) {
801+
auto new_size = to_vector<size_t>(modelOutputs[i].shape);
802+
outputArgs[i].resize(new_size);
803+
argsVec[model.orderedInputNames.count + i].resize(new_size);
804+
}
805+
::set_outputs(outputArgs, modelOutputs);
806+
result = YES;
807+
}
808+
}
803809
}
804-
805-
// Resize for dynamic shapes
806-
for (int i = 0; i < outputArgs.size(); i++) {
807-
auto new_size = to_vector<size_t>(modelOutputs[i].shape);
808-
outputArgs[i].resize(new_size);
809-
argsVec[model.orderedInputNames.count + i].resize(new_size);
810+
}
811+
if (!result) {
812+
if (error) {
813+
*error = localError;
810814
}
811-
::set_outputs(outputArgs, modelOutputs);
812-
return YES;
813815
}
816+
return result;
814817
}
815818

816819
- (BOOL)unloadModelWithHandle:(ModelHandle *)handle {

‎backends/apple/coreml/runtime/delegate/MLModel_Prewarm.mm‎

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -107,26 +107,29 @@ + (MLMultiArray *)zeroedMultiArrayWithShape:(NSArray<NSNumber *> *)shape
107107
@implementation MLModel (Prewarm)
108108

109109
- (BOOL)prewarmUsingState:(nullable id)state error:(NSError * __autoreleasing *)error {
110+
NSError *localError = nil;
111+
BOOL result = NO;
110112
@autoreleasepool {
111-
id<MLFeatureProvider> inputs = ::get_zeroed_inputs(self, error);
112-
if (!inputs) {
113-
return NO;
114-
}
115-
116-
117-
id<MLFeatureProvider> outputs = nil;
118-
if (state != nil) {
113+
id<MLFeatureProvider> inputs = ::get_zeroed_inputs(self, &localError);
114+
if (inputs) {
115+
id<MLFeatureProvider> outputs = nil;
116+
if (state) {
119117
#if MODEL_STATE_IS_SUPPORTED
120-
if (@available(macOS 15.0, iOS 18.0, tvOS 18.0, watchOS 11.0, *)) {
121-
outputs = [self predictionFromFeatures:inputs usingState:(MLState *)state error:error];
122-
return outputs != nil;
123-
}
118+
if (@available(macOS 15.0, iOS 18.0, tvOS 18.0, watchOS 11.0, *)) {
119+
outputs = [self predictionFromFeatures:inputs usingState:(MLState *)state error:&localError];
120+
}
124121
#endif
122+
}
123+
if (!outputs) {
124+
outputs = [self predictionFromFeatures:inputs error:&localError];
125+
}
126+
result = outputs != nil;
125127
}
126-
127-
outputs = [self predictionFromFeatures:inputs error:error];
128-
return outputs != nil;
129128
}
129+
if (!result && error) {
130+
*error = localError;
131+
}
132+
return result;
130133
}
131134

132135

‎backends/apple/coreml/runtime/sdk/ETCoreMLModelAnalyzer.mm‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,8 @@ - (nullable instancetype)initWithCompiledModelAsset:(ETCoreMLAsset *)compiledMod
138138
if (!self.debugger) {
139139
return nil;
140140
}
141-
141+
142+
NSError *localError = nil;
142143
NSArray<MLMultiArray *> *modelOutputs = nil;
143144
NSArray<ETCoreMLModelStructurePath *> *operationPaths = self.debugger.operationPaths;
144145
NSDictionary<ETCoreMLModelStructurePath *, NSString *> *operationPathToDebugSymbolMap = self.debugger.operationPathToDebugSymbolMap;
@@ -150,8 +151,11 @@ - (nullable instancetype)initWithCompiledModelAsset:(ETCoreMLAsset *)compiledMod
150151
options:predictionOptions
151152
inputs:inputs
152153
modelOutputs:&modelOutputs
153-
error:error];
154+
error:&localError];
154155
if (!outputs) {
156+
if (error) {
157+
*error = localError;
158+
}
155159
return nil;
156160
}
157161

‎backends/apple/coreml/runtime/sdk/ETCoreMLModelDebugger.mm‎

Lines changed: 26 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -661,37 +661,46 @@ - (nullable ETCoreMLAsset *)compiledModelAssetWithOutputsAtPaths:(NSArray<ETCore
661661

662662
- (nullable NSArray<DebuggableModel *> *)modelsWithOutputsOfOperationsAtPath:(NSArray<ETCoreMLModelStructurePath *> *)paths
663663
error:(NSError* __autoreleasing *)error {
664+
NSError *localError = nil;
665+
NSArray<DebuggableModel *> *result = nil;
664666
@autoreleasepool {
665-
return [self _modelsWithOutputsOfOperationsAtPath:paths error:error];
667+
result = [self _modelsWithOutputsOfOperationsAtPath:paths error:&localError];
668+
}
669+
if (!result && error) {
670+
*error = localError;
666671
}
672+
return result;
667673
}
668674

669675
- (nullable ETCoreMLModelOutputs *)outputsOfOperationsAtPaths:(NSArray<ETCoreMLModelStructurePath *> *)paths
670676
options:(MLPredictionOptions *)options
671677
inputs:(id<MLFeatureProvider>)inputs
672678
modelOutputs:(NSArray<MLMultiArray *> *_Nullable __autoreleasing *_Nonnull)modelOutputs
673679
error:(NSError* __autoreleasing *)error {
674-
NSArray<MLMultiArray *> *lModelOutputs = nil;
675-
NSMutableDictionary<ETCoreMLModelStructurePath *, MLMultiArray *> *result = [NSMutableDictionary dictionaryWithCapacity:paths.count];
680+
NSError *localError = nil;
681+
BOOL success = NO;
682+
NSArray<MLMultiArray *> *localModelOutputs = nil;
683+
ETCoreMLModelOutputs *result = [NSMutableDictionary dictionaryWithCapacity:paths.count];
676684
@autoreleasepool {
677-
NSArray<DebuggableModel *> *models = [self modelsWithOutputsOfOperationsAtPath:paths error:error];
678-
if (!models) {
679-
return nil;
680-
}
681-
682-
for (DebuggableModel *pair in models) {
683-
id<MLFeatureProvider> outputFeatures = [pair.first predictionFromFeatures:inputs options:options error:error];
684-
set_intermediate_outputs(outputFeatures, paths, result);
685-
if (modelOutputs) {
686-
set_model_outputs(outputFeatures, self.outputNames, &lModelOutputs);
687-
}
685+
NSArray<DebuggableModel *> *models = [self modelsWithOutputsOfOperationsAtPath:paths error:&localError];
686+
success = models != nil;
687+
if (success) {
688+
for (DebuggableModel *pair in models) {
689+
id<MLFeatureProvider> outputFeatures = [pair.first predictionFromFeatures:inputs options:options error:&localError];
690+
set_intermediate_outputs(outputFeatures, paths, result);
691+
if (modelOutputs) {
692+
set_model_outputs(outputFeatures, self.outputNames, &localModelOutputs);
693+
}
694+
}
688695
}
689696
}
690-
697+
if (!success && error) {
698+
*error = localError;
699+
return nil;
700+
}
691701
if (modelOutputs) {
692-
*modelOutputs = lModelOutputs;
702+
*modelOutputs = localModelOutputs;
693703
}
694-
695704
return result;
696705
}
697706

0 commit comments

Comments
 (0)