A customer had a program that crashed with this stack:<\/p>\n
contoso!Widget::GetCost\r\ncontoso!StandardWidgets::get_TotalCost+0x12f\r\nrpcrt4!Invoke+0x73\r\nrpcrt4!Ndr64StubWorker+0xb9b\r\nrpcrt4!NdrStubCall3+0xd7\r\ncombase!CStdStubBuffer_Invoke+0xdb\r\ncombase!ObjectMethodExceptionHandlingAction<<lambda_...> >+0x47\r\ncombase!DefaultStubInvoke+0x376\r\ncombase!ServerCall::ContextInvoke+0x6f3\r\ncombase!ComInvokeWithLockAndIPID+0xacb\r\ncombase!ThreadInvoke+0x103\r\nrpcrt4!DispatchToStubInCNoAvrf+0x18\r\nrpcrt4!RPC_INTERFACE::DispatchToStubWorker+0x1a9\r\nrpcrt4!RPC_INTERFACE::DispatchToStubWithObject+0x1a7\r\nrpcrt4!LRPC_SCALL::DispatchRequest+0x308\r\nrpcrt4!LRPC_SCALL::HandleRequest+0xdcb\r\nrpcrt4!LRPC_SASSOCIATION::HandleRequest+0x2c3\r\nrpcrt4!LRPC_ADDRESS::HandleRequest+0x183\r\nrpcrt4!LRPC_ADDRESS::ProcessIO+0x939\r\nrpcrt4!LrpcIoComplete+0xff\r\nntdll!TppAlpcpExecuteCallback+0x14d\r\nntdll!TppWorkerThread+0x4b4\r\nkernel32!BaseThreadInitThunk+0x18\r\nntdll!RtlUserThreadStart+0x21\r\n<\/pre>\nThey wondered if some recent change to Windows was the source of the problem, since it didn’t happen as much in earlier versions of Windows.<\/p>\n
The stack trace pointed to
Widget::IsEnabled<\/code>, which was crashing on the first instruction because it was given an invalid this<\/code> pointer.<\/p>\n00007fff`73a8a59f mov edx,dword ptr [rcx+40h]\r\n ds:00000000`00000040=????????\r\n<\/pre>\nThe
Widget<\/code> pointer came from astd::vector<\/code> that is a member of theStandard\u00adWidgets<\/code> class.<\/p>\nusing namespace Microsoft::WRL;\r\n\r\nclass StandardWidgets : RuntimeClass<IStandardWidgets, FtmBase>\r\n{\r\n IFACEMETHODIMP get_TotalCost(INT32* result);\r\n \u27e6 other methods not relevant here \u27e7\r\n\r\nprivate:\r\n HRESULT LazyInitializeWidgetList();\r\n\r\n static constexpr PCWSTR standardWidgetNames[] = {\r\n L\"Bob\", L\"Carol\", L\"Ted\", L\"Alice\" };\r\n static constexpr int standardWidgetCount =\r\n ARRAYSIZE(standardWidgetNames);\r\n\r\n std::vector<ComPtr<Widget>> m_widgets;\r\n};\r\n<\/pre>\nThe code crashed at this call to
Widget::GetCost<\/code>:<\/p>\n IFACEMETHODIMP StandardWidgets::get_TotalCost(INT32* result)\r\n{\r\n *result = 0;\r\n\r\n RETURN_IF_FAILED(LazyInitializeWidgetList());\r\n\r\n INT32 totalCost = 0;\r\n for (int i = 0; i < standardWidgetCount; i++) {\r\n totalCost += m_widgets[i]->GetCost()<\/span>; \/\/ here\r\n }\r\n *result = totalCost;\r\n return S_OK;\r\n}\r\n<\/pre>\nThe customer’s debugging showed that at the point of the crash, not only was the
widget<\/code> garbage, but them_widgets<\/code> vector had an impossibly large number of elements. Them_widgets<\/code> is expected to have only four widgets, but it somehow found itself with ten, and sometimes as many as a hundred widgets. Of course, they were nearly all corrupted.<\/p>\nHere’s the code that lazy-initializes the widget list:<\/p>\n
HRESULT StandardWidgets::LazyInitializeWidgetList()\r\n{\r\n \/\/ Early-out if already initialized.\r\n if (!m_widgets.empty()) {\r\n return S_OK;\r\n }\r\n\r\n \/\/ Lazy-create the vector of standard widgets\r\n try {\r\n m_widgets.reserve(standardWidgetCount);\r\n for (auto name : standardWidgetNames) {\r\n ComPtr<Widget> widget;\r\n RETURN_IF_FAILED(\r\n MakeAndInitialize<Widget>(&widget, name));\r\n m_widgets.push_back(widget);\r\n }\r\n } catch (std::bad_alloc const&) {\r\n return E_OUTOFMEMORY;\r\n }\r\n return S_OK;\r\n}\r\n<\/pre>\nThe customer noted that the
reserve<\/code> method is always called with the value 4, and the code never pushes more than four items into the vector. They admitted that if there is a problem creating all four of the standard widgets, the vector could end up with fewer than four widgets, but it should never have more<\/i> than four.<\/p>\nYou already have multiple clues that point toward what the customer’s problem is. I’ll give you some time to think about it.<\/p>\n
In the meantime, let’s look at other issues with how the code lazy-initializes the widget list.<\/p>\n
As the customer noted, if there is a problem creating any of the four standard widgets, the failure is propagated to the caller of
Lazy\u00adInitialize\u00adWidget\u00adList<\/code>, andget_TotalCost<\/code> in turn propagates the error to its own caller, and it never gets to the point where it walks through the vector adding up all the costs.<\/p>\nIf there is a memory allocation failure at the
reserve()<\/code>, or if there is a problem with the first standard widget, then the vector remains empty, and a second call toLazy\u00adInitialize\u00adWidget\u00adList<\/code> will make a new attempt at initialization.<\/p>\nIf there is a problem with the second or subsequent standard widget, however, things get weird. The
Lazy\u00adInitialize\u00adWidget\u00adList<\/code> function returns a failure, which causesget_TotalCost<\/code> to return failure. But the second time someone callsget_TotalCost<\/code>,Lazy\u00adInitialize\u00adWidget\u00adList<\/code> will see a nonempty vector and assume that everything was initialized. This time, theget_TotalCost<\/code> method will proceed with the summation and perform an out-of-bounds array access when it gets to the widget that failed to be created.<\/p>\nOops.<\/p>\n
This particular problem boils down to leaving a partially-initialized
m_widgets<\/code> if the lazy initialization fails. To avoid this problem, we should create the vector in a local variable and transfer it to the member variable only after we are sure all of the widgets were created successfully.<\/p>\nHRESULT StandardWidgets::LazyInitializeWidgetList()\r\n{\r\n \/\/ Early-out if already initialized.\r\n if (!m_widgets.empty()) {\r\n return S_OK;\r\n }\r\n\r\n \/\/ Lazy-create the vector of standard widgets\r\n try {\r\n std::vector<ComPtr<Widget>> widgets;<\/span>\r\n widgets<\/span>.reserve(standardWidgetCount);\r\n for (auto name : standardWidgetNames) {\r\n ComPtr<Widget> widget;\r\n RETURN_IF_FAILED(\r\n MakeAndInitialize<Widget>(&widget, name));\r\n widgets<\/span>.push_back(widget);\r\n }\r\n m_widgets.swap(widgets);<\/span>\r\n } catch (std::bad_alloc const&) {\r\n return E_OUTOFMEMORY;\r\n }\r\n return S_OK;\r\n}\r\n<\/pre>\nThis ensures that the
m_widgets<\/code> is either totally empty or totally initialized. It is never in a half-initialized state.<\/p>\nWhile we’re at it, we probably should convert the loop in
get_TotalCost<\/code> into a rangedfor<\/code> loop. Right now,get_Total\u00adCost<\/code> has a hidden dependency on Lazy\u00adInitialize\u00adWidgets<\/code>: It assumes thatLazy\u00adInitialize\u00adWidgets<\/code> always creates exactly the number of widgets as there arestandard\u00adWidget\u00adNames<\/code>. Maybe in the future, you might want to suppress some of the standard widgets based on some configuration setting. If you add that configuration setting and forget to updateget_Total\u00adCost<\/code> to account for suppressed widgets, you will have an out-of-bounds index. All the logic to decide which widgets are standard should be local to Lazy\u00adInitialize\u00adWidgets<\/code>.<\/p>\nIFACEMETHODIMP StandardWidgets::get_TotalCost(INT32* result)\r\n{\r\n *result = 0;\r\n\r\n RETURN_IF_FAILED(LazyInitializeWidgetList());\r\n\r\n INT32 totalCost = 0;\r\n for (auto&& widget : m_widgets) { <\/span>\r\n totalCost += widget->GetCost();<\/span>\r\n } <\/span>\r\n *result = totalCost;\r\n return S_OK;\r\n}\r\n<\/pre>\nOr if you want to get fancy,<\/p>\n
IFACEMETHODIMP StandardWidgets::get_TotalCost(INT32* result)\r\n{\r\n *result = 0;\r\n\r\n RETURN_IF_FAILED(LazyInitializeWidgetList());\r\n\r\n *result = std::transform_reduce( <\/span>\r\n m_widgets.begin(), m_widgets.end(), 0, std::plus<>(),<\/span>\r\n [](auto&& w) { return w->GetCost(); }); <\/span>\r\n return S_OK;\r\n}\r\n<\/pre>\nOkay, but back to the crash. I think I’ve added enough filler to give you time to consider what is happening.<\/p>\n