In the cifs_reopen_file function, if the following statement is
asserted:
(tcon->unix_ext && cap_unix(tcon->ses) &&
(CIFS_UNIX_POSIX_PATH_OPS_CAP &
(tcon->fsUnixInfo.Capability)))
and we succeed to open with cifs_posix_open, the function jumps
to the label reopen_success and checks for oparms.reconnect
which is not initialized.
To avoid this the oparms structure initialization is anticipated
before the if statement.
This issue has been reported by scan.coverity.com
Signed-off-by: Andi Shyti <[email protected]>
---
fs/cifs/file.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 1e57f36..fbeaf45 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -632,6 +632,15 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool can_flush)
else
oplock = 0;
+ oparms.tcon = tcon;
+ oparms.cifs_sb = cifs_sb;
+ oparms.desired_access = desired_access;
+ oparms.create_options = create_options;
+ oparms.disposition = disposition;
+ oparms.path = full_path;
+ oparms.fid = &cfile->fid;
+ oparms.reconnect = true;
+
if (tcon->unix_ext && cap_unix(tcon->ses) &&
(CIFS_UNIX_POSIX_PATH_OPS_CAP &
le64_to_cpu(tcon->fsUnixInfo.Capability))) {
@@ -663,15 +672,6 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool can_flush)
if (server->ops->get_lease_key)
server->ops->get_lease_key(inode, &cfile->fid);
- oparms.tcon = tcon;
- oparms.cifs_sb = cifs_sb;
- oparms.desired_access = desired_access;
- oparms.create_options = create_options;
- oparms.disposition = disposition;
- oparms.path = full_path;
- oparms.fid = &cfile->fid;
- oparms.reconnect = true;
-
/*
* Can not refresh inode by passing in file_info buf to be returned by
* CIFSSMBOpen and then calling get_inode_info with returned buf since
--
1.8.3.2
On Mon, 29 Jul 2013 10:58:13 +0200
Andi Shyti <[email protected]> wrote:
> In the cifs_reopen_file function, if the following statement is
> asserted:
>
> (tcon->unix_ext && cap_unix(tcon->ses) &&
> (CIFS_UNIX_POSIX_PATH_OPS_CAP &
> (tcon->fsUnixInfo.Capability)))
>
> and we succeed to open with cifs_posix_open, the function jumps
> to the label reopen_success and checks for oparms.reconnect
> which is not initialized.
>
> To avoid this the oparms structure initialization is anticipated
> before the if statement.
>
> This issue has been reported by scan.coverity.com
>
> Signed-off-by: Andi Shyti <[email protected]>
> ---
> fs/cifs/file.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 1e57f36..fbeaf45 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -632,6 +632,15 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool can_flush)
> else
> oplock = 0;
>
> + oparms.tcon = tcon;
> + oparms.cifs_sb = cifs_sb;
> + oparms.desired_access = desired_access;
> + oparms.create_options = create_options;
This patch just moves the brokenness around. You're
setting .desired_access here to an unintialized variable.
create_options also looks like it may potentially be wrong at this
point.
It may be that the code won't trip over these bugs in its current form,
but it's not really doing much to "future-proof" it. I think this
function needs a bit more refactoring instead of increasing the level
of spaghetti.
> + oparms.disposition = disposition;
> + oparms.path = full_path;
> + oparms.fid = &cfile->fid;
> + oparms.reconnect = true;
> +
> if (tcon->unix_ext && cap_unix(tcon->ses) &&
> (CIFS_UNIX_POSIX_PATH_OPS_CAP &
> le64_to_cpu(tcon->fsUnixInfo.Capability))) {
> @@ -663,15 +672,6 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool can_flush)
> if (server->ops->get_lease_key)
> server->ops->get_lease_key(inode, &cfile->fid);
>
> - oparms.tcon = tcon;
> - oparms.cifs_sb = cifs_sb;
> - oparms.desired_access = desired_access;
> - oparms.create_options = create_options;
> - oparms.disposition = disposition;
> - oparms.path = full_path;
> - oparms.fid = &cfile->fid;
> - oparms.reconnect = true;
> -
> /*
> * Can not refresh inode by passing in file_info buf to be returned by
> * CIFSSMBOpen and then calling get_inode_info with returned buf since
--
Jeff Layton <[email protected]>
> > + oparms.tcon = tcon;
> > + oparms.cifs_sb = cifs_sb;
> > + oparms.desired_access = desired_access;
> > + oparms.create_options = create_options;
>
> This patch just moves the brokenness around. You're
> setting .desired_access here to an unintialized variable.
> create_options also looks like it may potentially be wrong at this
> point.
Urrrca! This is what I achieve when I do one last fix before
going to sleep.
I spent a bit of time more going through the cifs/smb code and
the most sensful fix looks this
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 1e57f36..7e36ae3 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -647,6 +647,7 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool can_flu
oflags, &oplock, &cfile->fid.netfid, xid
if (rc == 0) {
cifs_dbg(FYI, "posix reopen succeeded\n");
+ oparms.reconnect = true;
goto reopen_success;
}
/*
There is only one case when reconnect becames false, that is when
open = smb2_open_file and calls SMB2_open() that calls
add_durable_context() that sets reconnect = false with some
nested ifs in between, and still only in case
everything succeeds. We are checking reconnect only for this
case, otherwise we could get rid of the if (oparms.reconnect) and
not falling into the unknown state.
If it makes sense, I can send the above suggestion.
Thanks,
Andi
In the cifs_reopen_file function, if the following statement is
asserted:
(tcon->unix_ext && cap_unix(tcon->ses) &&
(CIFS_UNIX_POSIX_PATH_OPS_CAP &
(tcon->fsUnixInfo.Capability)))
and we succeed to open with cifs_posix_open, the function jumps
to the label reopen_success and checks for oparms.reconnect
which is not initialized.
This issue has been reported by scan.coverity.com
Signed-off-by: Andi Shyti <[email protected]>
---
fs/cifs/file.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 1e57f36..7e36ae3 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -647,6 +647,7 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool can_flush)
oflags, &oplock, &cfile->fid.netfid, xid);
if (rc == 0) {
cifs_dbg(FYI, "posix reopen succeeded\n");
+ oparms.reconnect = true;
goto reopen_success;
}
/*
--
1.8.3.2
On Mon, 29 Jul 2013 20:04:35 +0200
Andi Shyti <[email protected]> wrote:
> In the cifs_reopen_file function, if the following statement is
> asserted:
>
> (tcon->unix_ext && cap_unix(tcon->ses) &&
> (CIFS_UNIX_POSIX_PATH_OPS_CAP &
> (tcon->fsUnixInfo.Capability)))
>
> and we succeed to open with cifs_posix_open, the function jumps
> to the label reopen_success and checks for oparms.reconnect
> which is not initialized.
>
> This issue has been reported by scan.coverity.com
>
> Signed-off-by: Andi Shyti <[email protected]>
> ---
> fs/cifs/file.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 1e57f36..7e36ae3 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -647,6 +647,7 @@ cifs_reopen_file(struct cifsFileInfo *cfile, bool can_flush)
> oflags, &oplock, &cfile->fid.netfid, xid);
> if (rc == 0) {
> cifs_dbg(FYI, "posix reopen succeeded\n");
> + oparms.reconnect = true;
> goto reopen_success;
> }
> /*
Still doesn't do much to improve this code, but that fix shouldn't
break anything.
Reviewed-by: Jeff Layton <[email protected]>