* Re: [PATCH] CIFS: fix structurally dead code in smb311_posix_mkdir
2018-06-21 13:35 [PATCH] CIFS: fix structurally dead code in smb311_posix_mkdir Gustavo A. R. Silva
@ 2018-06-21 16:59 ` Steve French
2018-06-21 17:36 ` Gustavo A. R. Silva
0 siblings, 1 reply; 3+ messages in thread
From: Steve French @ 2018-06-21 16:59 UTC (permalink / raw)
To: Gustavo A. R. Silva, Dan Carpenter
Cc: Aurelien Aptel, Steve French, CIFS, samba-technical, LKML
[-- Attachment #1: Type: text/plain, Size: 1487 bytes --]
Folded this patch and the equivalent one from Dan in with the comments
from Aurelien into one patch and repushed to cifs-2.6.git for-next -
see attached.
On Thu, Jun 21, 2018 at 8:35 AM, Gustavo A. R. Silva
<gustavo@embeddedor.com> wrote:
> Code at line: 1950: rc = -EIO; is unreachable. Hence, the function
> returns rc with a value of zero and, this is not the actual intention
> of this particular piece of code.
>
> Fix this by placing the goto instruction just after the update to rc.
>
> Addresses-Coverity-ID: 1470124 ("Structurally dead code")
> Fixes: 5539e9b24a38 ("CIFS: fix memory leak and remove dead code")
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
> fs/cifs/smb2pdu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 062a2a5..c9489b1 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -1946,8 +1946,8 @@ int smb311_posix_mkdir(const unsigned int xid, struct inode *inode,
> if (ses && (ses->server))
> server = ses->server;
> else {
> - goto err_free_path;
> rc = -EIO;
> + goto err_free_path;
> }
>
> /* resource #2: request */
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Thanks,
Steve
[-- Attachment #2: 0001-CIFS-fix-memory-leak-and-remove-dead-code.patch --]
[-- Type: text/x-patch, Size: 5293 bytes --]
From f877386d373753c83d791d82328356cd98812eb8 Mon Sep 17 00:00:00 2001
From: Aurelien Aptel <aaptel@suse.com>
Date: Tue, 19 Jun 2018 15:18:48 -0700
Subject: [PATCH 1/2] CIFS: fix memory leak and remove dead code
also fixes error code in smb311_posix_mkdir() (where
the error assignment needs to go before the goto)
a typo that Dan Carpenter and Paulo pointed out.
Signed-off-by: Aurelien Aptel <aaptel@suse.com>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Paulo Alcantara <palcantara@suse.de>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
fs/cifs/smb2pdu.c | 99 +++++++++++++++++++++++------------------------
1 file changed, 49 insertions(+), 50 deletions(-)
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 810b85787c91..ea102dca5f33 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -1934,27 +1934,31 @@ int smb311_posix_mkdir(const unsigned int xid, struct inode *inode,
char *pc_buf = NULL;
int flags = 0;
unsigned int total_len;
- __le16 *path = cifs_convert_path_to_utf16(full_path, cifs_sb);
-
- if (!path)
- return -ENOMEM;
+ __le16 *utf16_path = NULL;
cifs_dbg(FYI, "mkdir\n");
+ /* resource #1: path allocation */
+ utf16_path = cifs_convert_path_to_utf16(full_path, cifs_sb);
+ if (!utf16_path)
+ return -ENOMEM;
+
if (ses && (ses->server))
server = ses->server;
- else
- return -EIO;
+ else {
+ rc = -EIO;
+ goto err_free_path;
+ }
+ /* resource #2: request */
rc = smb2_plain_req_init(SMB2_CREATE, tcon, (void **) &req, &total_len);
-
if (rc)
- return rc;
+ goto err_free_path;
+
if (smb3_encryption_required(tcon))
flags |= CIFS_TRANSFORM_REQ;
-
req->ImpersonationLevel = IL_IMPERSONATION;
req->DesiredAccess = cpu_to_le32(FILE_WRITE_ATTRIBUTES);
/* File attributes ignored on open (used in create though) */
@@ -1983,50 +1987,44 @@ int smb311_posix_mkdir(const unsigned int xid, struct inode *inode,
req->sync_hdr.Flags |= SMB2_FLAGS_DFS_OPERATIONS;
rc = alloc_path_with_tree_prefix(©_path, ©_size,
&name_len,
- tcon->treeName, path);
- if (rc) {
- cifs_small_buf_release(req);
- return rc;
- }
+ tcon->treeName, utf16_path);
+ if (rc)
+ goto err_free_req;
+
req->NameLength = cpu_to_le16(name_len * 2);
uni_path_len = copy_size;
- path = copy_path;
+ /* free before overwriting resource */
+ kfree(utf16_path);
+ utf16_path = copy_path;
} else {
- uni_path_len = (2 * UniStrnlen((wchar_t *)path, PATH_MAX)) + 2;
+ uni_path_len = (2 * UniStrnlen((wchar_t *)utf16_path, PATH_MAX)) + 2;
/* MUST set path len (NameLength) to 0 opening root of share */
req->NameLength = cpu_to_le16(uni_path_len - 2);
if (uni_path_len % 8 != 0) {
copy_size = roundup(uni_path_len, 8);
copy_path = kzalloc(copy_size, GFP_KERNEL);
if (!copy_path) {
- cifs_small_buf_release(req);
- return -ENOMEM;
+ rc = -ENOMEM;
+ goto err_free_req;
}
- memcpy((char *)copy_path, (const char *)path,
+ memcpy((char *)copy_path, (const char *)utf16_path,
uni_path_len);
uni_path_len = copy_size;
- path = copy_path;
+ /* free before overwriting resource */
+ kfree(utf16_path);
+ utf16_path = copy_path;
}
}
iov[1].iov_len = uni_path_len;
- iov[1].iov_base = path;
+ iov[1].iov_base = utf16_path;
req->RequestedOplockLevel = SMB2_OPLOCK_LEVEL_NONE;
if (tcon->posix_extensions) {
- if (n_iov > 2) {
- struct create_context *ccontext =
- (struct create_context *)iov[n_iov-1].iov_base;
- ccontext->Next =
- cpu_to_le32(iov[n_iov-1].iov_len);
- }
-
+ /* resource #3: posix buf */
rc = add_posix_context(iov, &n_iov, mode);
- if (rc) {
- cifs_small_buf_release(req);
- kfree(copy_path);
- return rc;
- }
+ if (rc)
+ goto err_free_req;
pc_buf = iov[n_iov-1].iov_base;
}
@@ -2035,32 +2033,33 @@ int smb311_posix_mkdir(const unsigned int xid, struct inode *inode,
rqst.rq_iov = iov;
rqst.rq_nvec = n_iov;
- rc = cifs_send_recv(xid, ses, &rqst, &resp_buftype, flags,
- &rsp_iov);
-
- cifs_small_buf_release(req);
- rsp = (struct smb2_create_rsp *)rsp_iov.iov_base;
-
- if (rc != 0) {
+ /* resource #4: response buffer */
+ rc = cifs_send_recv(xid, ses, &rqst, &resp_buftype, flags, &rsp_iov);
+ if (rc) {
cifs_stats_fail_inc(tcon, SMB2_CREATE_HE);
trace_smb3_posix_mkdir_err(xid, tcon->tid, ses->Suid,
- CREATE_NOT_FILE, FILE_WRITE_ATTRIBUTES, rc);
- goto smb311_mkdir_exit;
- } else
- trace_smb3_posix_mkdir_done(xid, rsp->PersistentFileId, tcon->tid,
- ses->Suid, CREATE_NOT_FILE,
- FILE_WRITE_ATTRIBUTES);
+ CREATE_NOT_FILE,
+ FILE_WRITE_ATTRIBUTES, rc);
+ goto err_free_rsp_buf;
+ }
+
+ rsp = (struct smb2_create_rsp *)rsp_iov.iov_base;
+ trace_smb3_posix_mkdir_done(xid, rsp->PersistentFileId, tcon->tid,
+ ses->Suid, CREATE_NOT_FILE,
+ FILE_WRITE_ATTRIBUTES);
SMB2_close(xid, tcon, rsp->PersistentFileId, rsp->VolatileFileId);
/* Eventually save off posix specific response info and timestaps */
-smb311_mkdir_exit:
- kfree(copy_path);
- kfree(pc_buf);
+err_free_rsp_buf:
free_rsp_buf(resp_buftype, rsp);
+ kfree(pc_buf);
+err_free_req:
+ cifs_small_buf_release(req);
+err_free_path:
+ kfree(utf16_path);
return rc;
-
}
#endif /* SMB311 */
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread