Changeset 83587 in vbox for trunk/src/VBox/Devices/Network

Timestamp:

Apr 6, 2020 12:31:32 PM (5 years ago)

Author:

vboxsync

Message:

Virtio_1_0: Reworked the VIRTIO_DESC_CHAIN_T handling to decrease the potential for memory leaks by employing reference counting and not having virtioCoreR3QueuePut be the one to free/release the chain. Combined the 5 allocations into a single one. bugref:9440

File:

• trunk/src/VBox/Devices/Network/DevVirtioNet_1_0.cpp (modified) (9 diffs)

trunk/src/VBox/Devices/Network/DevVirtioNet_1_0.cpp

@@ -1427 +1427 @@
     uint16_t cSegsAllocated = VIRTIONET_PREALLOCATE_RX_SEG_COUNT;
 
-    PRTSGBUF pVirtSegBufToGuest = (PRTSGBUF)RTMemAllocZ(sizeof(RTSGBUF));
+    /**  @todo r=bird: error codepaths below are almost all leaky!  Maybe keep
+     *         allocations and cleanup here and put the code doing the complicated
+     *         work into a helper that can AssertReturn at will without needing to
+     *         care about cleaning stuff up. */
+    PRTSGBUF pVirtSegBufToGuest = (PRTSGBUF)RTMemAllocZ(sizeof(RTSGBUF)); /** @todo r=bird: Missing check. */
     PRTSGSEG paVirtSegsToGuest  = (PRTSGSEG)RTMemAllocZ(sizeof(RTSGSEG) * cSegsAllocated);
     AssertReturn(paVirtSegsToGuest, VERR_NO_MEMORY);
@@ -1438 +1442 @@
     for (cDescs = uOffset = 0; uOffset < cb; )
     {
-        PVIRTIO_DESC_CHAIN_T pDescChain;
+        PVIRTIO_DESC_CHAIN_T pDescChain = NULL;
 
         int rc = virtioCoreR3QueueGet(pDevIns, &pThis->Virtio, RXQIDX_QPAIR(idxQueue), &pDescChain, true);
-        AssertRC(rc == VINF_SUCCESS || rc == VERR_NOT_AVAILABLE);
+        Assert(rc == VINF_SUCCESS || rc == VERR_NOT_AVAILABLE, ("%Rrc\n", rc));
 
         /** @todo  Find a better way to deal with this */
-        AssertMsgReturn(rc == VINF_SUCCESS && pDescChain->cbPhysReturn,
-                        ("Not enough Rx buffers in queue to accomodate ethernet packet\n"),
-                        VERR_INTERNAL_ERROR);
+        AssertMsgReturnStmt(rc == VINF_SUCCESS && pDescChain->cbPhysReturn,
+                            ("Not enough Rx buffers in queue to accomodate ethernet packet\n"),
+                            virtioCoreR3DescChainRelease(pDescChain),
+                            VERR_INTERNAL_ERROR);
 
         /* Unlikely that len of 1st seg of guest Rx (IN) buf is less than sizeof(virtio_net_pkt_hdr) == 12.
          * Assert it to reduce complexity. Robust solution would entail finding seg idx and offset of
          * virtio_net_header.num_buffers (to update field *after* hdr & pkts copied to gcPhys) */
-        AssertMsgReturn(pDescChain->pSgPhysReturn->paSegs[0].cbSeg >= sizeof(VIRTIONET_PKT_HDR_T),
-                        ("Desc chain's first seg has insufficient space for pkt header!\n"),
-                        VERR_INTERNAL_ERROR);
+        AssertMsgReturnStmt(pDescChain->pSgPhysReturn->paSegs[0].cbSeg >= sizeof(VIRTIONET_PKT_HDR_T),
+                            ("Desc chain's first seg has insufficient space for pkt header!\n"),
+                            virtioCoreR3DescChainRelease(pDescChain),
+                            VERR_INTERNAL_ERROR);
 
         uint32_t cbDescChainLeft = pDescChain->cbPhysReturn;
@@ -1500 +1506 @@
                 break;
         }
+
+        virtioCoreR3DescChainRelease(pDescChain);
     }
 
@@ -2063 +2071 @@
 
     int rc;
-    PVIRTIO_DESC_CHAIN_T pDescChain;
+    PVIRTIO_DESC_CHAIN_T pDescChain = NULL;
     while ((rc = virtioCoreR3QueuePeek(pVirtio->pDevIns, pVirtio, idxQueue, &pDescChain)) == VINF_SUCCESS)
     {
-        if (RT_SUCCESS(rc))
+        if (RT_SUCCESS(rc)) /** @todo r=bird: pointless, see loop condition. */
             Log10Func(("%s fetched descriptor chain from %s\n", INSTANCE(pThis), VIRTQNAME(idxQueue)));
         else
         {
             LogFunc(("%s failed to find expected data on %s, rc = %Rrc\n", INSTANCE(pThis), VIRTQNAME(idxQueue), rc));
+            virtioCoreR3DescChainRelease(pDescChain);
             break;
         }
@@ -2096 +2105 @@
         if (pThisCC->pDrv)
         {
-            PDMNETWORKGSO Gso, *pGso = virtioNetR3SetupGsoCtx(&Gso, &PktHdr);
+            PDMNETWORKGSO  Gso;
+            PPDMNETWORKGSO pGso = virtioNetR3SetupGsoCtx(&Gso, &PktHdr);
 
             /** @todo Optimize away the extra copying! (lazy bird) */
@@ -2141 +2151 @@
                 Log4Func(("Failed to allocate S/G buffer: size=%u rc=%Rrc\n", uSize, rc));
                 /* Stop trying to fetch TX descriptors until we get more bandwidth. */
+                virtioCoreR3DescChainRelease(pDescChain);
                 break;
             }
@@ -2152 +2163 @@
             virtioCoreQueueSync(pVirtio->pDevIns, pVirtio, idxQueue);
         }
+
+        virtioCoreR3DescChainRelease(pDescChain);
+        pDescChain = NULL;
     }
     virtioNetR3SetWriteLed(pThisCC, false);
@@ -2266 +2280 @@
              {
                  Log10Func(("%s fetching next descriptor chain from %s\n", INSTANCE(pThis), VIRTQNAME(idxQueue)));
-                 PVIRTIO_DESC_CHAIN_T pDescChain;
+                 PVIRTIO_DESC_CHAIN_T pDescChain = NULL;
                  int rc = virtioCoreR3QueueGet(pDevIns, &pThis->Virtio, idxQueue, &pDescChain, true);
                  if (rc == VERR_NOT_AVAILABLE)
@@ -2274 +2288 @@
                  }
                  virtioNetR3Ctrl(pDevIns, pThis, pThisCC, pDescChain);
+                 virtioCoreR3DescChainRelease(pDescChain);
              }
              else if (IS_TX_QUEUE(idxQueue))