Skip to content

Commit 87ecc02

Browse files
acoates-msgmacmastershirakabaCopilot
authored
[0.81] Cherry picking Image use-after-free and make powershell lookup lazy (#16433)
* Fix use-after-free when an Image is destroyed mid-download (#11) (#16345) * Make the PowerShell path lookup lazy again, to stop spamming errors on macOS when react-native.config.js is loaded (#16430) * Defer PowerShell discovery until command or health check execution * Remove standalone PowerShell discovery tests * Fix conditional assignment for powershell variable Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * add try-catch * separate from try-catch * capitalisation --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix change files * format --------- Co-authored-by: Gordon MacMaster <31481849+gmacmaster@users.noreply.github.com> Co-authored-by: Jamie Birch <14055146+shirakaba@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
1 parent 6c65aae commit 87ecc02

5 files changed

Lines changed: 74 additions & 15 deletions

File tree

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"type": "patch",
3+
"comment": "Defer PowerShell discovery until a command or health check needs it so CLI configuration can load without Windows build tools.",
4+
"packageName": "@react-native-windows/cli",
5+
"email": "14055146+shirakaba@users.noreply.github.com",
6+
"dependentChangeType": "patch"
7+
}
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"type": "patch",
3+
"comment": "Fix use-after-free crash when an Image is destroyed while its download is still in flight",
4+
"packageName": "react-native-windows",
5+
"email": "gordomacmaster@gmail.com",
6+
"dependentChangeType": "patch"
7+
}

packages/@react-native-windows/cli/src/commands/healthCheck/healthChecks.ts

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import type {
1717
import {findPowerShell} from '@react-native-windows/find-dotnet-tools';
1818
import {HealthCheckList} from './healthCheckList';
1919

20-
const powershell = findPowerShell();
20+
let powershell: string | undefined;
2121

2222
export function getHealthChecks(): HealthCheckCategory[] | undefined {
2323
// #8471: There are known cases where the dependencies script will error out.
@@ -68,6 +68,7 @@ function getHealthChecksUnsafe(): HealthCheckCategory[] | undefined {
6868
getDiagnostics: async () => {
6969
let needsToBeFixed = true;
7070
try {
71+
powershell ??= findPowerShell();
7172
await execa(
7273
`"${powershell}" -ExecutionPolicy Unrestricted -NoProfile "${rnwDepScriptPath}" -NoPrompt -Check ${id}`,
7374
);
@@ -78,6 +79,21 @@ function getHealthChecksUnsafe(): HealthCheckCategory[] | undefined {
7879
};
7980
},
8081
runAutomaticFix: async ({loader, logManualInstallation}) => {
82+
try {
83+
powershell ??= findPowerShell();
84+
} catch (error) {
85+
const errorMessage =
86+
error instanceof Error ? error.message : undefined;
87+
logManualInstallation({
88+
healthcheck: `react-native-windows dependency "${id}"`,
89+
message: `Error finding PowerShell${
90+
errorMessage ? `: ${errorMessage}` : ''
91+
}`,
92+
});
93+
loader.fail();
94+
return;
95+
}
96+
8197
const command = `"${powershell}" -ExecutionPolicy Unrestricted -NoProfile "${rnwDepScriptPath}" -Check ${id}`;
8298
try {
8399
const {exitCode} = await execa(command, {stdio: 'inherit'});

packages/@react-native-windows/cli/src/utils/commandWithProgress.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ export function newSpinner(text: string) {
4848
return ora(options).start();
4949
}
5050

51-
const powershell = findPowerShell();
51+
let powershell: string | undefined;
5252

5353
export async function runPowerShellScriptFunction(
5454
taskDescription: string,
@@ -58,6 +58,8 @@ export async function runPowerShellScriptFunction(
5858
errorCategory: CodedErrorType,
5959
useAppxCompatibility = false,
6060
) {
61+
powershell ??= findPowerShell();
62+
6163
try {
6264
const printException = verbose ? '$_;' : '';
6365
const importAppx = useAppxCompatibility

vnext/Microsoft.ReactNative/Fabric/WindowsImageManager.cpp

Lines changed: 40 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,16 @@ facebook::react::ImageRequest WindowsImageManager::requestImage(
186186
auto weakObserverCoordinator = (std::weak_ptr<const facebook::react::ImageResponseObserverCoordinator>)
187187
imageRequest.getSharedObserverCoordinator();
188188

189+
// ImageResponseObserverCoordinator copies its observer list under a lock but dereferences the raw
190+
// observer pointers after releasing it. Observers are added and removed on the UI thread (from
191+
// ImageComponentView::setStateAndResubscribeImageResponseObserver), and that is also where the
192+
// owning ImageComponentView - and with it the WindowsImageResponseObserver - is destroyed. Notifying
193+
// the coordinator from the download/completion threads therefore races that teardown and can call
194+
// into a freed observer. Marshal every notification onto the UI thread so subscription and
195+
// notification are serialized on the same thread. Image decoding deliberately stays off the UI
196+
// thread; only the notification itself is posted.
197+
auto uiDispatcher = m_reactContext.UIDispatcher();
198+
189199
auto rnImageSource = winrt::Microsoft::ReactNative::Composition::implementation::MakeImageSource(imageSource);
190200
auto provider = m_uriImageManager->TryGetUriImageProvider(m_reactContext.Handle(), rnImageSource);
191201

@@ -202,45 +212,62 @@ facebook::react::ImageRequest WindowsImageManager::requestImage(
202212
source.sourceType = ImageSourceType::Download;
203213
source.body = imageSource.body;
204214

205-
auto progressCallback = [weakObserverCoordinator](int64_t loaded, int64_t total) {
206-
if (auto observerCoordinator = weakObserverCoordinator.lock()) {
207-
float progress = total > 0 ? static_cast<float>(loaded) / static_cast<float>(total) : 1.0f;
208-
observerCoordinator->nativeImageResponseProgress(progress, loaded, total);
209-
}
215+
auto progressCallback = [weakObserverCoordinator, uiDispatcher](int64_t loaded, int64_t total) {
216+
float progress = total > 0 ? static_cast<float>(loaded) / static_cast<float>(total) : 1.0f;
217+
uiDispatcher.Post([weakObserverCoordinator, progress, loaded, total]() {
218+
if (auto observerCoordinator = weakObserverCoordinator.lock()) {
219+
observerCoordinator->nativeImageResponseProgress(progress, loaded, total);
220+
}
221+
});
210222
};
211223
imageResponseTask = GetImageRandomAccessStreamAsync(source, progressCallback);
212224
}
213225

214-
imageResponseTask.Completed([weakObserverCoordinator](auto asyncOp, auto status) {
215-
auto observerCoordinator = weakObserverCoordinator.lock();
216-
if (!observerCoordinator) {
226+
imageResponseTask.Completed([weakObserverCoordinator, uiDispatcher](auto asyncOp, auto status) {
227+
if (weakObserverCoordinator.expired()) {
217228
return;
218229
}
219230

231+
auto postComplete = [weakObserverCoordinator, uiDispatcher](auto image) {
232+
uiDispatcher.Post([weakObserverCoordinator, image = std::move(image)]() {
233+
if (auto observerCoordinator = weakObserverCoordinator.lock()) {
234+
observerCoordinator->nativeImageResponseComplete(facebook::react::ImageResponse(image, nullptr /*metadata*/));
235+
}
236+
});
237+
};
238+
239+
auto postFailure = [weakObserverCoordinator,
240+
uiDispatcher](std::shared_ptr<facebook::react::ImageErrorInfo> errorInfo) {
241+
uiDispatcher.Post([weakObserverCoordinator, errorInfo = std::move(errorInfo)]() {
242+
if (auto observerCoordinator = weakObserverCoordinator.lock()) {
243+
observerCoordinator->nativeImageResponseFailed(facebook::react::ImageLoadError(errorInfo));
244+
}
245+
});
246+
};
247+
220248
switch (status) {
221249
case winrt::Windows::Foundation::AsyncStatus::Completed: {
222250
auto imageResponse = asyncOp.GetResults();
223251
auto selfImageResponse =
224252
winrt::get_self<winrt::Microsoft::ReactNative::Composition::implementation::ImageResponse>(imageResponse);
225253
auto imageResultOrError = selfImageResponse->ResolveImage();
226254
if (imageResultOrError.image) {
227-
observerCoordinator->nativeImageResponseComplete(
228-
facebook::react::ImageResponse(imageResultOrError.image, nullptr /*metadata*/));
255+
postComplete(std::move(imageResultOrError.image));
229256
} else {
230-
observerCoordinator->nativeImageResponseFailed(facebook::react::ImageLoadError(imageResultOrError.errorInfo));
257+
postFailure(std::move(imageResultOrError.errorInfo));
231258
}
232259
break;
233260
}
234261
case winrt::Windows::Foundation::AsyncStatus::Canceled: {
235262
auto errorInfo = std::make_shared<facebook::react::ImageErrorInfo>();
236263
errorInfo->error = FormatHResultError(winrt::hresult_error(asyncOp.ErrorCode()));
237-
observerCoordinator->nativeImageResponseFailed(facebook::react::ImageLoadError(errorInfo));
264+
postFailure(std::move(errorInfo));
238265
break;
239266
}
240267
case winrt::Windows::Foundation::AsyncStatus::Error: {
241268
auto errorInfo = std::make_shared<facebook::react::ImageErrorInfo>();
242269
errorInfo->error = FormatHResultError(winrt::hresult_error(asyncOp.ErrorCode()));
243-
observerCoordinator->nativeImageResponseFailed(facebook::react::ImageLoadError(errorInfo));
270+
postFailure(std::move(errorInfo));
244271
break;
245272
}
246273
}

0 commit comments

Comments
 (0)