On Fri, 2009-07-03 at 20:39 +0200, ext Guzman Lugo, Fernando wrote: > Please see my comments below. > > > -----Original Message----- > > From: Ameya Palande [mailto:ameya.palande@xxxxxxxxx] > > Sent: Thursday, July 02, 2009 9:21 AM > > To: linux-omap@xxxxxxxxxxxxxxx > > Cc: Guzman Lugo, Fernando; Kanigeri, Hari; ext-phil.2.carmody@xxxxxxxxx > > Subject: [PATCH 3/4] DSPBRIDGE: PROCWRAP_Load function cleanup in a > > complete mess > > > > From: Phil Carmody <ext-phil.2.carmody@xxxxxxxxx> > > > > If you followed some failure paths, it was entirely possible that > > you'd attempt to MEM_Free a user-space pointer, because it wouldn't > > have been replaced with a kernel-space copy yet. Now replace all > > untouched pointers with NULL, so the cleanup does nothing for those > > ones. > > > > Signed-off-by: Phil Carmody <ext-phil.2.carmody@xxxxxxxxx> > > Signed-off-by: Ameya Palande <ameya.palande@xxxxxxxxx> > > --- > > drivers/dsp/bridge/pmgr/wcd.c | 95 +++++++++++++++++++++++------------- > > ---- > > 1 files changed, 55 insertions(+), 40 deletions(-) > > > > diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c > > index 00b2770..2fd9f8c 100644 > > --- a/drivers/dsp/bridge/pmgr/wcd.c > > +++ b/drivers/dsp/bridge/pmgr/wcd.c > > @@ -880,83 +880,99 @@ u32 PROCWRAP_Load(union Trapped_Args *args) > > { > > s32 i, len; > > DSP_STATUS status = DSP_SOK; > > - char *temp; > > - s32 argc = args->ARGS_PROC_LOAD.iArgc; > > + char *temp; > > + s32 count = args->ARGS_PROC_LOAD.iArgc; > > u8 **argv, **envp = NULL; > > > > + DBC_Require(count > 0); > > + DBC_Require(count <= MAX_LOADARGS); > > > > - DBC_Require(argc > 0); > > - DBC_Require(argc <= MAX_LOADARGS); > > - > > - argv = MEM_Alloc(argc * sizeof(u8 *), MEM_NONPAGED); > > - if (argv == NULL) > > + argv = MEM_Alloc(count * sizeof(u8 *), MEM_NONPAGED); > > + if (!argv) { > > status = DSP_EMEMORY; > > + goto func_cont; > > + } > > > > - cp_fm_usr(argv, args->ARGS_PROC_LOAD.aArgv, status, argc); > > - if (DSP_FAILED(status)) > > + cp_fm_usr(argv, args->ARGS_PROC_LOAD.aArgv, status, count); > > + if (DSP_FAILED(status)) { > > + MEM_Free(argv); > > + argv = NULL; > > goto func_cont; > > + } > > > > - for (i = 0; DSP_SUCCEEDED(status) && (i < argc); i++) { > > + for (i = 0; DSP_SUCCEEDED(status) && (i < count); i++) { > > if (argv[i] != NULL) { > > /* User space pointer to argument */ > > - temp = (char *) argv[i]; > > - len = strlen_user((char *)temp); > > + temp = (char *) argv[i]; > > + len = strlen_user((char *)temp); > > /* Kernel space pointer to argument */ > > argv[i] = MEM_Alloc(len + 1, MEM_NONPAGED); > > - if (argv[i] == NULL) { > > + if (argv[i] != NULL) { > > + cp_fm_usr(argv[i], temp, status, len); > > + } else { > > status = DSP_EMEMORY; > > - break; > > } > > Brackets should be removed from if and else sentences if only one statement follow them. In normal C, yes. However, your code uses rather unpleasant macros for cp_fm_usr() which will mess things up horribly inside an if/else without curly brackets. The real fix should be to fix the macro as well to use the do {} while 0 technique - I shall propose that to Ameya. > > - cp_fm_usr(argv[i], temp, status, len); > > - if (DSP_FAILED(status)) > > + > > + if (DSP_FAILED(status)) { > > + while (i < count) > > + argv[i++] = NULL; > > In this point memory has been allocated to argv[i] and you are assigned argv[i] to null losing that reference and provoking a memory leak. Other think due to the check for freeing " for (i = 0; (i < count) && argv[i]; i++)" it only frees memory until it finds the first null (or until count), assigning argv[i+1] to null is enough and the "while" is not needed. Agree - good catch! Ditto other 2, will resend. (Regarding patch 2, we'll re-work that too.) Phil -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
