[PATCH 0/2] dspbridge: proc: double free on error path

[PATCH 0/2] dspbridge: proc: double free on error path

[Phil Carmody]

First a bit of a cleanup of what was clearly some legacy 'handle'
usage, and then a new quicker escape route for an error path to
avoid a double free.

It's tempting to get a bit more invasive, as there's a lot of
quite dodgy looking code (e.g. casting away const) in the areas
around my changes, but they'll have to wait for another time.

[PATCH 1/2] dspbridge: proc: it's a pointer, not a handle

[Phil Carmody]

From: Phil Carmody <ext-phil.2.carmody@nokia.com>

If there's one thing worse than systems hungarian notation,
it's misleading systems hungarian notation. And you don't
need 2 copies of the pointer.

Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
---
 drivers/dsp/bridge/rmgr/proc.c |   25 +++++++++++--------------
 1 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/dsp/bridge/rmgr/proc.c b/drivers/dsp/bridge/rmgr/proc.c
index 2ccbc9b..7bc1bcd 100644
--- a/drivers/dsp/bridge/rmgr/proc.c
+++ b/drivers/dsp/bridge/rmgr/proc.c
@@ -313,7 +313,6 @@ DSP_STATUS PROC_AutoStart(struct CFG_DEVNODE *hDevNode,
 	DSP_STATUS status = DSP_EFAIL;
 	u32 dwAutoStart = 0;	/* autostart flag */
 	struct PROC_OBJECT *pProcObject;
-	struct PROC_OBJECT *hProcObject;
 	char szExecFile[MAXCMDLINELEN];
 	char *argv[2];
 	struct MGR_OBJECT *hMgrObject = NULL;
@@ -345,19 +344,18 @@ DSP_STATUS PROC_AutoStart(struct CFG_DEVNODE *hDevNode,
 	GT_0trace(PROC_DebugMask, GT_1CLASS, "NTFY Created \n");
 	pProcObject->hDevObject = hDevObject;
 	pProcObject->hMgrObject = hMgrObject;
-	hProcObject = pProcObject;
 	status = DEV_GetIntfFxns(hDevObject, &pProcObject->pIntfFxns);
 	if (DSP_SUCCEEDED(status)) {
 		status = DEV_GetWMDContext(hDevObject,
 					&pProcObject->hWmdContext);
 		if (DSP_FAILED(status)) {
-			MEM_FreeObject(hProcObject);
+			MEM_FreeObject(pProcObject);
 			GT_0trace(PROC_DebugMask, GT_7CLASS,
 				 "PROC_AutoStart: Failed "
 				 "to get WMD Context \n");
 		}
 	} else {
-		MEM_FreeObject(hProcObject);
+		MEM_FreeObject(pProcObject);
 		GT_0trace(PROC_DebugMask, GT_7CLASS,
 			 "PROC_AutoStart: Failed to "
 			 "get IntFxns \n");
@@ -366,7 +364,7 @@ DSP_STATUS PROC_AutoStart(struct CFG_DEVNODE *hDevNode,
 		goto func_cont;
 
 	/* Stop the Device, put it into standby mode */
-	status = PROC_Stop(hProcObject);
+	status = PROC_Stop(pProcObject);
 
 	if (DSP_FAILED(status))
 		goto func_cont;
@@ -388,9 +386,9 @@ DSP_STATUS PROC_AutoStart(struct CFG_DEVNODE *hDevNode,
 		argv[0] = szExecFile;
 		argv[1] = NULL;
 		/* ...and try to load it: */
-		status = PROC_Load(hProcObject, 1, (CONST char **)argv, NULL);
+		status = PROC_Load(pProcObject, 1, (CONST char **)argv, NULL);
 		if (DSP_SUCCEEDED(status)) {
-			status = PROC_Start(hProcObject);
+			status = PROC_Start(pProcObject);
 			if (DSP_SUCCEEDED(status)) {
 				GT_0trace(PROC_DebugMask, GT_1CLASS,
 					  "PROC_AutoStart: Processor started "
@@ -409,9 +407,9 @@ DSP_STATUS PROC_AutoStart(struct CFG_DEVNODE *hDevNode,
 			 "No Exec file found \n");
 	}
 func_cont:
-	kfree(hProcObject->g_pszLastCoff);
-	hProcObject->g_pszLastCoff = NULL;
-	MEM_FreeObject(hProcObject);
+	kfree(pProcObject->g_pszLastCoff);
+	pProcObject->g_pszLastCoff = NULL;
+	MEM_FreeObject(pProcObject);
 func_end:
 	GT_1trace(PROC_DebugMask, GT_ENTER,
 		 "Exiting PROC_AutoStart, status:0x%x\n", status);
@@ -1742,7 +1740,7 @@ func_end:
  *      This does a WMD_BRD_Stop, DEV_Destroy2 and WMD_BRD_Monitor.
  *      In DEV_Destroy2 we delete the node manager.
  *  Parameters:
- *      hProcObject:    Handle to Processor Object
+ *      pProcObject:    Pointer to Processor Object
  *  Returns:
  *      DSP_SOK:	Processor placed in monitor mode.
  *      !DSP_SOK:       Failed to place processor in monitor mode.
@@ -1751,10 +1749,9 @@ func_end:
  *  Ensures:
  *      Success:	ProcObject state is PROC_IDLE
  */
-static DSP_STATUS PROC_Monitor(struct PROC_OBJECT *hProcObject)
+static DSP_STATUS PROC_Monitor(struct PROC_OBJECT *pProcObject)
 {
 	DSP_STATUS status = DSP_EFAIL;
-	struct PROC_OBJECT *pProcObject = (struct PROC_OBJECT *)hProcObject;
 	struct MSG_MGR *hMsgMgr;
 #ifdef CONFIG_BRIDGE_DEBUG
 	BRD_STATUS uBrdState;
@@ -1764,7 +1761,7 @@ static DSP_STATUS PROC_Monitor(struct PROC_OBJECT *hProcObject)
 	DBC_Require(MEM_IsValidHandle(pProcObject, PROC_SIGNATURE));
 
 	GT_1trace(PROC_DebugMask, GT_ENTER, "Entered PROC_Monitor, args:\n\t"
-		 "hProcessor: 0x%x\n", hProcObject);
+		 "hProcessor: 0x%x\n", pProcObject);
 	/* This is needed only when Device is loaded when it is
 	 * already 'ACTIVE' */
 	/* Destory the Node Manager, MSG Manager */
-- 
1.6.0.4

[PATCH 2/2] dspbridge: proc: fix a double-free on 2 error paths

[Phil Carmody]

From: Phil Carmody <ext-phil.2.carmody@nokia.com>

We free in the tail cleanup, so don't free before jumping there.

Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
---
 drivers/dsp/bridge/rmgr/proc.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/dsp/bridge/rmgr/proc.c b/drivers/dsp/bridge/rmgr/proc.c
index 7bc1bcd..e89077b 100644
--- a/drivers/dsp/bridge/rmgr/proc.c
+++ b/drivers/dsp/bridge/rmgr/proc.c
@@ -349,13 +349,11 @@ DSP_STATUS PROC_AutoStart(struct CFG_DEVNODE *hDevNode,
 		status = DEV_GetWMDContext(hDevObject,
 					&pProcObject->hWmdContext);
 		if (DSP_FAILED(status)) {
-			MEM_FreeObject(pProcObject);
 			GT_0trace(PROC_DebugMask, GT_7CLASS,
 				 "PROC_AutoStart: Failed "
 				 "to get WMD Context \n");
 		}
 	} else {
-		MEM_FreeObject(pProcObject);
 		GT_0trace(PROC_DebugMask, GT_7CLASS,
 			 "PROC_AutoStart: Failed to "
 			 "get IntFxns \n");
@@ -377,6 +375,10 @@ DSP_STATUS PROC_AutoStart(struct CFG_DEVNODE *hDevNode,
 			 "CFG_GetAutoStart DSP_FAILED \n");
 		goto func_cont;
 	}
+
+	/* paranoid - must be able to kfree this on remaining error paths */
+	pProcObject->g_pszLastCoff = NULL;
+
 	/* Get the default executable for this board... */
 	DEV_GetDevType(hDevObject, (u32 *)&devType);
 	pProcObject->uProcessor = devType;
@@ -406,9 +408,9 @@ DSP_STATUS PROC_AutoStart(struct CFG_DEVNODE *hDevNode,
 		GT_0trace(PROC_DebugMask, GT_7CLASS, "PROC_AutoStart: "
 			 "No Exec file found \n");
 	}
-func_cont:
 	kfree(pProcObject->g_pszLastCoff);
 	pProcObject->g_pszLastCoff = NULL;
+func_cont:
 	MEM_FreeObject(pProcObject);
 func_end:
 	GT_1trace(PROC_DebugMask, GT_ENTER,
-- 
1.6.0.4

Re: [PATCH 2/2] dspbridge: proc: fix a double-free on 2 error paths

[Omar Ramirez Luna]

On 2/12/2010 10:26 AM, Phil Carmody wrote:
> From: Phil Carmody<ext-phil.2.carmody@nokia.com>
>
> We free in the tail cleanup, so don't free before jumping there.
>
> Signed-off-by: Phil Carmody<ext-phil.2.carmody@nokia.com>
> ---

[...]
>   	}
> +
> +	/* paranoid - must be able to kfree this on remaining error paths */
> +	pProcObject->g_pszLastCoff = NULL;
> +

do we need this? afaik it should be NULL at this point, because of 
kmalloc + memset (which btw should be replaced by kzalloc).

i.e:
MEM_AllocObject(pProcObject, struct PROC_OBJECT, PROC_SIGNATURE);
-> MEM_Calloc(sizeof(Obj), MEM_NONPAGED);
->	pMem = kmalloc(...)
	if (pMem)
		memset(pMem, 0, cBytes);

>   	/* Get the default executable for this board... */
>   	DEV_GetDevType(hDevObject, (u32 *)&devType);
>   	pProcObject->uProcessor = devType;
[...]

Regards,

Omar

Re: [PATCH 2/2] dspbridge: proc: fix a double-free on 2 error paths

[Phil Carmody]

On 17/02/10 01:24 +0100, ext Omar Ramirez Luna wrote:
> On 2/12/2010 10:26 AM, Phil Carmody wrote:
> > From: Phil Carmody<ext-phil.2.carmody@nokia.com>
> >
> > We free in the tail cleanup, so don't free before jumping there.
> >
> > Signed-off-by: Phil Carmody<ext-phil.2.carmody@nokia.com>
> > ---
> 
> [...]
> >   	}
> > +
> > +	/* paranoid - must be able to kfree this on remaining error paths */
> > +	pProcObject->g_pszLastCoff = NULL;
> > +
> 
> do we need this? afaik it should be NULL at this point, because of 
> kmalloc + memset (which btw should be replaced by kzalloc).

I suspected it wasn't. I confess I was a little bit rushed when
looking at that.

> i.e:
> MEM_AllocObject(pProcObject, struct PROC_OBJECT, PROC_SIGNATURE);
> -> MEM_Calloc(sizeof(Obj), MEM_NONPAGED);
> ->	pMem = kmalloc(...)
> 	if (pMem)
> 		memset(pMem, 0, cBytes);

i.e. kzalloc().

> >   	/* Get the default executable for this board... */
> >   	DEV_GetDevType(hDevObject, (u32 *)&devType);
> >   	pProcObject->uProcessor = devType;
> [...]
> 
> Regards,

I can resend without, if you like, whatever's simplest for you.

Phil

Re: [PATCH 2/2] dspbridge: proc: fix a double-free on 2 error paths

[Omar Ramirez Luna]

On 2/17/2010 7:12 AM, Phil Carmody wrote:
> On 17/02/10 01:24 +0100, ext Omar Ramirez Luna wrote:
>> On 2/12/2010 10:26 AM, Phil Carmody wrote:
>>> From: Phil Carmody<ext-phil.2.carmody@nokia.com>
>>>
>>> We free in the tail cleanup, so don't free before jumping there.
>>>
>>> Signed-off-by: Phil Carmody<ext-phil.2.carmody@nokia.com>
>>> ---
>>
>> [...]
>>>    	}
>>> +
>>> +	/* paranoid - must be able to kfree this on remaining error paths */
>>> +	pProcObject->g_pszLastCoff = NULL;
>>> +
>>
>> do we need this? afaik it should be NULL at this point, because of
>> kmalloc + memset (which btw should be replaced by kzalloc).
>
> I suspected it wasn't. I confess I was a little bit rushed when
> looking at that.
>
>> i.e:
>> MEM_AllocObject(pProcObject, struct PROC_OBJECT, PROC_SIGNATURE);
>> ->  MEM_Calloc(sizeof(Obj), MEM_NONPAGED);
>> ->	pMem = kmalloc(...)
>> 	if (pMem)
>> 		memset(pMem, 0, cBytes);
>
> i.e. kzalloc().
>
>>>    	/* Get the default executable for this board... */
>>>    	DEV_GetDevType(hDevObject, (u32 *)&devType);
>>>    	pProcObject->uProcessor = devType;
>> [...]
>>
>> Regards,
>
> I can resend without, if you like, whatever's simplest for you.
>

No worries, I'll remove from original.

Regards,

Omar

Re: [PATCH 2/2] dspbridge: proc: fix a double-free on 2 error paths

[Omar Ramirez Luna]

On 2/12/2010 10:26 AM, Phil Carmody wrote:
> From: Phil Carmody<ext-phil.2.carmody@nokia.com>
>
> We free in the tail cleanup, so don't free before jumping there.
>
> Signed-off-by: Phil Carmody<ext-phil.2.carmody@nokia.com>
> ---
>   drivers/dsp/bridge/rmgr/proc.c |    8 +++++---
>   1 files changed, 5 insertions(+), 3 deletions(-)
>

Removed NULL assignment as agreed.

Acked-by: Omar Ramirez Luna <omar.ramirez@ti.com>

Pushed to dspbridge.

- omar

Re: [PATCH 1/2] dspbridge: proc: it's a pointer, not a handle

[Omar Ramirez Luna]

On 2/12/2010 10:26 AM, Phil Carmody wrote:
> From: Phil Carmody<ext-phil.2.carmody@nokia.com>
>
> If there's one thing worse than systems hungarian notation,
> it's misleading systems hungarian notation. And you don't
> need 2 copies of the pointer.
>
> Signed-off-by: Phil Carmody<ext-phil.2.carmody@nokia.com>
> ---
>   drivers/dsp/bridge/rmgr/proc.c |   25 +++++++++++--------------
>   1 files changed, 11 insertions(+), 14 deletions(-)
>

Acked-by: Omar Ramirez Luna <omar.ramirez@ti.com>

Pushed to dspbridge.

- omar