* [PATCH v2] xfsdump: Fix memory and fd leak in invutil/stobj.c
@ 2014-03-07 14:52 Boris Ranto
2014-03-07 18:48 ` Brian Foster
0 siblings, 1 reply; 3+ messages in thread
From: Boris Ranto @ 2014-03-07 14:52 UTC (permalink / raw)
To: xfs; +Cc: Boris Ranto, branto
The function open_stobj duplicates its argument, upon successful
duplication, the fstat is called. If the fstat command fails then
the memory for the duplicated string is leaked. Fix this by moving
the string duplication after the fstat call. This is ok because
the fstat call does not use the duplicated string.
Brian Foster noticed that the function also leaks a file descriptor
in case the file cannot be fstated. Fixing that, too.
Signed-off-by: Boris Ranto <ranto.boris@gmail.com>
---
invutil/stobj.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/invutil/stobj.c b/invutil/stobj.c
index 428b419..a74ba0f 100644
--- a/invutil/stobj.c
+++ b/invutil/stobj.c
@@ -578,23 +578,24 @@ open_stobj(char *StObjFileName)
return fd;
}
- name = strdup(StObjFileName);
- if(name == NULL) {
- fprintf(stderr, "%s: internal memory error: strdup stobj_name\n", g_programName);
- exit(1);
- }
-
read_n_bytes(fd, &cnt, sizeof(invt_sescounter_t), StObjFileName);
lseek( fd, 0, SEEK_SET );
errno = 0;
if (fstat(fd, &sb) < 0) {
fprintf(stderr, "Could not get stat info on %s\n", StObjFileName);
perror("fstat");
+ close(fd);
return -1;
}
size = sb.st_size;
mapaddr = mmap_n_bytes(fd, size, BOOL_FALSE, StObjFileName);
+ name = strdup(StObjFileName);
+ if(name == NULL) {
+ fprintf(stderr, "%s: internal memory error: strdup stobj_name\n", g_programName);
+ exit(1);
+ }
+
return add_stobj(name, fd, size, mapaddr, (invt_sescounter_t *)mapaddr);
}
--
1.9.0
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2] xfsdump: Fix memory and fd leak in invutil/stobj.c
2014-03-07 14:52 [PATCH v2] xfsdump: Fix memory and fd leak in invutil/stobj.c Boris Ranto
@ 2014-03-07 18:48 ` Brian Foster
2014-03-11 13:55 ` Boris Ranto
0 siblings, 1 reply; 3+ messages in thread
From: Brian Foster @ 2014-03-07 18:48 UTC (permalink / raw)
To: Boris Ranto; +Cc: branto, xfs
On Fri, Mar 07, 2014 at 03:52:42PM +0100, Boris Ranto wrote:
> The function open_stobj duplicates its argument, upon successful
> duplication, the fstat is called. If the fstat command fails then
> the memory for the duplicated string is leaked. Fix this by moving
> the string duplication after the fstat call. This is ok because
> the fstat call does not use the duplicated string.
>
> Brian Foster noticed that the function also leaks a file descriptor
> in case the file cannot be fstated. Fixing that, too.
>
> Signed-off-by: Boris Ranto <ranto.boris@gmail.com>
> ---
> invutil/stobj.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/invutil/stobj.c b/invutil/stobj.c
> index 428b419..a74ba0f 100644
> --- a/invutil/stobj.c
> +++ b/invutil/stobj.c
> @@ -578,23 +578,24 @@ open_stobj(char *StObjFileName)
> return fd;
> }
>
> - name = strdup(StObjFileName);
> - if(name == NULL) {
> - fprintf(stderr, "%s: internal memory error: strdup stobj_name\n", g_programName);
> - exit(1);
> - }
> -
> read_n_bytes(fd, &cnt, sizeof(invt_sescounter_t), StObjFileName);
> lseek( fd, 0, SEEK_SET );
> errno = 0;
> if (fstat(fd, &sb) < 0) {
> fprintf(stderr, "Could not get stat info on %s\n", StObjFileName);
> perror("fstat");
> + close(fd);
> return -1;
> }
> size = sb.st_size;
> mapaddr = mmap_n_bytes(fd, size, BOOL_FALSE, StObjFileName);
>
> + name = strdup(StObjFileName);
> + if(name == NULL) {
> + fprintf(stderr, "%s: internal memory error: strdup stobj_name\n", g_programName);
> + exit(1);
> + }
> +
> return add_stobj(name, fd, size, mapaddr, (invt_sescounter_t *)mapaddr);
> }
>
The remaining failure cases all appear to exit(), including those in
add_stobj(). Looks good to me, thanks for making that fix...
Reviewed-by: Brian Foster <bfoster@redhat.com>
> --
> 1.9.0
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] xfsdump: Fix memory and fd leak in invutil/stobj.c
2014-03-07 18:48 ` Brian Foster
@ 2014-03-11 13:55 ` Boris Ranto
0 siblings, 0 replies; 3+ messages in thread
From: Boris Ranto @ 2014-03-11 13:55 UTC (permalink / raw)
To: Brian Foster; +Cc: Boris Ranto, xfs
On Fri, 2014-03-07 at 13:48 -0500, Brian Foster wrote:
> On Fri, Mar 07, 2014 at 03:52:42PM +0100, Boris Ranto wrote:
> > The function open_stobj duplicates its argument, upon successful
> > duplication, the fstat is called. If the fstat command fails then
> > the memory for the duplicated string is leaked. Fix this by moving
> > the string duplication after the fstat call. This is ok because
> > the fstat call does not use the duplicated string.
> >
> > Brian Foster noticed that the function also leaks a file descriptor
> > in case the file cannot be fstated. Fixing that, too.
> >
> > Signed-off-by: Boris Ranto <ranto.boris@gmail.com>
> > ---
> > invutil/stobj.c | 13 +++++++------
> > 1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/invutil/stobj.c b/invutil/stobj.c
> > index 428b419..a74ba0f 100644
> > --- a/invutil/stobj.c
> > +++ b/invutil/stobj.c
> > @@ -578,23 +578,24 @@ open_stobj(char *StObjFileName)
> > return fd;
> > }
> >
> > - name = strdup(StObjFileName);
> > - if(name == NULL) {
> > - fprintf(stderr, "%s: internal memory error: strdup stobj_name\n", g_programName);
> > - exit(1);
> > - }
> > -
> > read_n_bytes(fd, &cnt, sizeof(invt_sescounter_t), StObjFileName);
> > lseek( fd, 0, SEEK_SET );
> > errno = 0;
> > if (fstat(fd, &sb) < 0) {
> > fprintf(stderr, "Could not get stat info on %s\n", StObjFileName);
> > perror("fstat");
> > + close(fd);
> > return -1;
> > }
> > size = sb.st_size;
> > mapaddr = mmap_n_bytes(fd, size, BOOL_FALSE, StObjFileName);
> >
> > + name = strdup(StObjFileName);
> > + if(name == NULL) {
> > + fprintf(stderr, "%s: internal memory error: strdup stobj_name\n", g_programName);
> > + exit(1);
> > + }
> > +
> > return add_stobj(name, fd, size, mapaddr, (invt_sescounter_t *)mapaddr);
> > }
> >
>
> The remaining failure cases all appear to exit(), including those in
> add_stobj(). Looks good to me, thanks for making that fix...
>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
>
No problem, thanks for the review. And yes, I've also looked at the
remaining cases and did not update them because they all just exited.
Boris
> > --
> > 1.9.0
> >
> > _______________________________________________
> > xfs mailing list
> > xfs@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-03-11 13:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-07 14:52 [PATCH v2] xfsdump: Fix memory and fd leak in invutil/stobj.c Boris Ranto
2014-03-07 18:48 ` Brian Foster
2014-03-11 13:55 ` Boris Ranto
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox