Please see my comments bellow. > -----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 2/4] DSPBRIDGE: Heuristic fixes of strlen/malloc out by > one and termination errors > > From: Phil Carmody <ext-phil.2.carmody@xxxxxxxxx> > > I say 'heuristic', as I can't prove they're wrong, they just look > wrong, and for that reason should be given extra close scrutiny. > These are basically just the old malloc-one-more-than-strlen and > strncpy-doesn't-write-a-terminal-nil gotchas. > > Signed-off-by: Phil Carmody <ext-phil.2.carmody@xxxxxxxxx> > --- > drivers/dsp/bridge/pmgr/wcd.c | 7 ++++--- > drivers/dsp/bridge/rmgr/nldr.c | 3 ++- > drivers/dsp/bridge/rmgr/node.c | 5 +++-- > 3 files changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c > index 7732492..00b2770 100644 > --- a/drivers/dsp/bridge/pmgr/wcd.c > +++ b/drivers/dsp/bridge/pmgr/wcd.c > @@ -902,7 +902,7 @@ u32 PROCWRAP_Load(union Trapped_Args *args) > temp = (char *) argv[i]; > len = strlen_user((char *)temp); > /* Kernel space pointer to argument */ > - argv[i] = MEM_Alloc(len, MEM_NONPAGED); > + argv[i] = MEM_Alloc(len + 1, MEM_NONPAGED); > if (argv[i] == NULL) { > status = DSP_EMEMORY; > break; > @@ -910,7 +910,7 @@ u32 PROCWRAP_Load(union Trapped_Args *args) > cp_fm_usr(argv[i], temp, status, len); > if (DSP_FAILED(status)) > goto func_cont; > - > + argv[i][len] = '\0'; It is ok, what do you think changing len = strlen_user((char *)temp); to len = strlen_user((char *)temp) + 1;? With that cp_fm_usr would copy the null character and it whould replace the change in MEM_Alloc and argv[i][len] = '\0'; > } > } > /* TODO: validate this */ > @@ -935,7 +935,7 @@ u32 PROCWRAP_Load(union Trapped_Args *args) > temp = (char *)envp[i]; > len = strlen_user((char *)temp); > /* Kernel space pointer to argument */ > - envp[i] = MEM_Alloc(len, MEM_NONPAGED); > + envp[i] = MEM_Alloc(len + 1, MEM_NONPAGED); > if (envp[i] == NULL) { > status = DSP_EMEMORY; > break; > @@ -943,6 +943,7 @@ u32 PROCWRAP_Load(union Trapped_Args *args) > cp_fm_usr(envp[i], temp, status, len); > if (DSP_FAILED(status)) > goto func_cont; > + envp[i][len] = '\0'; > } > } > GT_5trace(WCD_debugMask, GT_ENTER, > diff --git a/drivers/dsp/bridge/rmgr/nldr.c > b/drivers/dsp/bridge/rmgr/nldr.c > index 79f7505..a6a0528 100644 > --- a/drivers/dsp/bridge/rmgr/nldr.c > +++ b/drivers/dsp/bridge/rmgr/nldr.c > @@ -1128,7 +1128,8 @@ static DSP_STATUS AddOvlyNode(struct DSP_UUID > *pUuid, > if (pBuf == NULL) { > status = DSP_EMEMORY; > } else { > - strncpy(pBuf, pNodeName, uLen); > + strncpy(pBuf, pNodeName, uLen); > + pBuf[uLen] = '\0'; pBuf is allocated using MEM_Calloc which allocates zero-initialized memory so that "pBuf[uLen] = '\0';" is not needed. > hNldr->ovlyTable[hNldr->nNode].pNodeName = pBuf; > hNldr->nNode++; > } > diff --git a/drivers/dsp/bridge/rmgr/node.c > b/drivers/dsp/bridge/rmgr/node.c > index 53a42bf..9f7e4d4 100644 > --- a/drivers/dsp/bridge/rmgr/node.c > +++ b/drivers/dsp/bridge/rmgr/node.c > @@ -3272,8 +3272,9 @@ static DSP_STATUS GetNodeProps(struct DCD_MANAGER > *hDcdMgr, > if (hNode->pstrDevName == NULL) { > status = DSP_EMEMORY; > } else { > - strncpy(hNode->pstrDevName, > - pndbProps->acName, uLen); > + strncpy(hNode->pstrDevName, > + pndbProps->acName, uLen); > + hNode->pstrDevName[uLen] = '\0'; hNode->pstrDevName is allocated using MEM_Calloc which allocates zero-initialized memory so that "hNode->pstrDevName[uLen] = '\0';" is not needed. > } > } > } > -- > 1.6.2.4 > Regards, Fernando Guzman Lugo. -- 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
