Attachment 600963 Details for Bug 841159 – Proposed patch to resolve issues in PCP result PDU handling

[patch] Proposed patch to resolve issues in PCP result PDU handling

pcp_result (text/plain), 8.50 KB, created by Nathan Scott on 2012-07-29 01:24:53 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Nathan Scott

Created: 2012-07-29 01:24:53 UTC

Size: 8.50 KB

patch

obsolete

>Index: pcp/src/libpcp/src/p_result.c
>===================================================================
>--- pcp.orig/src/libpcp/src/p_result.c	2012-07-25 16:44:55.999948441 +1000
>+++ pcp/src/libpcp/src/p_result.c	2012-07-28 17:46:42.428050298 +1000
>@@ -175,7 +175,9 @@ __pmDecodeResult(__pmPDU *pdubuf, pmResu
>     int		sts;		/* function status */
>     int		i = 0;		/* range of metrics */
>     int		j;		/* range over values */
>+    int		index;
>     int		version;
>+    char	*pduend;
>     result_t	*pp;
>     vlist_t	*vlp;
>     pmResult	*pr = NULL;
>@@ -183,6 +185,7 @@ __pmDecodeResult(__pmPDU *pdubuf, pmResu
>     char	*newbuf;
>     int		valfmt;
>     int		numval;
>+    int		need;
> /*
>  * Note: all sizes are in units of bytes ... beware that pp->data is in
>  *	 units of __pmPDU
>@@ -202,7 +205,15 @@ __pmDecodeResult(__pmPDU *pdubuf, pmResu
> 	return sts;
> 
>     pp = (result_t *)pdubuf;
>+    pduend = (char *)pdubuf + pp->hdr.len;
>+    if (pduend - (char *)pdubuf < sizeof(result_t))
>+	return PM_ERR_IPC;
>+
>     numpmid = ntohl(pp->numpmid);
>+    if (numpmid < 0 || numpmid > pp->hdr.len)
>+	return PM_ERR_IPC;
>+    if (numpmid >= (INT_MAX - sizeof(pmResult)) / sizeof(pmValueSet *))
>+	return PM_ERR_IPC;
>     if ((pr = (pmResult *)malloc(sizeof(pmResult) +
> 			     (numpmid - 1) * sizeof(pmValueSet *))) == NULL) {
> 	return -oserror();
>@@ -215,7 +226,12 @@ __pmDecodeResult(__pmPDU *pdubuf, pmResu
>     nvsize = vsize = vbsize = 0;
>     for (i = 0; i < numpmid; i++) {
> 	vlp = (vlist_t *)&pp->data[vsize/sizeof(__pmPDU)];
>+
>+	if (sizeof(*vlp) - sizeof(vlp->vlist) > (size_t)(pduend - (char *)vlp))
>+	    goto corrupt;
>+
> 	vsize += sizeof(vlp->pmid) + sizeof(vlp->numval);
>+	nvsize += sizeof(pmValueSet);
> 	numval = ntohl(vlp->numval);
> 	valfmt = ntohl(vlp->valfmt);
> #ifdef DESPERATE
>@@ -226,8 +242,11 @@ __pmDecodeResult(__pmPDU *pdubuf, pmResu
> 			i, pmIDStr_r(pmid, strbuf, sizeof(strbuf)), numval);
> 	}
> #endif
>-	nvsize += sizeof(pmValueSet);
>+	if (numval < 0 || numval > pp->hdr.len)
>+	    goto corrupt;
> 	if (numval > 0) {
>+	    if (numval >= (INT_MAX - sizeof(vlp->valfmt)) / sizeof(__pmValue_PDU))
>+		goto corrupt;
> 	    vsize += sizeof(vlp->valfmt) + numval * sizeof(__pmValue_PDU);
> 	    nvsize += (numval - 1) * sizeof(pmValue);
> #ifdef DESPERATE
>@@ -236,10 +255,19 @@ __pmDecodeResult(__pmPDU *pdubuf, pmResu
> #endif
> 	    if (valfmt != PM_VAL_INSITU) {
> 		for (j = 0; j < numval; j++) {
>-		    int		index = ntohl(vlp->vlist[j].value.pval);
>-		    pmValueBlock *pduvbp = (pmValueBlock *)&pdubuf[index];
>+		    __pmValue_PDU *pduvp;
>+		    pmValueBlock *pduvbp;
> 
>+		    pduvp = &vlp->vlist[j];
>+		    if (sizeof(__pmValue_PDU) > (size_t)(pduend - (char *)pduvp))
>+			goto corrupt;
>+		    index = ntohl(vlp->vlist[j].value.pval);
>+		    pduvbp = (pmValueBlock *)&pdubuf[index];
>+		    if (sizeof(pmValueBlock) > (size_t)(pduend - (char *)pduvbp))
>+			goto corrupt;
> 		    __ntohpmValueBlock(pduvbp);
>+		    if (pduvbp->vlen < 0 || pduvbp->vlen > pp->hdr.len)
>+			goto corrupt;
> 		    vbsize += PM_PDU_SIZE_BYTES(pduvbp->vlen);
> #ifdef DESPERATE
> 		    fprintf(stderr, " len: %d type: %d",
>@@ -256,21 +284,29 @@ __pmDecodeResult(__pmPDU *pdubuf, pmResu
>     fprintf(stderr, "vsize: %d nvsize: %d vbsize: %d\n", vsize, nvsize, vbsize);
> #endif
> 
>-    /* the original pdubuf is already pinned so we won't just allocate that again */
>-    if ((newbuf = (char *)__pmFindPDUBuf((int)nvsize + vbsize)) == NULL) {
>+    need = nvsize + vbsize;
>+    if (need < 0 ||
>+	vsize > INT_MAX / sizeof(__pmPDU) ||
>+	vbsize > INT_MAX / sizeof(pmValueBlock) ||
>+	sizeof(result_t) - sizeof(__pmPDU) + need > pduend - (char *)pdubuf)
>+	goto corrupt;
>+
>+    /* the original pdubuf is already pinned so we won't allocate that again */
>+    if ((newbuf = (char *)__pmFindPDUBuf(need)) == NULL) {
> 	free(pr);
> 	return -oserror();
>     }
> 
>     /*
>-     * At this point, we have the following set up ...
>+     * At this point, we have verified the contents of the incoming PDU and
>+     * the following is set up ...
>      *
>      * From the original PDU buffer ...
>-     * :-----:---------:-----------:----------------:--------------------:
>-     * : Hdr : numpmid : timestamp : ... vlists ... : .. pmValueBocks .. :
>-     * :-----:---------:-----------:----------------:--------------------:
>-     *                              <---  vsize ---> <---   vbsize   --->
>-     *                                    bytes             bytes
>+     * :-----:---------:-----------:----------------:---------------------:
>+     * : Hdr : numpmid : timestamp : ... vlists ... : .. pmValueBlocks .. :
>+     * :-----:---------:-----------:----------------:---------------------:
>+     *                              <---  vsize ---> <----   vbsize  ---->
>+     *                                    bytes              bytes
>      *
>      * and in the new PDU buffer we are going to build ...
>      * :---------------------:--------------------:
>@@ -282,7 +318,8 @@ __pmDecodeResult(__pmPDU *pdubuf, pmResu
> 
>     if (vbsize) {
> 	/* pmValueBlocks (if any) are copied across "as is" */
>-	memcpy((void *)&newbuf[nvsize], (void *)&pp->data[vsize/sizeof(__pmPDU)], vbsize);
>+	index = vsize / sizeof(__pmPDU);
>+	memcpy((void *)&newbuf[nvsize], (void *)&pp->data[index], vbsize);
>     }
> 
>     /*
>@@ -334,7 +371,8 @@ __pmDecodeResult(__pmPDU *pdubuf, pmResu
> 		     * in the input PDU buffer, pval is an index to the
> 		     * start of the pmValueBlock, in units of __pmPDU
> 		     */
>-		    nvp->value.pval = (pmValueBlock *)&newbuf[sizeof(__pmPDU) * ntohl(vp->value.pval) + offset];
>+		    index = sizeof(__pmPDU) * ntohl(vp->value.pval) + offset;
>+		    nvp->value.pval = (pmValueBlock *)&newbuf[index];
> #ifdef DESPERATE
> 		    {
> 			int		k, len;
>@@ -360,56 +398,77 @@ __pmDecodeResult(__pmPDU *pdubuf, pmResu
>     pr->timestamp.tv_sec = ntohl(pp->timestamp.tv_sec);
>     pr->timestamp.tv_usec = ntohl(pp->timestamp.tv_usec);
>     vlp = (vlist_t *)pp->data;
>+
>     /*
>      * Now fix up any pointers in pmValueBlocks (currently offsets into
>      * the PDU buffer) by adding the base address of the PDU buffer.
>      */
>     for (i = 0; i < numpmid; i++) {
>+	if (sizeof(vlist_t) - sizeof(__pmValue_PDU) > (pduend - (char *)vlp))
>+	    goto corrupt;
>+
> 	vsp = pr->vset[i] = (pmValueSet *)vlp;
>-#ifndef HAVE_NETWORK_BYTEORDER
> 	vsp->pmid = __ntohpmID(vsp->pmid);
> 	vsp->numval = ntohl(vsp->numval);
>+	if (vsp->numval < 0 || vsp->numval > pp->hdr.len)
>+	    goto corrupt;
>+	if (vsp->numval >= (INT_MAX - sizeof(*vlp)) / sizeof(__pmValue_PDU))
>+	    goto corrupt;
> 	if (vsp->numval > 0) {
> 	    vsp->valfmt = ntohl(vsp->valfmt);
> 	    for (j = 0; j < vsp->numval; j++) {
>-		vsp->vlist[j].inst = ntohl(vsp->vlist[j].inst);
>-		if (vsp->valfmt == PM_VAL_INSITU)
>-		    vsp->vlist[j].value.lval = ntohl(vsp->vlist[j].value.lval);
>-	    }
>-	}
>-#endif
>-	if (vsp->numval > 0) {
>-	    if (vsp->valfmt != PM_VAL_INSITU) {
>-		/* salvage pmValueBlocks from end of PDU */
>-		for (j = 0; j < vsp->numval; j++) {
>-		    vsp->vlist[j].value.pval = (pmValueBlock *)&pdubuf[ntohl(vsp->vlist[j].value.lval)];
>-		    __ntohpmValueBlock(vsp->vlist[j].value.pval);
>+		__pmValue_PDU *pduvp;
>+		pmValueBlock *pduvbp;
>+
>+		pduvp = &vsp->vlist[j];
>+		if (sizeof(__pmValue_PDU) > (size_t)(pduend - (char *)pduvp))
>+		    goto corrupt;
>+
>+		pduvp->inst = ntohl(pduvp->inst);
>+		if (vsp->valfmt == PM_VAL_INSITU) {
>+		    pduvp->value.lval = ntohl(pduvp->value.lval);
>+		} else {
>+		    /* salvage pmValueBlocks from end of PDU */
>+		    index = ntohl(pduvp->value.lval);
>+		    pduvbp = (pmValueBlock *)&pdubuf[index];
>+		    if (sizeof(pmValueBlock) > (size_t)(pduend - (char *)pduvbp))
>+			goto corrupt;
>+		    if (pduvbp->vlen < 0 || pduvbp->vlen > pp->hdr.len)
>+			goto corrupt;
>+		    pduvp->value.pval = pduvbp;
>+		    __ntohpmValueBlock(pduvbp);
> 		}
> 	    }
> 	    vlp = (vlist_t *)((__psint_t)vlp + sizeof(*vlp) + (vsp->numval-1)*sizeof(vlp->vlist[0]));
> 	}
>-	else
>+	else {
> 	    vlp = (vlist_t *)((__psint_t)vlp + sizeof(vlp->pmid) + sizeof(vlp->numval));
>+	}
>     }
>-    if (numpmid > 0)
>+    if (numpmid > 0) {
> 	/*
> 	 * PDU buffer already pinned on entry, pin again so that
> 	 * the caller can safely call _both_ pmFreeResult() and
> 	 * __pmUnpinPDUBuf() ... refer to thread-safe notes above.
> 	 */
> 	__pmPinPDUBuf(pdubuf);
>+    }
> #endif
> 
> #ifdef PCP_DEBUG
>     if (pmDebug & DBG_TRACE_PDU)
> 	__pmDumpResult(stderr, pr);
> #endif
>-    *result = pr;
> 
>     /*
>      * Note we return with the input buffer (pdubuf) still pinned and
>      * for the 64-bit pointer case the new buffer (newbuf) also pinned -
>      * if numpmid != 0 see the thread-safe comments above
>      */
>+    *result = pr;
>     return 0;
>+
>+corrupt:
>+    free(pr);
>+    return PM_ERR_IPC;
> }

Actions: View | Diff

Attachments on bug 841159: 600963 | 601673 | 602218 | 602222 | 602400