Changeset 83565 in vbox for trunk/src

Timestamp:

Apr 5, 2020 3:01:10 PM (5 years ago)

Author:

vboxsync

Message:

DevVirtioSCSI.cpp: Some memory todos. Use RTMemFreeZ for one allocation. bugref:9698

File:

• trunk/src/VBox/Devices/Storage/DevVirtioSCSI.cpp (modified) (19 diffs)

trunk/src/VBox/Devices/Storage/DevVirtioSCSI.cpp

@@ -768 +768 @@
                               PVIRTIO_DESC_CHAIN_T pDescChain, REQ_RESP_HDR_T *pRespHdr, uint8_t *pbSense)
 {
+    /** @todo r=bird: There is too much allocating here and we'll leak stuff if
+     *        we're low on memory and one of the RTMemAllocZ calls fail! */
     uint8_t *pabSenseBuf = (uint8_t *)RTMemAllocZ(pThis->virtioScsiConfig.uSenseSize);
     AssertReturn(pabSenseBuf, VERR_NO_MEMORY);
 
     Log2Func(("   status: %s    response: %s\n",
-        SCSIStatusText(pRespHdr->uStatus), virtioGetReqRespText(pRespHdr->uResponse)));
+              SCSIStatusText(pRespHdr->uStatus), virtioGetReqRespText(pRespHdr->uResponse)));
 
     PRTSGBUF pReqSegBuf = (PRTSGBUF)RTMemAllocZ(sizeof(RTSGBUF));
@@ -791 +793 @@
 
     /* Copy segment data to malloc'd memory to avoid stack out-of-scope errors sanitizer doesn't detect */
+    /** @todo r=bird: The above comment makes zero sense as the memory is freed
+     *        before we return, so there cannot be any trouble with out-of-scope
+     *        stuff here. */
     for (int i = 0; i < 2; i++)
     {
         void *pv = paReqSegs[i].pvSeg;
-        paReqSegs[i].pvSeg = RTMemAlloc(paReqSegs[i].cbSeg);
+        paReqSegs[i].pvSeg = RTMemDup(pv, paReqSegs[i].cbSeg);
         AssertReturn(paReqSegs[i].pvSeg, VERR_NO_MEMORY);
-        memcpy(paReqSegs[i].pvSeg, pv, paReqSegs[i].cbSeg);
     }
 
@@ -930 +934 @@
     }
 
-    int cSegs = 0;
-
     if (   (VIRTIO_IS_IN_DIRECTION(pReq->enmTxDir)  && cbXfer32 > pReq->cbDataIn)
         || (VIRTIO_IS_OUT_DIRECTION(pReq->enmTxDir) && cbXfer32 > pReq->cbDataOut))
@@ -950 +952 @@
 
         /* req datain bytes already in guest phys mem. via virtioScsiIoReqCopyFromBuf() */
+        /** @todo r=bird: There is too much allocating here and we'll leak stuff if
+         *        we're low on memory and one of the RTMemAllocZ calls fail! */
 
         PRTSGBUF pReqSegBuf = (PRTSGBUF)RTMemAllocZ(sizeof(RTSGBUF));
@@ -957 +961 @@
         AssertReturn(paReqSegs, VERR_NO_MEMORY);
 
+        int cSegs = 0;
         paReqSegs[cSegs].pvSeg = &respHdr;
         paReqSegs[cSegs++].cbSeg = sizeof(respHdr);
@@ -964 +969 @@
 
         /* Copy segment data to malloc'd memory to avoid stack out-of-scope errors sanitizer doesn't detect */
+        /** @todo r=bird: The above comment makes zero sense as the memory is freed
+         *        before we return, so there cannot be any trouble with out-of-scope
+         *        stuff here. */
         for (int i = 0; i < cSegs; i++)
         {
             void *pv = paReqSegs[i].pvSeg;
-            paReqSegs[i].pvSeg = RTMemAlloc(paReqSegs[i].cbSeg);
+            paReqSegs[i].pvSeg = RTMemDup(pv, paReqSegs[i].cbSeg);
             AssertReturn(paReqSegs[i].pvSeg, VERR_NO_MEMORY);
-            memcpy(paReqSegs[i].pvSeg, pv, paReqSegs[i].cbSeg);
         }
 
@@ -1126 +1133 @@
     {
         LogRel(("* * * REPORT LUNS LU ACCESSED * * * "));
-        /* Force rejection. todo: figure out right way to handle. Note this is a very
+        /* Force rejection. */ /** @todo figure out right way to handle. Note this is a very
          * vague and confusing part of the VirtIO spec which deviates from the SCSI standard
          * I have not been able to determine how to implement this properly.  Guest drivers
@@ -1166 +1173 @@
         respHdr.uResidual  = cbDataIn + cbDataOut;
         virtioScsiR3ReqErr(pDevIns, pThis, pThisCC, qIdx, pDescChain, &respHdr , NULL);
-        RTMemFree(pVirtqReq);
+        RTMemFreeZ(pVirtqReq, cbReqHdr);
         return VINF_SUCCESS;
     }
@@ -1186 +1193 @@
         respHdr.uResidual  = cbDataOut + cbDataIn;
         virtioScsiR3ReqErr(pDevIns, pThis, pThisCC, qIdx, pDescChain, &respHdr, abSense);
-        RTMemFree(pVirtqReq);
+        RTMemFreeZ(pVirtqReq, cbReqHdr);
         return VINF_SUCCESS;
 
@@ -1204 +1211 @@
         respHdr.uResidual  = cbDataOut + cbDataIn;
         virtioScsiR3ReqErr(pDevIns, pThis, pThisCC, qIdx, pDescChain, &respHdr, abSense);
-        RTMemFree(pVirtqReq);
+        RTMemFreeZ(pVirtqReq, cbReqHdr);
         return VINF_SUCCESS;
     }
@@ -1218 +1225 @@
         respHdr.uResidual  = cbDataIn + cbDataOut;
         virtioScsiR3ReqErr(pDevIns, pThis, pThisCC, qIdx, pDescChain, &respHdr, NULL);
-        RTMemFree(pVirtqReq);
+        RTMemFreeZ(pVirtqReq, cbReqHdr);
         return VINF_SUCCESS;
     }
@@ -1237 +1244 @@
         respHdr.uResidual  = cbDataIn + cbDataOut;
         virtioScsiR3ReqErr(pDevIns, pThis, pThisCC, qIdx, pDescChain, &respHdr , abSense);
-        RTMemFree(pVirtqReq);
+        RTMemFreeZ(pVirtqReq, cbReqHdr);
         return VINF_SUCCESS;
     }
@@ -1253 +1260 @@
     if (RT_FAILURE(rc))
     {
-        RTMemFree(pVirtqReq);
+        RTMemFreeZ(pVirtqReq, cbReqHdr);
         AssertMsgRCReturn(rc, ("Failed to allocate I/O request, rc=%Rrc\n", rc), rc);
     }
@@ -1269 +1276 @@
     pReq->pbSense      = (uint8_t *)RTMemAllocZ(pReq->cbSenseAlloc);
     AssertMsgReturnStmt(pReq->pbSense, ("Out of memory allocating sense buffer"),
-                        virtioScsiR3FreeReq(pTarget, pReq); RTMemFree(pVirtqReq), VERR_NO_MEMORY);
+                        virtioScsiR3FreeReq(pTarget, pReq); RTMemFreeZ(pVirtqReq, cbReqHdr);, VERR_NO_MEMORY);
 
     /* Note: DrvSCSI allocates one virtual memory buffer for input and output phases of the request */
@@ -1309 +1316 @@
     }
 
-    RTMemFree(pVirtqReq);
+    RTMemFreeZ(pVirtqReq, cbReqHdr);
     return VINF_SUCCESS;
 }
@@ -1348 +1355 @@
     }
 
+    /** @todo r=bird: Memory leak here.  */
     AssertReturn(   (pScsiCtrlUnion->scsiCtrl.uType == VIRTIOSCSI_T_TMF
                      && pDescChain->cbPhysSend >= sizeof(VIRTIOSCSI_CTRL_TMF_T))
@@ -1354 +1362 @@
                      && pDescChain->cbPhysSend >= sizeof(VIRTIOSCSI_CTRL_AN_T)), 0);
 
+    /** @todo r=bird: This segment handling is unnecessary. A stack array should
+     *        suffice.  The stack variable w/ initializer + memcpy into paReqSegs
+     *        done in the switch below is also weird + suboptimal. */
     PRTSGSEG paReqSegs = (PRTSGSEG)RTMemAllocZ(sizeof(RTSGSEG) * 2);
-    AssertReturn(paReqSegs, VERR_NO_MEMORY);
+    AssertReturn(paReqSegs, VERR_NO_MEMORY); /** @todo r=bird: Memory leak here.  */
 
     switch (pScsiCtrlUnion->scsiCtrl.uType)
@@ -1492 +1503 @@
 
     PRTSGBUF pReqSegBuf = (PRTSGBUF)RTMemAllocZ(sizeof(RTSGBUF));
-    AssertReturn(pReqSegBuf, VERR_NO_MEMORY);
+    AssertReturn(pReqSegBuf, VERR_NO_MEMORY); /** @todo r=bird: Memory leak here.  */
 
     /* Copy segment data to malloc'd memory to avoid stack out-of-scope errors sanitizer doesn't detect */
+    /** @todo r=bird: The above comment makes zero sense as the memory is freed
+     *        before we return, so there cannot be any trouble with out-of-scope
+     *        stuff here. */
     for (int i = 0; i < cSegs; i++)
     {
         void *pv = paReqSegs[i].pvSeg;
-        paReqSegs[i].pvSeg = RTMemAlloc(paReqSegs[i].cbSeg);
-        AssertReturn(paReqSegs[i].pvSeg, VERR_NO_MEMORY);
-        memcpy(paReqSegs[i].pvSeg, pv, paReqSegs[i].cbSeg);
+        paReqSegs[i].pvSeg = RTMemDup(pv, paReqSegs[i].cbSeg);
+        AssertReturn(paReqSegs[i].pvSeg, VERR_NO_MEMORY); /** @todo r=bird: Memory leak here.  */
     }
 
@@ -1513 +1526 @@
     RTMemFree(paReqSegs);
     RTMemFree(pReqSegBuf);
+    /** @todo r=bird: Leaking pScsiCtrlUnion here! */
 
     return VINF_SUCCESS;