Skip to content

Commit 86e3790

Browse files
authored
Mitigate stack overflow issue (#6224)
We see some very rare crashes where the stack is too small to support the WIL message formatting of `THROW_HR_MSG`. This change attempts to mitigate that through a few targeted reductions in stack usage (doesn't get a lot but these were the only high ROI locations found) and by probing the remaining stack to decide whether to include the exception message or not on the throw.
1 parent d0f25e0 commit 86e3790

6 files changed

Lines changed: 60 additions & 27 deletions

File tree

src/AppInstallerCommonCore/HttpClientHelper.cpp

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,20 @@ namespace AppInstaller::Http
267267
toThrow = HRESULT_FROM_WIN32(errorValue);
268268
}
269269

270-
THROW_HR_MSG(toThrow, "%hs", exception.what());
270+
// Ensure that sufficient stack space remains to include the message.
271+
// We have seen rare crashes due to running out of stack space in this path
272+
// so we will only include the message if there is enough space.
273+
// PrintLoggingMessage usage in WIL always creates a 4K stack buffer (2K of wchar_t),
274+
// so we leave some on top of that as well as the buffer is kept alive into further calls.
275+
static constexpr size_t s_msgMinimum = 5 * 1024;
276+
277+
if (Runtime::IsStackAvailable(s_msgMinimum))
278+
{
279+
THROW_HR_MSG(toThrow, "%hs", exception.what());
280+
}
281+
else
282+
{
283+
THROW_HR(toThrow);
284+
}
271285
}
272286
}

src/AppInstallerRepositoryCore/Public/winget/RepositorySource.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -272,7 +272,7 @@ namespace AppInstaller::Repository
272272
std::string GetIdentifier() const;
273273

274274
// Get the source's configuration details from settings.
275-
SourceDetails GetDetails() const;
275+
const SourceDetails& GetDetails() const;
276276

277277
// Get the source's information.
278278
SourceInformation GetInformation() const;

src/AppInstallerRepositoryCore/RepositorySource.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -621,7 +621,7 @@ namespace AppInstaller::Repository
621621
}
622622
}
623623

624-
SourceDetails Source::GetDetails() const
624+
const SourceDetails& Source::GetDetails() const
625625
{
626626
if (m_source)
627627
{

src/AppInstallerRepositoryCore/Rest/RestClient.cpp

Lines changed: 22 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -142,36 +142,34 @@ namespace AppInstaller::Repository::Rest
142142

143143
// Check the cache for a valid information entry
144144
RestInformationCache informationCache;
145-
std::optional<Schema::IRestClient::Information> cachedInformation = informationCache.Get(endpoint, customHeader, caller);
145+
std::optional<Schema::IRestClient::Information> result = informationCache.Get(endpoint, customHeader, caller);
146146

147-
if (cachedInformation)
147+
if (!result)
148148
{
149-
return std::move(cachedInformation).value();
150-
}
151-
152-
// Not in cache, make REST call to retrieve it
153-
auto headers = GetHeaders(customHeader, caller);
154-
CacheControlPolicy cacheControl;
155-
156-
std::optional<web::json::value> response = helper.HandleGet(
157-
endpoint,
158-
headers,
159-
{},
160-
[&](const web::http::http_response& httpResponse)
161-
{
162-
cacheControl = CacheControlPolicy{ httpResponse.headers().cache_control() };
163-
return Http::HttpClientHelper::HttpResponseHandlerResult{ std::nullopt, true };
164-
});
149+
// Not in cache, make REST call to retrieve it
150+
auto headers = GetHeaders(customHeader, caller);
151+
CacheControlPolicy cacheControl;
152+
153+
std::optional<web::json::value> response = helper.HandleGet(
154+
endpoint,
155+
headers,
156+
{},
157+
[&](const web::http::http_response& httpResponse)
158+
{
159+
cacheControl = CacheControlPolicy{ httpResponse.headers().cache_control() };
160+
return Http::HttpClientHelper::HttpResponseHandlerResult{ std::nullopt, true };
161+
});
165162

166-
THROW_HR_IF(APPINSTALLER_CLI_ERROR_UNSUPPORTED_RESTSOURCE, !response);
163+
THROW_HR_IF(APPINSTALLER_CLI_ERROR_UNSUPPORTED_RESTSOURCE, !response);
167164

168-
InformationResponseDeserializer responseDeserializer;
169-
auto result = responseDeserializer.Deserialize(response.value());
165+
InformationResponseDeserializer responseDeserializer;
166+
result = responseDeserializer.Deserialize(response.value());
170167

171-
// Cache the information value as requested
172-
informationCache.Cache(endpoint, customHeader, caller, cacheControl, std::move(response).value());
168+
// Cache the information value as requested
169+
informationCache.Cache(endpoint, customHeader, caller, cacheControl, std::move(response).value());
170+
}
173171

174-
return result;
172+
return std::move(result).value();
175173
}
176174

177175
std::unique_ptr<Schema::IRestClient> RestClient::GetSupportedInterface(

src/AppInstallerSharedLib/Public/winget/Runtime.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,10 @@ namespace AppInstaller::Runtime
5151
// 3. the token is not already elevated
5252
bool IsRunningWithLimitedToken();
5353

54+
// Determines if the given amount of stack bytes are available.
55+
// If the answer cannot be determined properly, the return value will be `false`.
56+
DECLSPEC_NOINLINE bool IsStackAvailable(size_t bytes);
57+
5458
// Returns true if this is a release build; false if not.
5559
inline constexpr bool IsReleaseBuild()
5660
{

src/AppInstallerSharedLib/Runtime.cpp

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -218,4 +218,21 @@ namespace AppInstaller::Runtime
218218
{
219219
return wil::get_token_information<TOKEN_ELEVATION_TYPE>() == TokenElevationTypeLimited;
220220
}
221+
222+
DECLSPEC_NOINLINE bool IsStackAvailable(size_t bytes)
223+
{
224+
// https://devblogs.microsoft.com/oldnewthing/20200610-00/?p=103855
225+
ULONG_PTR low, high;
226+
GetCurrentThreadStackLimits(&low, &high);
227+
auto remaining = reinterpret_cast<ULONG_PTR>(&low) - low;
228+
if (remaining > high - low)
229+
{
230+
// Choosing to return false instead of failing
231+
return false;
232+
}
233+
234+
ULONG guarantee = 0;
235+
SetThreadStackGuarantee(&guarantee);
236+
return remaining >= bytes + guarantee;
237+
}
221238
}

0 commit comments

Comments
 (0)