Changeset 82506 in vbox for trunk/src/VBox/HostServices

Timestamp:

Dec 9, 2019 3:18:31 AM (5 years ago)

Author:

vboxsync

svn:sync-xref-src-repo-rev:

135359

Message:

SharedClipboardSvc,Vbgl: Reviewed and adjusted the handling of the VBOX_SHCL_GUEST_FN_DATA_WRITE message, paddling back two thirds of the parameter changes from the 6.1 dev cycle and fixing a couple of bugs introduced (buggy code commented out and marked with @todos). Fixing the broadcast approach of VBOX_SHCL_GUEST_FN_FORMATS_REPORT as the host doesn't want to know about clipboard stuff when VRDE clients are using the clipboard. Also documented the message. bugref:9437

Location:

trunk/src/VBox/HostServices/SharedClipboard

Files:

• VBoxSharedClipboardSvc-internal.h (modified) (1 diff)
• VBoxSharedClipboardSvc.cpp (modified) (3 diffs)

trunk/src/VBox/HostServices/SharedClipboard/VBoxSharedClipboardSvc-internal.h

@@ -146 +146 @@
     RTCList<SHCLCLIENTMSG *> queueMsg;
     /** The client's own event source.
-     *  Needed for events which are not bound to a specific transfer. */
+     *  Needed for events which are not bound to a specific transfer.
+     * @todo r=bird: s/Events/EventSrc/ !!  */
     SHCLEVENTSOURCE          Events;
 #ifdef VBOX_WITH_SHARED_CLIPBOARD_TRANSFERS

trunk/src/VBox/HostServices/SharedClipboard/VBoxSharedClipboardSvc.cpp

@@ -1459 +1459 @@
     LogFlowFuncEnter();
 
-    if (   ShClSvcGetMode() != VBOX_SHCL_MODE_GUEST_TO_HOST
-        && ShClSvcGetMode() != VBOX_SHCL_MODE_BIDIRECTIONAL)
-    {
+    /*
+     * Check if the service mode allows this operation and whether the guest is
+     * supposed to be reading from the host.
+     */
+    uint32_t uMode = ShClSvcGetMode();
+    if (   uMode == VBOX_SHCL_MODE_BIDIRECTIONAL
+        || uMode == VBOX_SHCL_MODE_GUEST_TO_HOST)
+    { /* likely */ }
+    else
         return VERR_ACCESS_DENIED;
-    }
-
-    /* Is the guest supposed to write any clipboard data from the host? */
-    if (!(pClient->State.fFlags & SHCLCLIENTSTATE_FLAGS_WRITE_ACTIVE))
-        return VERR_WRONG_ORDER;
-
-    int rc;
-
-    SHCLDATABLOCK dataBlock;
-    RT_ZERO(dataBlock);
-
+
+    /** @todo r=bird: This whole active flag stuff is broken, so disabling for now. */
+    //if (pClient->State.fFlags & SHCLCLIENTSTATE_FLAGS_WRITE_ACTIVE)
+    //{ /* likely */ }
+    //else
+    //    return VERR_WRONG_ORDER;
+
+    /*
+     * Digest parameters.
+     *
+     * There are 3 different format here, formatunately no parameters have been
+     * switch around so it's plain sailing compared to the DATA_READ message.
+     */
+    ASSERT_GUEST_RETURN(pClient->State.fGuestFeatures0 & VBOX_SHCL_GF_0_CONTEXT_ID
+                        ? cParms == VBOX_SHCL_CPARMS_DATA_WRITE || cParms == VBOX_SHCL_CPARMS_DATA_WRITE_61B
+                        : cParms == VBOX_SHCL_CPARMS_DATA_WRITE_OLD,
+                        VERR_WRONG_PARAMETER_COUNT);
+
+    uintptr_t iParm = 0;
     SHCLCLIENTCMDCTX cmdCtx;
     RT_ZERO(cmdCtx);
-
-    if (!(pClient->State.fGuestFeatures0 & VBOX_SHCL_GF_0_CONTEXT_ID)) /* Legacy, Guest Additions < 6.1. */
-    {
-        if (cParms != 2)
-        {
-            rc = VERR_INVALID_PARAMETER;
-        }
-        else
-        {
-            rc = HGCMSvcGetU32(&paParms[0], &dataBlock.uFormat);
-            if (RT_SUCCESS(rc))
-            {
-                if (pClient->State.POD.uFormat == VBOX_SHCL_FMT_NONE)
-                    pClient->State.POD.uFormat = dataBlock.uFormat;
-
-                if (   dataBlock.uFormat == VBOX_SHCL_FMT_NONE
-                    || dataBlock.uFormat != pClient->State.POD.uFormat)
-                {
-                    LogFunc(("Invalid format (client=%RU32 vs host=%RU32)\n", dataBlock.uFormat, pClient->State.POD.uFormat));
-                    rc = VERR_INVALID_PARAMETER;
-                }
-                else
-                    rc = HGCMSvcGetBuf(&paParms[1], &dataBlock.pvData, &dataBlock.cbData);
-            }
-        }
+    if (cParms > VBOX_SHCL_CPARMS_DATA_WRITE_OLD)
+    {
+        ASSERT_GUEST_RETURN(paParms[iParm].type == VBOX_HGCM_SVC_PARM_64BIT, VERR_WRONG_PARAMETER_TYPE);
+        cmdCtx.uContextID = paParms[iParm].u.uint64;
+        uint64_t const idCtxExpected = VBOX_SHCL_CONTEXTID_MAKE(pClient->State.uSessionID, pClient->Events.uID,
+                                                                VBOX_SHCL_CONTEXTID_GET_EVENT(cmdCtx.uContextID));
+        ASSERT_GUEST_MSG_RETURN(cmdCtx.uContextID == idCtxExpected,
+                                ("Wrong context ID: %#RX64, expected %#RX64\n", cmdCtx.uContextID, idCtxExpected),
+                                VERR_INVALID_CONTEXT);
+        iParm++;
     }
     else
     {
-        if (cParms != VBOX_SHCL_CPARMS_WRITE_DATA)
-        {
-            rc = VERR_INVALID_PARAMETER;
-        }
-        else
-        {
-            rc = HGCMSvcGetU64(&paParms[0], &cmdCtx.uContextID);
-
-            /** @todo Handle paParms[1] flags. */
-
-            if (RT_SUCCESS(rc))
-                rc = HGCMSvcGetU32(&paParms[2], &dataBlock.uFormat);
-            if (RT_SUCCESS(rc))
-                rc = HGCMSvcGetBuf(&paParms[4], &dataBlock.pvData, &dataBlock.cbData);
-        }
-    }
-
+        /** @todo supply CID from client state? Setting it in ShClSvcDataReadRequest? */
+    }
+    if (cParms == VBOX_SHCL_CPARMS_DATA_WRITE_61B)
+    {
+        ASSERT_GUEST_RETURN(paParms[iParm].type == VBOX_HGCM_SVC_PARM_32BIT, VERR_WRONG_PARAMETER_TYPE);
+        ASSERT_GUEST_RETURN(paParms[iParm].u.uint32 == 0, VERR_INVALID_FLAGS);
+        iParm++;
+    }
+    SHCLDATABLOCK dataBlock;
+    ASSERT_GUEST_RETURN(paParms[iParm].type == VBOX_HGCM_SVC_PARM_32BIT, VERR_WRONG_PARAMETER_TYPE); /* Format bit. */
+    dataBlock.uFormat = paParms[iParm].u.uint32;
+    iParm++;
+    if (cParms == VBOX_SHCL_CPARMS_DATA_WRITE_61B)
+    {
+        ASSERT_GUEST_RETURN(paParms[iParm].type == VBOX_HGCM_SVC_PARM_32BIT, VERR_WRONG_PARAMETER_TYPE); /* "cbData" - duplicates buffer size. */
+        iParm++;
+    }
+    ASSERT_GUEST_RETURN(paParms[iParm].type == VBOX_HGCM_SVC_PARM_PTR, VERR_WRONG_PARAMETER_TYPE); /* Data buffer */
+    dataBlock.pvData = paParms[iParm].u.pointer.addr;
+    dataBlock.cbData = paParms[iParm].u.pointer.size;
+    iParm++;
+    Assert(iParm == cParms);
+
+    /*
+     * For some reason we need to do this (makes absolutely no sense to bird).
+     */
+    /** @todo r=bird: I really don't get why you need the State.POD.uFormat
+     *        member.  I'm sure there is a reason.  Incomplete code? */
+    if (!(pClient->State.fGuestFeatures0 & VBOX_SHCL_GF_0_CONTEXT_ID))
+    {
+        if (pClient->State.POD.uFormat == VBOX_SHCL_FMT_NONE)
+            pClient->State.POD.uFormat = dataBlock.uFormat;
+        /** @todo r=bird: this must be buggy to, I've disabled it without testing
+         *        though. */
+        //ASSERT_GUEST_MSG_RETURN(pClient->State.POD.uFormat == dataBlock.uFormat,
+        //                        ("Requested %#x, POD.uFormat=%#x\n", dataBlock.uFormat, pClient->State.POD.uFormat),
+        //                        VERR_BAD_EXE_FORMAT /*VERR_INTERNAL_ERROR*/);
+    }
+
+    /*
+     * Write the data to the active host side clipboard.
+     */
+    int rc;
+    if (g_ExtState.pfnExtension)
+    {
+        SHCLEXTPARMS parms;
+        RT_ZERO(parms);
+        parms.uFormat   = dataBlock.uFormat;
+        parms.u.pvData  = dataBlock.pvData;
+        parms.cbData    = dataBlock.cbData;
+
+        g_ExtState.pfnExtension(g_ExtState.pvExtension, VBOX_CLIPBOARD_EXT_FN_DATA_WRITE, &parms, sizeof(parms));
+        rc = VINF_SUCCESS;
+    }
+    else
+        rc = ShClSvcImplWriteData(pClient, &cmdCtx, &dataBlock);
     if (RT_SUCCESS(rc))
     {
-        if (g_ExtState.pfnExtension)
-        {
-            SHCLEXTPARMS parms;
-            RT_ZERO(parms);
-
-            parms.uFormat   = dataBlock.uFormat;
-            parms.u.pvData  = dataBlock.pvData;
-            parms.cbData    = dataBlock.cbData;
-
-            g_ExtState.pfnExtension(g_ExtState.pvExtension, VBOX_CLIPBOARD_EXT_FN_DATA_WRITE, &parms, sizeof(parms));
-        }
-
-        rc = ShClSvcImplWriteData(pClient, &cmdCtx, &dataBlock);
-        if (RT_SUCCESS(rc))
-        {
-            /* Remove "write active" flag after successful read again. */
-            pClient->State.fFlags &= ~SHCLCLIENTSTATE_FLAGS_WRITE_ACTIVE;
-        }
+        /* Remove "write active" flag after successful read again. */
+        /** @todo r=bird: This doesn't make any effing sense.  What if the host
+         *         wants to have the guest write it another format???  */
+        pClient->State.fFlags &= ~SHCLCLIENTSTATE_FLAGS_WRITE_ACTIVE;
     }
 
@@ -1848 +1872 @@
                                                     &parms, sizeof(parms));
                         }
-
-                        SHCLCLIENTCMDCTX cmdCtx;
-                        RT_ZERO(cmdCtx);
-
-                        SHCLFORMATDATA formatData;
-                        RT_ZERO(formatData);
-
-                        formatData.Formats = uFormats;
-                        Assert(formatData.Formats != VBOX_SHCL_FMT_NONE); /* Sanity. */
-
-                        rc = ShClSvcImplFormatAnnounce(pClient, &cmdCtx, &formatData);
+                        else
+                        {
+
+                            SHCLCLIENTCMDCTX cmdCtx;
+                            RT_ZERO(cmdCtx);
+
+                            SHCLFORMATDATA formatData;
+                            RT_ZERO(formatData);
+
+                            formatData.Formats = uFormats;
+                            Assert(formatData.Formats != VBOX_SHCL_FMT_NONE); /* Sanity. */
+
+                            rc = ShClSvcImplFormatAnnounce(pClient, &cmdCtx, &formatData);
+                        }
+
+                        /** @todo r=bird: I'm not sure if the guest should be automatically allowed
+                         *        to write the host clipboard now.  It would make more sense to disallow
+                         *        host clipboard reads until the host reports formats.
+                         *
+                         *        The writes should only really be allowed upon request from the host,
+                         *        shouldn't they? (Though, I'm not sure, maybe there are situations
+                         *        where the guest side will just want to push the content over
+                         *        immediately while it's still available, I don't quite recall now...
+                         */
                         if (RT_SUCCESS(rc))
-                        {
                             pClient->State.fFlags |= SHCLCLIENTSTATE_FLAGS_WRITE_ACTIVE;
-                        }
                     }
                 }
@@ -1875 +1910 @@
 
         case VBOX_SHCL_GUEST_FN_DATA_WRITE:
-        {
             rc = shClSvcGetDataWrite(pClient, cParms, paParms);
             break;
-        }
 
         case VBOX_SHCL_GUEST_FN_CANCEL: