Changeset 83567 in vbox for trunk/src

Timestamp:

Apr 5, 2020 7:24:56 PM (5 years ago)

Author:

vboxsync

Message:

DevVirtioSCSI.cpp: Made virtioScsiR3ReqErr use a stack buffer for the sense data instead of heap. Eliminated the other allocations too in that function. bugref:9440

File:

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

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

@@ -765 +765 @@
  * @param   pRespHdr    Response header
  * @param   pbSense     Pointer to sense buffer or NULL if none.
+ * @param   cbSenseCfg  The configured sense buffer size.
  *
- * @returns VBox status code.
+ * @returns VINF_SUCCESS
  */
 static int virtioScsiR3ReqErr(PPDMDEVINS pDevIns, PVIRTIOSCSI pThis, PVIRTIOSCSICC pThisCC, uint16_t qIdx,
-                              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);
-
+                              PVIRTIO_DESC_CHAIN_T pDescChain, REQ_RESP_HDR_T *pRespHdr, uint8_t *pbSense,
+                              uint32_t cbSenseCfg)
+{
     Log2Func(("   status: %s    response: %s\n",
               SCSIStatusText(pRespHdr->uStatus), virtioGetReqRespText(pRespHdr->uResponse)));
 
-    PRTSGBUF pReqSegBuf = (PRTSGBUF)RTMemAllocZ(sizeof(RTSGBUF));
-    AssertReturn(pReqSegBuf, VERR_NO_MEMORY);
-
-    PRTSGSEG paReqSegs  = (PRTSGSEG)RTMemAllocZ(sizeof(RTSGSEG) * 2);
-    AssertReturn(paReqSegs, VERR_NO_MEMORY);
-
-    paReqSegs[0].cbSeg = sizeof(*pRespHdr);
-    paReqSegs[0].pvSeg = pRespHdr;
-    paReqSegs[1].cbSeg = pThis->virtioScsiConfig.uSenseSize;
-    paReqSegs[1].pvSeg = pabSenseBuf;
-
+    RTSGSEG aReqSegs[2];
+
+    /* Segment #1: Request header*/
+    aReqSegs[0].pvSeg = pRespHdr;
+    aReqSegs[0].cbSeg = sizeof(*pRespHdr);
+
+    /* Segment #2: Sense data. */
+    uint8_t abSenseBuf[VIRTIOSCSI_SENSE_SIZE_MAX];
+    AssertCompile(VIRTIOSCSI_SENSE_SIZE_MAX <= 4096);
+    Assert(cbSenseCfg <= sizeof(abSenseBuf));
+
+    RT_ZERO(abSenseBuf);
     if (pbSense && pRespHdr->cbSenseLen)
-        memcpy(pabSenseBuf, pbSense, pRespHdr->cbSenseLen);
+        memcpy(abSenseBuf, pbSense, RT_MIN(pRespHdr->cbSenseLen, sizeof(abSenseBuf)));
     else
         pRespHdr->cbSenseLen = 0;
 
-    /* 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 = RTMemDup(pv, paReqSegs[i].cbSeg);
-        AssertReturn(paReqSegs[i].pvSeg, VERR_NO_MEMORY);
-    }
-
-    RTSgBufInit(pReqSegBuf, paReqSegs, 2);
+    aReqSegs[1].pvSeg = abSenseBuf;
+    aReqSegs[1].cbSeg = cbSenseCfg;
+
+    /* Init S/G buffer. */
+    RTSGBUF ReqSgBuf;
+    RTSgBufInit(&ReqSgBuf, aReqSegs, RT_ELEMENTS(aReqSegs));
 
     if (pThis->fResetting)
         pRespHdr->uResponse = VIRTIOSCSI_S_RESET;
 
-    virtioCoreR3QueuePut(pDevIns, &pThis->Virtio, qIdx, pReqSegBuf, pDescChain, true /* fFence */);
+    virtioCoreR3QueuePut(pDevIns, &pThis->Virtio, qIdx, &ReqSgBuf, pDescChain, true /* fFence */);
     virtioCoreQueueSync(pDevIns, &pThis->Virtio, qIdx);
-
-    for (int i = 0; i < 2; i++)
-        RTMemFree(paReqSegs[i].pvSeg);
-
-    RTMemFree(paReqSegs);
-    RTMemFree(pReqSegBuf);
-    RTMemFree(pabSenseBuf);
 
     if (!ASMAtomicDecU32(&pThis->cActiveReqs) && pThisCC->fQuiescing)
@@ -948 +933 @@
         respHdr.uResidual  = pReq->cbDataIn;
 
-        virtioScsiR3ReqErr(pDevIns, pThis, pThisCC, pReq->qIdx, pReq->pDescChain, &respHdr, abSense);
+        virtioScsiR3ReqErr(pDevIns, pThis, pThisCC, pReq->qIdx, pReq->pDescChain, &respHdr, abSense,
+                           RT_MIN(pThis->virtioScsiConfig.uSenseSize, VIRTIOSCSI_SENSE_SIZE_MAX));
     }
     else
@@ -1198 +1184 @@
         respHdr.uResponse  = VIRTIOSCSI_S_FAILURE;
         respHdr.uResidual  = cbDataIn + cbDataOut;
-        virtioScsiR3ReqErr(pDevIns, pThis, pThisCC, qIdx, pDescChain, &respHdr , NULL);
+        virtioScsiR3ReqErr(pDevIns, pThis, pThisCC, qIdx, pDescChain, &respHdr, NULL, cbSense);
         return VINF_SUCCESS;
     }
@@ -1217 +1203 @@
         respHdr.uResponse  = VIRTIOSCSI_S_BAD_TARGET;
         respHdr.uResidual  = cbDataOut + cbDataIn;
-        virtioScsiR3ReqErr(pDevIns, pThis, pThisCC, qIdx, pDescChain, &respHdr, abSense);
+        virtioScsiR3ReqErr(pDevIns, pThis, pThisCC, qIdx, pDescChain, &respHdr, abSense, cbSense);
         return VINF_SUCCESS;
 
@@ -1234 +1220 @@
         respHdr.uResponse  = VIRTIOSCSI_S_OK;
         respHdr.uResidual  = cbDataOut + cbDataIn;
-        virtioScsiR3ReqErr(pDevIns, pThis, pThisCC, qIdx, pDescChain, &respHdr, abSense);
+        virtioScsiR3ReqErr(pDevIns, pThis, pThisCC, qIdx, pDescChain, &respHdr, abSense, cbSense);
         return VINF_SUCCESS;
     }
@@ -1247 +1233 @@
         respHdr.uResponse  = VIRTIOSCSI_S_RESET;
         respHdr.uResidual  = cbDataIn + cbDataOut;
-        virtioScsiR3ReqErr(pDevIns, pThis, pThisCC, qIdx, pDescChain, &respHdr, NULL);
+        virtioScsiR3ReqErr(pDevIns, pThis, pThisCC, qIdx, pDescChain, &respHdr, NULL, cbSense);
         return VINF_SUCCESS;
     }
@@ -1265 +1251 @@
         respHdr.uResponse  = VIRTIOSCSI_S_FAILURE;
         respHdr.uResidual  = cbDataIn + cbDataOut;
-        virtioScsiR3ReqErr(pDevIns, pThis, pThisCC, qIdx, pDescChain, &respHdr , abSense);
+        virtioScsiR3ReqErr(pDevIns, pThis, pThisCC, qIdx, pDescChain, &respHdr, abSense, cbSense);
         return VINF_SUCCESS;
     }
@@ -1329 +1315 @@
         respHdr.uResponse  = VIRTIOSCSI_S_FAILURE;
         respHdr.uResidual  = cbDataIn + cbDataOut;
-        virtioScsiR3ReqErr(pDevIns, pThis, pThisCC, qIdx, pDescChain, &respHdr, abSense);
+        virtioScsiR3ReqErr(pDevIns, pThis, pThisCC, qIdx, pDescChain, &respHdr, abSense, cbSense);
         virtioScsiR3FreeReq(pTarget, pReq);
     }