* [PATCH] xfs_io: don't leak fd in open -Tr failure case
@ 2014-02-27 20:25 Eric Sandeen
2014-02-27 20:32 ` Eric Sandeen
2014-02-27 20:41 ` [PATCH V2] xfs_io: test for invalid -Tr flag combination before open Eric Sandeen
0 siblings, 2 replies; 4+ messages in thread
From: Eric Sandeen @ 2014-02-27 20:25 UTC (permalink / raw)
To: xfs-oss
Coverity spotted this.
It complained that we didn't close the fd before returning in
this case of incompatible options, but it seems like we should
just test for the incompatible flags before even trying to open
the file, no?
(The open would have failed in any case, but with a somewhat
cryptic "Invalid argument" - so it's probably better to state
it plainly and bail immediately.)
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
diff --git a/io/open.c b/io/open.c
index 6bb0d46..c106fa7 100644
--- a/io/open.c
+++ b/io/open.c
@@ -342,6 +342,11 @@ open_f(
if (optind != argc - 1)
return command_usage(&open_cmd);
+ if ((flags & (IO_READONLY|IO_TMPFILE)) == (IO_READONLY|IO_TMPFILE)) {
+ fprintf(stderr, _("-T and -r options are incompatible\n"));
+ return -1;
+ }
+
fd = openfile(argv[optind], &geometry, flags, mode);
if (fd < 0)
return 0;
@@ -349,11 +354,6 @@ open_f(
if (!platform_test_xfs_fd(fd))
flags |= IO_FOREIGN;
- if ((flags & (IO_READONLY|IO_TMPFILE)) == (IO_READONLY|IO_TMPFILE)) {
- fprintf(stderr, _("-T and -r options are incompatible\n"));
- return -1;
- }
-
addfile(argv[optind], fd, &geometry, flags);
return 0;
}
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs_io: don't leak fd in open -Tr failure case
2014-02-27 20:25 [PATCH] xfs_io: don't leak fd in open -Tr failure case Eric Sandeen
@ 2014-02-27 20:32 ` Eric Sandeen
2014-02-27 20:34 ` Christoph Hellwig
2014-02-27 20:41 ` [PATCH V2] xfs_io: test for invalid -Tr flag combination before open Eric Sandeen
1 sibling, 1 reply; 4+ messages in thread
From: Eric Sandeen @ 2014-02-27 20:32 UTC (permalink / raw)
To: Eric Sandeen, xfs-oss
On 2/27/14, 2:25 PM, Eric Sandeen wrote:
> Coverity spotted this.
>
> It complained that we didn't close the fd before returning in
> this case of incompatible options, but it seems like we should
> just test for the incompatible flags before even trying to open
> the file, no?
>
> (The open would have failed in any case, but with a somewhat
> cryptic "Invalid argument" - so it's probably better to state
> it plainly and bail immediately.)
So actually, we wouldn't leak, because the open would fail.
So I guess it's not the best subject & description...
-Eric
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> diff --git a/io/open.c b/io/open.c
> index 6bb0d46..c106fa7 100644
> --- a/io/open.c
> +++ b/io/open.c
> @@ -342,6 +342,11 @@ open_f(
> if (optind != argc - 1)
> return command_usage(&open_cmd);
>
> + if ((flags & (IO_READONLY|IO_TMPFILE)) == (IO_READONLY|IO_TMPFILE)) {
> + fprintf(stderr, _("-T and -r options are incompatible\n"));
> + return -1;
> + }
> +
> fd = openfile(argv[optind], &geometry, flags, mode);
> if (fd < 0)
> return 0;
> @@ -349,11 +354,6 @@ open_f(
> if (!platform_test_xfs_fd(fd))
> flags |= IO_FOREIGN;
>
> - if ((flags & (IO_READONLY|IO_TMPFILE)) == (IO_READONLY|IO_TMPFILE)) {
> - fprintf(stderr, _("-T and -r options are incompatible\n"));
> - return -1;
> - }
> -
> addfile(argv[optind], fd, &geometry, flags);
> return 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] 4+ messages in thread
* Re: [PATCH] xfs_io: don't leak fd in open -Tr failure case
2014-02-27 20:32 ` Eric Sandeen
@ 2014-02-27 20:34 ` Christoph Hellwig
0 siblings, 0 replies; 4+ messages in thread
From: Christoph Hellwig @ 2014-02-27 20:34 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Eric Sandeen, xfs-oss
On Thu, Feb 27, 2014 at 02:32:47PM -0600, Eric Sandeen wrote:
> On 2/27/14, 2:25 PM, Eric Sandeen wrote:
> > Coverity spotted this.
> >
> > It complained that we didn't close the fd before returning in
> > this case of incompatible options, but it seems like we should
> > just test for the incompatible flags before even trying to open
> > the file, no?
> >
> > (The open would have failed in any case, but with a somewhat
> > cryptic "Invalid argument" - so it's probably better to state
> > it plainly and bail immediately.)
>
> So actually, we wouldn't leak, because the open would fail.
> So I guess it's not the best subject & description...
It's not, but doing the check earlier seems sensible.
Reviewed-by: Christoph Hellwig <hch@lst.de>
(with a better description)
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH V2] xfs_io: test for invalid -Tr flag combination before open
2014-02-27 20:25 [PATCH] xfs_io: don't leak fd in open -Tr failure case Eric Sandeen
2014-02-27 20:32 ` Eric Sandeen
@ 2014-02-27 20:41 ` Eric Sandeen
1 sibling, 0 replies; 4+ messages in thread
From: Eric Sandeen @ 2014-02-27 20:41 UTC (permalink / raw)
To: Eric Sandeen, xfs-oss
Coverity spotted this.
It complained that we didn't close the fd before returning in
the error case of incompatible options, but in reality, we wouldn't
have gotten that far because open(O_RDONLY|O_TMPFILE) would be
rejected with EINVAL.
So the error handling test would never actually be true.
Fix this by moving the error checking prior to the open so
the user gets a more useful error message than "Invalid Argument."
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
V2: New summary & commit message
diff --git a/io/open.c b/io/open.c
index 6bb0d46..c106fa7 100644
--- a/io/open.c
+++ b/io/open.c
@@ -342,6 +342,11 @@ open_f(
if (optind != argc - 1)
return command_usage(&open_cmd);
+ if ((flags & (IO_READONLY|IO_TMPFILE)) == (IO_READONLY|IO_TMPFILE)) {
+ fprintf(stderr, _("-T and -r options are incompatible\n"));
+ return -1;
+ }
+
fd = openfile(argv[optind], &geometry, flags, mode);
if (fd < 0)
return 0;
@@ -349,11 +354,6 @@ open_f(
if (!platform_test_xfs_fd(fd))
flags |= IO_FOREIGN;
- if ((flags & (IO_READONLY|IO_TMPFILE)) == (IO_READONLY|IO_TMPFILE)) {
- fprintf(stderr, _("-T and -r options are incompatible\n"));
- return -1;
- }
-
addfile(argv[optind], fd, &geometry, flags);
return 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 related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-02-27 20:41 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-27 20:25 [PATCH] xfs_io: don't leak fd in open -Tr failure case Eric Sandeen
2014-02-27 20:32 ` Eric Sandeen
2014-02-27 20:34 ` Christoph Hellwig
2014-02-27 20:41 ` [PATCH V2] xfs_io: test for invalid -Tr flag combination before open Eric Sandeen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.