{"id":108348,"date":"2023-06-16T07:00:00","date_gmt":"2023-06-16T14:00:00","guid":{"rendered":"https:\/\/devblogs.microsoft.com\/oldnewthing\/?p=108348"},"modified":"2023-06-16T07:04:59","modified_gmt":"2023-06-16T14:04:59","slug":"20230616-00","status":"publish","type":"post","link":"https:\/\/devblogs.microsoft.com\/oldnewthing\/20230616-00\/?p=108348","title":{"rendered":"The case of the invalid handle despite being managed by an RAII type, part 2"},"content":{"rendered":"

Last time, we investigated the case of the invalid handle despite being managed by an RAII type<\/a>, and we identified that the problem was a use-after-free. So how do we fix the problem?<\/p>\n

The minimum fix is to stop the tracker and wait for it to be destroyed before assessing the results.<\/p>\n

  \/\/ Stop the tracker and wait for it to be destroyed.\r\n  tracker.reset();\r\n  results.WaitForDestroyed();\r\n  \u21c5<\/span>\r\n  \/\/ Fail if no qualifying widget was found\r\n  THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_NOT_FOUND),\r\n              results.GetWidgetId() == INVALID_WIDGET_ID);\r\n<\/pre>\n
This fixes the problem, but it’s a band-aid. Really, we should make this type of mistake harder to make in the first place.<\/p>\n
One idea is to put the destroyedEvent.wait()<\/code> in the First\u00adWidget\u00adResults<\/code> destructor, so that no matter what happens, we always wait for the tracker to finish its callbacks. But we have to be careful to do this only if the tracker was started<\/i>. So maybe we can do this:<\/p>\n
struct FirstWidgetResults\r\n{\r\n  ~FirstWidgetResults()                <\/span>\r\n  {                                    <\/span>\r\n    if (started) destroyedEvent.wait();<\/span>\r\n  }                                    <\/span>\r\n\r\n  int GetWidgetId() { return widgetId; }\r\n  void WaitForResults()\r\n  {\r\n    started = true;<\/span>\r\n    readyEvent.wait();\r\n  }\r\n  \/\/ void WaitForDestroyed() { destroyedEvent.wait(); }<\/del><\/span>\r\n\r\n  static void CALLBACK OnCallback(\r\n      int reason, int id, void* context)\r\n  {\r\n    auto self = (MyTrackerResults*)context;\r\n\r\n    switch (reason) {\r\n    case WidgetFound:\r\n      if (self->widgetId == INVALID_WIDGET_ID) {\r\n        self->widgetId = id;\r\n        self->readyEvent.Set();\r\n      }\r\n      break;\r\n        \r\n    case InitialScanComplete:\r\n      self->readyEvent.Set();\r\n      break;\r\n\r\n    case TrackerDestroyed:\r\n      self->destroyedEvent.Set();\r\n      break;\r\n    }\r\n  }\r\n\r\nprivate:\r\n  int widgetId = INVALID_WIDGET_ID;\r\n  bool started = false;<\/span>\r\n\r\n  wil::unique_event readyEvent{ wil::EventOptions::None };\r\n  wil::unique_event destroyedEvent{ wil::EventOptions::None };\r\n};\r\n\r\nWidget FindWidgetWithEnabledAdapter()\r\n{\r\n  TrackerOptions options;\r\n  \/* ... code to set the options ... *\/\r\n\r\n  FirstWidgetResults results;\r\n\r\n  \/\/ Start the tracker (or die trying)\r\n  unique_tracker tracker;\r\n  THROW_IF_FAILED(CreateTracker(\r\n        &options,\r\n        FirstWidgetResults::OnCallback,\r\n        &results,\r\n        &tracker));\r\n\r\n  \/\/ Wait for results to arrive\r\n  results.WaitForResults();\r\n\r\n  \/\/ Stop the tracker \/\/ and wait for it to be destroyed.<\/del><\/span>\r\n  tracker.reset();\r\n  \/\/ results.WaitForDestroyed();<\/del><\/span>\r\n\r\n  \/\/ Fail if no qualifying widget was found\r\n  THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_NOT_FOUND),\r\n              results.widgetId == INVALID_WIDGET_ID);\r\n\r\n  \/\/ Return the widget that we found.\r\n  return WidgetFromId(results.GetWidgetId());\r\n}\r\n<\/pre>\n
Once nice thing about this new version that you also don’t have to worry about making sure to stop the tracker before calling Wait\u00adFor\u00adDestroyed()<\/code>. The Wait\u00adFor\u00adDestroyed()<\/code> always happens last because the First\u00adWidget\u00adResults<\/code> was constructed first, and local objects destruct in reverse order of construction. Even if you forget to stop the tracker, it will stop on its own at destruction. (The only reason to stop it earlier is performance: It allows you to do other work, like converting the Widget ID to a Widget, in parallel with the teardown of the tracker.)<\/p>\n
But really, there’s no reason for the caller to have access to the tracker. They don’t care about the tracker. They care only about the results. The way the code was originally written, the only reason the caller has the tracker is so that the caller can shut it down. But we can make that the responsibility of the First\u00adWidget\u00adResults<\/code>.<\/p>\n
struct FirstWidgetResults\r\n{\r\n  ~FirstWidgetResults()\r\n  {\r\n    if (started) destroyedEvent.wait();\r\n  }\r\n\r\n  int GetWidgetId() { return widgetId; }\r\n\r\n  void RunQuery(TrackerOptions* options)<\/span>\r\n  {                                     <\/span>\r\n    \/\/ Start the tracker (or die trying)<\/span>\r\n    unique_tracker tracker;             <\/span>\r\n    THROW_IF_FAILED(CreateTracker(      <\/span>\r\n        &options,                       <\/span>\r\n        FirstWidgetResults::OnCallback, <\/span>\r\n        &results,                       <\/span>\r\n        &tracker));                     <\/span>\r\n  \u00a0                                     <\/span>\r\n    started = true;                     <\/span>\r\n    readyEvent.wait();                  <\/span>\r\n  }                                     <\/span>\r\nprivate:<\/span>\r\n  static void CALLBACK OnCallback(\r\n      int reason, int id, void* context)\r\n  {\r\n    auto self = (MyTrackerResults*)context;\r\n\r\n    switch (reason) {\r\n    case WidgetFound:\r\n      if (self->widgetId == INVALID_WIDGET_ID) {\r\n        self->widgetId = id;\r\n        self->readyEvent.Set();\r\n      }\r\n      break;\r\n        \r\n    case InitialScanComplete:\r\n      self->readyEvent.Set();\r\n      break;\r\n\r\n    case TrackerDestroyed:\r\n      self->destroyedEvent.Set();\r\n      break;\r\n    }\r\n  }\r\n\r\nprivate:\r\n  int widgetId = INVALID_WIDGET_ID;\r\n  bool started = false;\r\n\r\n  wil::unique_event readyEvent{ wil::EventOptions::None };\r\n  wil::unique_event destroyedEvent{ wil::EventOptions::None };\r\n};\r\n\r\nWidget FindWidgetWithEnabledAdapter()\r\n{\r\n  TrackerOptions options;\r\n  \/* ... code to set the options ... *\/\r\n\r\n  FirstWidgetResults results;\r\n\r\n  \/\/ Start the tracker (or die trying) and get the results<\/span>\r\n  results.RunQuery(&options);<\/span>\r\n\r\n  \/\/ Wait for results to arrive<\/del><\/span>\r\n  \/\/ results.WaitForResults();<\/del> <\/span>\r\n  \u00a0                            <\/span>\r\n  \/\/ Stop the tracker<\/del>          <\/span>\r\n  \/\/ tracker.reset();<\/del>          <\/span>\r\n\r\n  \/\/ Fail if no qualifying widget was found\r\n  THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_NOT_FOUND),\r\n              results.GetWidgetId() == INVALID_WIDGET_ID);\r\n\r\n  \/\/ Return the widget that we found.\r\n  return WidgetFromId(results.GetWidgetId());\r\n}\r\n<\/pre>\n
The First\u00adWidget\u00adResults<\/code> class manages the tracker. This also implicitly does the “early tracker destruction” optimization, because the tracker destructs as part of returning from Run\u00adQuery<\/code>, long before the First\u00adWidget\u00adResults<\/code> destructs.<\/p>\n
Finally, we can put the Run\u00adQuery<\/code> into the constructor. This avoids the need for a started<\/code> state variable.<\/p>\n
struct FirstWidgetResults\r\n{\r\n  ~FirstWidgetResults()\r\n  {\r\n    \/\/ if (started)<\/del><\/span>\r\n    destroyedEvent.wait();\r\n  }\r\n\r\n  int GetWidgetId() { return widgetId; }\r\n\r\n  FirstWidgetResults<\/span>(TrackerOptions* options)\r\n  {\r\n    \/\/ Start the tracker (or die trying)\r\n    unique_tracker tracker;\r\n\r\n    \/\/ Throwing from a constructor bypasses the destructor<\/span>\r\n    THROW_IF_FAILED(CreateTracker(\r\n        &options,\r\n        FirstWidgetResults::OnCallback,\r\n        &results,\r\n        &tracker));\r\n\r\n    \/\/ started = true;<\/del><\/span>\r\n    readyEvent.wait();\r\n  }\r\n\r\n  ...\r\n  \/\/ bool started = false;<\/del><\/span>\r\n  ...\r\n};\r\n\r\nWidget FindWidgetWithEnabledAdapter()\r\n{\r\n  TrackerOptions options;\r\n  \/* ... code to set the options ... *\/\r\n\r\n  FirstWidgetResults results(&options);<\/span>\r\n\r\n  \/\/ Fail if no qualifying widget was found\r\n  THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_NOT_FOUND),\r\n              results.GetWidgetId() == INVALID_WIDGET_ID);\r\n\r\n  \/\/ Return the widget that we found.\r\n  return WidgetFromId(results.GetWidgetId());\r\n}\r\n<\/pre>\n
This takes advantage of the fact that throwing from a class’s constructor bypasses the class’s destructor, because you can’t destruct something that hasn’t fully constructed. This aspect of the C++ language is often a gotcha, but here we are taking advantage of it: If CreateTracker<\/code> fails, then the tracker never started, so we don’t want to wait for it, and bypassing the destructor is exactly what we want.<\/p>\n
Now, this “bypassing the destructor” behavior is rather subtle, so I can see why you wouldn’t want to take advantage of it, especially since you can introduce bugs if you add more code to the constructor that could throw after the tracker has started, so this last transformation may not be something you’re comfortable with.<\/p>\n
And that’s fine. I’m not sure I’m comfortable with it either.<\/p>\n","protected":false},"excerpt":{"rendered":"
Avoiding using an object after is has destructed, and maybe using a less-well-known corner of the C++ language.<\/p>\n","protected":false},"author":1069,"featured_media":111744,"comment_status":"open","ping_status":"closed","sticky":false,"template":"","format":"standard","meta":{"_acf_changed":false,"footnotes":""},"categories":[1],"tags":[25],"class_list":["post-108348","post","type-post","status-publish","format-standard","has-post-thumbnail","hentry","category-oldnewthing","tag-code"],"acf":[],"blog_post_summary":"
Avoiding using an object after is has destructed, and maybe using a less-well-known corner of the C++ language.<\/p>\n","_links":{"self":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts\/108348","targetHints":{"allow":["GET"]}}],"collection":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/users\/1069"}],"replies":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/comments?post=108348"}],"version-history":[{"count":0,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts\/108348\/revisions"}],"wp:featuredmedia":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/media\/111744"}],"wp:attachment":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/media?parent=108348"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/categories?post=108348"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/tags?post=108348"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}