Skip to contentSkip to conversation

CIFS: fix structurally dead code in smb311_posix_mkdir

PATCH In review Gustavo A. R. Silva 2 participants 3 messages Jun 21, 2018

Subsystems (2)
SMB client (CIFS) Kernel SMB3 server (ksmbd)
lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

PATCH 1/1 CIFS: fix structurally dead code in smb311_posix_mkdir

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 <[email protected]>
---
 fs/cifs/smb2pdu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/cifs/smb2pdu.c +1 -1
index 062a2a5..c9489b1 100644
@@ -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 */
Signature · 1 line
2.7.4
Reviewed-by Paulo Alcantara on v1 1/1
#1 lore raw

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.

Quoted 34 lines · Gustavo A. R. Silva

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 <[email protected]>
---
 fs/cifs/smb2pdu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/cifs/smb2pdu.c +1 -2
index 062a2a5..c9489b1 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 3 lines
Thanks,

Steve

From f877386d373753c83d791d82328356cd98812eb8 Mon Sep 17 00:00:00 2001
From: Aurelien Aptel <[email protected]>
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 <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>
Reviewed-by Paulo Alcantara <[email protected]>
Signed-off-by Steve French <[email protected]>
---
 fs/cifs/smb2pdu.c | 99 +++++++++++++++++++++++------------------------
 1 file changed, 49 insertions(+), 50 deletions(-)
fs/cifs/smb2pdu.c +49 -50
index 810b85787c91..ea102dca5f33 100644
@@ -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(&copy_path, &copy_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 */
Signature · 1 line
2.17.1
author
#2 lore raw
Quoted 4 lines · Steve French

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.

Fine. You can add my Signed-off-by

Thanks
--
Gustavo