Changeset 78234 in vbox for trunk/src/VBox

Timestamp:

Apr 20, 2019 11:49:01 PM (6 years ago)

Author:

vboxsync

Message:

Main/GuestCtrl: Fixed three i_waitForStatusChange() methods that would return VERR_GSTCTL_GUEST_ERROR without setting prcGuest, making the caller use uninitialized stack as status code for the operation. bugref:9320

Location:

trunk/src/VBox/Main/src-client

Files:

• GuestCtrlPrivate.cpp (modified) (2 diffs)
• GuestFileImpl.cpp (modified) (2 diffs)
• GuestImpl.cpp (modified) (1 diff)
• GuestProcessImpl.cpp (modified) (2 diffs)
• GuestSessionImpl.cpp (modified) (2 diffs)

trunk/src/VBox/Main/src-client/GuestCtrlPrivate.cpp

@@ -1266 +1266 @@
  *
  * @return  VBox status code.
+ * @retval  VERR_GSTCTL_GUEST_ERROR may be returned, call GuestResult() to get
+ *          the actual result.
+ *
  * @param   pWaitEvt                Pointer to event to wait for.
  * @param   msTimeout               Timeout (in ms) for waiting.
@@ -1478 +1481 @@
  *
  * @returns VBox status code.
+ * @retval  VERR_GSTCTL_GUEST_ERROR may be returned, call GuestResult() to get
+ *          the actual result.
+ *
  * @param   msTimeout           Timeout (in ms) to wait.
  *                              Specifiy 0 to wait indefinitely.

trunk/src/VBox/Main/src-client/GuestFileImpl.cpp

@@ -1076 +1076 @@
 }
 
+/**
+ * Undocumented, use with great care.
+ *
+ * @note Similar code in GuestProcess::i_waitForStatusChange() and
+ *       GuestSession::i_waitForStatusChange().
+ */
 int GuestFile::i_waitForStatusChange(GuestWaitEvent *pEvent, uint32_t uTimeoutMS,
                                      FileStatus_T *pFileStatus, int *prcGuest)
@@ -1116 +1122 @@
             *prcGuest = (int)lGuestRc;
     }
+    /* waitForEvent may also return VERR_GSTCTL_GUEST_ERROR like we do above, so make prcGuest is set. */
+    /** @todo r=bird: Andy, you seem to have forgotten this scenario.  Showed up occasionally when
+     * using the wrong password with a copyto command in a debug  build on windows, error info
+     * contained "Unknown Status -858993460 (0xcccccccc)".  As you know windows fills the stack frames
+     * with 0xcccccccc in debug builds to highlight use of uninitialized data, so that's what happened
+     * here.  It's actually good you didn't initialize lGuest, as it would be heck to find otherwise.
+     *
+     * I'm still not very impressed with the error managment or the usuefullness of the documentation
+     * in this code, though the latter is getting better! */
+    else if (vrc == VERR_GSTCTL_GUEST_ERROR && prcGuest)
+        *prcGuest = pEvent->GuestResult();
+    Assert(vrc != VERR_GSTCTL_GUEST_ERROR || !prcGuest || *prcGuest != (int)0xcccccccc);
 
     return vrc;

trunk/src/VBox/Main/src-client/GuestImpl.cpp

@@ -173 +173 @@
     {
 # ifdef DEBUG
+/** @todo r=bird: hit a use-after-free situation here while debugging the
+ * 0xcccccccc status code issue in copyto.  My bet is that this happens
+ * because of an uninit race, where GuestSession::close(), or someone, does
+ * not ensure that the parent object (Guest) is okay to use (in the AutoCaller
+ * sense), only their own object. */
         ULONG cRefs = itSessions->second->AddRef();
         LogFlowThisFunc(("sessionID=%RU32, cRefs=%RU32\n", itSessions->first, cRefs > 1 ? cRefs - 1 : 0));

trunk/src/VBox/Main/src-client/GuestProcessImpl.cpp

@@ -1609 +1609 @@
 }
 
+/**
+ * Undocumented, you guess what it does.
+ *
+ * @note Similar code in GuestFile::i_waitForStatusChange() and
+ *       GuestSession::i_waitForStatusChange().
+ */
 int GuestProcess::i_waitForStatusChange(GuestWaitEvent *pEvent, uint32_t uTimeoutMS,
                                         ProcessStatus_T *pProcessStatus, int *prcGuest)
@@ -1649 +1655 @@
             *prcGuest = (int)lGuestRc;
     }
+    /* waitForEvent may also return VERR_GSTCTL_GUEST_ERROR like we do above, so make prcGuest is set. */
+    else if (vrc == VERR_GSTCTL_GUEST_ERROR && prcGuest)
+        *prcGuest = pEvent->GuestResult();
+    Assert(vrc != VERR_GSTCTL_GUEST_ERROR || !prcGuest || *prcGuest != (int)0xcccccccc);
 
     LogFlowFuncLeaveRC(vrc);

trunk/src/VBox/Main/src-client/GuestSessionImpl.cpp

@@ -2884 +2884 @@
 }
 
+/**
+ * Undocumented, you guess what it does.
+ *
+ * @note Similar code in GuestFile::i_waitForStatusChange() and
+ *       GuestProcess::i_waitForStatusChange().
+ */
 int GuestSession::i_waitForStatusChange(GuestWaitEvent *pEvent, uint32_t fWaitFlags, uint32_t uTimeoutMS,
                                         GuestSessionStatus_T *pSessionStatus, int *prcGuest)
@@ -2922 +2928 @@
                          RT_SUCCESS((int)lGuestRc) ? VINF_SUCCESS : (int)lGuestRc));
     }
+    /* waitForEvent may also return VERR_GSTCTL_GUEST_ERROR like we do above, so make prcGuest is set. */
+    else if (vrc == VERR_GSTCTL_GUEST_ERROR && prcGuest)
+        *prcGuest = pEvent->GuestResult();
+    Assert(vrc != VERR_GSTCTL_GUEST_ERROR || !prcGuest || *prcGuest != (int)0xcccccccc);
 
     LogFlowFuncLeaveRC(vrc);