[PATCH] Fixes for issues found by covscan or Coverity
Andreas Schneider
asn at samba.org
Tue Feb 12 15:47:14 UTC 2019
Hi,
attached is a patchset which addresses some issues either found by our
internal covscan or the Coverity scan service (the latter has a CID).
Review is much appreciated, push if OK please.
Thanks,
Andreas
--
Andreas Schneider asn at samba.org
Samba Team www.samba.org
GPG-ID: 8DFF53E18F2ABC8D8F3C92237EE0FC4DCC014E3D
-------------- next part --------------
>From abb7afff2838fcee6e191f0390d146671692bf6b Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Mon, 4 Feb 2019 16:10:50 +0100
Subject: [PATCH 1/5] s3:smbd: Add additional size check to dup_smb2_req()
Covscan reported:
3. samba-4.9.1/source3/smbd/smb2_server.c:1242: assignment: Assigning: "i" = "1".
5. samba-4.9.1/source3/smbd/smb2_server.c:1243: overrun-buffer-val:
Overrunning struct type iovec of 16 bytes by passing it to a
function which accesses it at byte offset 48.
1241| /* Setup the vectors identically to the ones in req. */
1242| for (i = 1; i < count; i += SMBD_SMB2_NUM_IOV_PER_REQ) {
1243|-> if (!dup_smb2_vec4(outvec, &outvec[i], &req->out.vector[i])) {
1244| break;
1245| }
Signed-off-by: Andreas Schneider <asn at samba.org>
---
source3/smbd/smb2_server.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
index 1e9ed331aa6..a4dacfcdae7 100644
--- a/source3/smbd/smb2_server.c
+++ b/source3/smbd/smb2_server.c
@@ -1240,7 +1240,9 @@ static struct smbd_smb2_request *dup_smb2_req(const struct smbd_smb2_request *re
memcpy(newreq->out.nbt_hdr, req->out.nbt_hdr, 4);
/* Setup the vectors identically to the ones in req. */
- for (i = 1; i < count; i += SMBD_SMB2_NUM_IOV_PER_REQ) {
+ for (i = 1;
+ i < count && i + SMBD_SMB2_NUM_IOV_PER_REQ <= count;
+ i += SMBD_SMB2_NUM_IOV_PER_REQ) {
if (!dup_smb2_vec4(outvec, &outvec[i], &req->out.vector[i])) {
break;
}
--
2.20.1
>From e7f891fa97a43f09f6b707253f2aed8db4d12973 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Mon, 4 Feb 2019 16:17:30 +0100
Subject: [PATCH 2/5] s3:locking: Add additional check for sizes
42. samba-4.9.1/source3/locking/brlock.c:875: overrun-buffer-val:
Overrunning struct type lock_struct of 72 bytes by passing it to a
function which accesses it at byte offset 72.
873|
874| /* Work out overlaps. */
875|-> tmp_count += brlock_posix_split_merge(&tp[count], curr_lock, plock);
876| posix_count += tmp_count;
877| count += tmp_count;
36. samba-4.9.1/source3/locking/brlock.c:1245: overrun-buffer-val:
Overrunning struct type lock_struct of 72 bytes by passing it to a
function which accesses it at byte offset 72.
1243|
1244| /* Work out overlaps. */
1245|-> tmp_count = brlock_posix_split_merge(&tp[count],
lock, plock);
1246|
1247| if (tmp_count == 0) {
Signed-off-by: Andreas Schneider <asn at samba.org>
---
source3/locking/brlock.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/source3/locking/brlock.c b/source3/locking/brlock.c
index 0c91d55e813..2e06aeac2be 100644
--- a/source3/locking/brlock.c
+++ b/source3/locking/brlock.c
@@ -826,7 +826,9 @@ static NTSTATUS brl_lock_posix(struct messaging_context *msg_ctx,
count = posix_count = 0;
- for (i=0; i < br_lck->num_locks; i++) {
+ for (i = 0;
+ i < br_lck->num_locks && count < br_lck->num_locks;
+ i++) {
struct lock_struct *curr_lock = &locks[i];
/* If we have a pending read lock, a lock downgrade should
@@ -1217,7 +1219,9 @@ static bool brl_unlock_posix(struct messaging_context *msg_ctx,
}
count = 0;
- for (i = 0; i < br_lck->num_locks; i++) {
+ for (i = 0;
+ i < br_lck->num_locks && count < br_lck->num_locks;
+ i++) {
struct lock_struct *lock = &locks[i];
unsigned int tmp_count;
--
2.20.1
>From dc9e90c3ded3a8f7ca34da6eb03510a9be63d658 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Mon, 4 Feb 2019 17:19:55 +0100
Subject: [PATCH 3/5] s3:locking: Add missing NULL check
Signed-off-by: Andreas Schneider <asn at samba.org>
---
source3/locking/locking.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/source3/locking/locking.c b/source3/locking/locking.c
index ae5f0bbcf33..d4c3b32be7f 100644
--- a/source3/locking/locking.c
+++ b/source3/locking/locking.c
@@ -150,6 +150,9 @@ bool strict_lock_check_default(files_struct *fsp, struct lock_struct *plock)
* autocleanup. This is the slow path anyway.
*/
br_lck = brl_get_locks(talloc_tos(), fsp);
+ if (br_lck == NULL) {
+ return true;
+ }
ret = brl_locktest(br_lck, plock);
TALLOC_FREE(br_lck);
}
--
2.20.1
>From 85784495122335097371ec0098d1679322954f99 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Mon, 4 Feb 2019 17:23:05 +0100
Subject: [PATCH 4/5] s3:utils: Add missing NULL check in
rpc_fetch_domain_aliases()
Signed-off-by: Andreas Schneider <asn at samba.org>
---
source3/utils/net_rpc.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/source3/utils/net_rpc.c b/source3/utils/net_rpc.c
index f2d63d2af65..a56190f7be5 100644
--- a/source3/utils/net_rpc.c
+++ b/source3/utils/net_rpc.c
@@ -4669,6 +4669,10 @@ static NTSTATUS rpc_fetch_domain_aliases(struct rpc_pipe_client *pipe_hnd,
if (alias.num_members > 0) {
alias.members = SMB_MALLOC_ARRAY(struct dom_sid, alias.num_members);
+ if (alias.members == NULL) {
+ status = NT_STATUS_NO_MEMORY;
+ goto done;
+ }
for (j = 0; j < alias.num_members; j++)
sid_copy(&alias.members[j],
--
2.20.1
>From bc317f9985ade3c45fad253f9dc3f21ad3cd9937 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Mon, 4 Feb 2019 16:44:02 +0100
Subject: [PATCH 5/5] lib:replace: Avoid out of bound reads warnings
1. Condition *((char const *)string) == '.', taking true branch.
2. Condition (char const *)string[1] == '.', taking true branch.
3. Condition (char const *)string[2] == 0, taking true branch.
1655 if (ISDOTDOT(string))
4. alias: Assigning: string = ".". string now points to byte 0 of "." (which consists of 2 bytes).
1656 string = ".";
1. Condition *((char const *)string) == '.', taking true branch.
2. Condition (char const *)string[1] == '.', taking true branch.
3. index_const: Pointer string directly indexed by constant 2.
160 if (ISDOTDOT(string)) {
161 string = ".";
162 }
I guess it wants a check for '\0' at string[1] and leave.
CID 710714
Signed-off-by: Andreas Schneider <asn at samba.org>
---
lib/replace/system/dir.h | 38 +++++++++++++++++++++++++++-----------
1 file changed, 27 insertions(+), 11 deletions(-)
diff --git a/lib/replace/system/dir.h b/lib/replace/system/dir.h
index 497b5a7c0fa..e1ad8382854 100644
--- a/lib/replace/system/dir.h
+++ b/lib/replace/system/dir.h
@@ -50,22 +50,38 @@
# include <libgen.h>
#endif
-/* Test whether a file name is the "." or ".." directory entries.
- * These really should be inline functions.
+/*
+ * Test whether a file name is the "." or ".." directory entries.
*/
+static inline bool is_dot(const char *path)
+{
+ if (unlikely(path == NULL)) {
+ return false;
+ }
+
+ if (unlikely(path[0] == '\0')) {
+ return false;
+ }
+
+ return path[0] == '.' && path[1] == '\0';
+}
+
+static inline bool is_dotdot(const char *path)
+{
+ /* The is_dot() function will to NULL checks */
+ if (is_dot(path)) {
+ return false;
+ }
+
+ return path[1] == '.' && path[2] == '\0';
+}
+
#ifndef ISDOT
-#define ISDOT(path) ( \
- *((const char *)(path)) == '.' && \
- *(((const char *)(path)) + 1) == '\0' \
- )
+#define ISDOT(path) is_dot(path)
#endif
#ifndef ISDOTDOT
-#define ISDOTDOT(path) ( \
- *((const char *)(path)) == '.' && \
- *(((const char *)(path)) + 1) == '.' && \
- *(((const char *)(path)) + 2) == '\0' \
- )
+#define ISDOTDOT(path) is_dotdot(path)
#endif
#endif
--
2.20.1
More information about the samba-technical
mailing list