From: Thomas R. <co...@st...> - 2011-03-09 14:51:20
|
This patch removes a potential memory leak in make_tempfile (in arch/um/os-Linux/mem.c). If tempdir is NULL or as long as MAXPATHLEN, the function will return without freeing the previously malloced tempname, resulting in a memory leak. Another way to fix this issue is to replace : if ((tempdir == NULL) || (strlen(tempdir) >= MAXPATHLEN)) return -1; with: if ((tempdir == NULL) || (strlen(tempdir) >= MAXPATHLEN)) goto out; But I think the way in the attached patch is cleaner, as it doesn't malloc tempname at all if it's not necessary. Cheers, Thomas --- arch/um/os-Linux/mem.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/arch/um/os-Linux/mem.c b/arch/um/os-Linux/mem.c index e696144..e135422 100644 --- a/arch/um/os-Linux/mem.c +++ b/arch/um/os-Linux/mem.c @@ -170,14 +170,14 @@ static int __init make_tempfile(const char *template, char **out_tempname, int fd; which_tmpdir(); - tempname = malloc(MAXPATHLEN); - if (tempname == NULL) - return -1; - find_tempdir(); if ((tempdir == NULL) || (strlen(tempdir) >= MAXPATHLEN)) return -1; + tempname = malloc(MAXPATHLEN); + if (tempname == NULL) + return -1; + if (template[0] != '/') strcpy(tempname, tempdir); else -- 1.7.2.3 |
From: richard -r. w. <ric...@gm...> - 2011-03-09 15:53:47
|
On Wed, Mar 9, 2011 at 3:21 PM, Thomas Roth <co...@st...> wrote: > This patch removes a potential memory leak in make_tempfile (in > arch/um/os-Linux/mem.c). This is not really a problem because UML terminates anyway when tempdir == NULL. Why should we waste CPU cycles with free()ing memory in the exit path? > If tempdir is NULL or as long as MAXPATHLEN, the function will return > without freeing the previously > malloced tempname, resulting in a memory leak. Another way to fix this > issue is to replace : > if ((tempdir == NULL) || (strlen(tempdir) >= MAXPATHLEN)) > return -1; > with: > if ((tempdir == NULL) || (strlen(tempdir) >= MAXPATHLEN)) > goto out; > > But I think the way in the attached patch is cleaner, as it doesn't > malloc tempname at all if it's not necessary. > > Cheers, > > Thomas > > --- > arch/um/os-Linux/mem.c | 8 ++++---- > 1 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/um/os-Linux/mem.c b/arch/um/os-Linux/mem.c > index e696144..e135422 100644 > --- a/arch/um/os-Linux/mem.c > +++ b/arch/um/os-Linux/mem.c > @@ -170,14 +170,14 @@ static int __init make_tempfile(const char > *template, char **out_tempname, > int fd; > > which_tmpdir(); > - tempname = malloc(MAXPATHLEN); > - if (tempname == NULL) > - return -1; > - > find_tempdir(); > if ((tempdir == NULL) || (strlen(tempdir) >= MAXPATHLEN)) > return -1; > > + tempname = malloc(MAXPATHLEN); > + if (tempname == NULL) > + return -1; > + > if (template[0] != '/') > strcpy(tempname, tempdir); > else > -- > 1.7.2.3 > > ------------------------------------------------------------------------------ > Colocation vs. Managed Hosting > A question and answer guide to determining the best fit > for your organization - today and in the future. > http://p.sf.net/sfu/internap-sfd2d > _______________________________________________ > User-mode-linux-devel mailing list > Use...@li... > https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel > -- Thanks, //richard |
From: Thomas R. <co...@st...> - 2011-03-09 16:08:45
|
On Wed, Mar 9, 2011 at 4:53 PM, richard -rw- weinberger <ric...@gm...> wrote: > On Wed, Mar 9, 2011 at 3:21 PM, Thomas Roth <co...@st...> wrote: >> This patch removes a potential memory leak in make_tempfile (in >> arch/um/os-Linux/mem.c). > > This is not really a problem because UML terminates anyway > when tempdir == NULL. > Why should we waste CPU cycles with free()ing memory in the exit path? This patch doesn't add any overhead to the function, as no additional code was introduced. The 'old' way now wastes more CPU cycles if tempdir is NULL. I just stumbled upon it while looking for something entirely different and it didn't look right to me. >> If tempdir is NULL or as long as MAXPATHLEN, the function will return >> without freeing the previously >> malloced tempname, resulting in a memory leak. Another way to fix this >> issue is to replace : >> if ((tempdir == NULL) || (strlen(tempdir) >= MAXPATHLEN)) >> return -1; >> with: >> if ((tempdir == NULL) || (strlen(tempdir) >= MAXPATHLEN)) >> goto out; >> >> But I think the way in the attached patch is cleaner, as it doesn't >> malloc tempname at all if it's not necessary. >> >> Cheers, >> >> Thomas >> >> --- >> arch/um/os-Linux/mem.c | 8 ++++---- >> 1 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/arch/um/os-Linux/mem.c b/arch/um/os-Linux/mem.c >> index e696144..e135422 100644 >> --- a/arch/um/os-Linux/mem.c >> +++ b/arch/um/os-Linux/mem.c >> @@ -170,14 +170,14 @@ static int __init make_tempfile(const char >> *template, char **out_tempname, >> int fd; >> >> which_tmpdir(); >> - tempname = malloc(MAXPATHLEN); >> - if (tempname == NULL) >> - return -1; >> - >> find_tempdir(); >> if ((tempdir == NULL) || (strlen(tempdir) >= MAXPATHLEN)) >> return -1; >> >> + tempname = malloc(MAXPATHLEN); >> + if (tempname == NULL) >> + return -1; >> + >> if (template[0] != '/') >> strcpy(tempname, tempdir); >> else >> -- >> 1.7.2.3 >> >> ------------------------------------------------------------------------------ >> Colocation vs. Managed Hosting >> A question and answer guide to determining the best fit >> for your organization - today and in the future. >> http://p.sf.net/sfu/internap-sfd2d >> _______________________________________________ >> User-mode-linux-devel mailing list >> Use...@li... >> https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel >> > > -- > Thanks, > //richard > |
From: richard -r. w. <ric...@gm...> - 2011-03-09 16:15:11
|
On Wed, Mar 9, 2011 at 5:01 PM, Thomas Roth <co...@st...> wrote: > On Wed, Mar 9, 2011 at 4:53 PM, richard -rw- weinberger > <ric...@gm...> wrote: >> On Wed, Mar 9, 2011 at 3:21 PM, Thomas Roth <co...@st...> wrote: >>> This patch removes a potential memory leak in make_tempfile (in >>> arch/um/os-Linux/mem.c). >> >> This is not really a problem because UML terminates anyway >> when tempdir == NULL. >> Why should we waste CPU cycles with free()ing memory in the exit path? > > This patch doesn't add any overhead to the function, as no additional code was > introduced. The 'old' way now wastes more CPU cycles if tempdir is NULL. I > just stumbled upon it while looking for something entirely different > and it didn't > look right to me. Sorry, my fault. Your path does not add code. But it also does not fix any real problem. >>> If tempdir is NULL or as long as MAXPATHLEN, the function will return >>> without freeing the previously >>> malloced tempname, resulting in a memory leak. Another way to fix this >>> issue is to replace : >>> if ((tempdir == NULL) || (strlen(tempdir) >= MAXPATHLEN)) >>> return -1; >>> with: >>> if ((tempdir == NULL) || (strlen(tempdir) >= MAXPATHLEN)) >>> goto out; >>> >>> But I think the way in the attached patch is cleaner, as it doesn't >>> malloc tempname at all if it's not necessary. >>> >>> Cheers, >>> >>> Thomas >>> >>> --- >>> arch/um/os-Linux/mem.c | 8 ++++---- >>> 1 files changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/arch/um/os-Linux/mem.c b/arch/um/os-Linux/mem.c >>> index e696144..e135422 100644 >>> --- a/arch/um/os-Linux/mem.c >>> +++ b/arch/um/os-Linux/mem.c >>> @@ -170,14 +170,14 @@ static int __init make_tempfile(const char >>> *template, char **out_tempname, >>> int fd; >>> >>> which_tmpdir(); >>> - tempname = malloc(MAXPATHLEN); >>> - if (tempname == NULL) >>> - return -1; >>> - >>> find_tempdir(); >>> if ((tempdir == NULL) || (strlen(tempdir) >= MAXPATHLEN)) >>> return -1; >>> >>> + tempname = malloc(MAXPATHLEN); >>> + if (tempname == NULL) >>> + return -1; >>> + >>> if (template[0] != '/') >>> strcpy(tempname, tempdir); >>> else >>> -- >>> 1.7.2.3 >>> >>> ------------------------------------------------------------------------------ >>> Colocation vs. Managed Hosting >>> A question and answer guide to determining the best fit >>> for your organization - today and in the future. >>> http://p.sf.net/sfu/internap-sfd2d >>> _______________________________________________ >>> User-mode-linux-devel mailing list >>> Use...@li... >>> https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel >>> >> >> -- >> Thanks, >> //richard >> > -- Thanks, //richard |