[PATCH] Fixes for undefined behavior in util_tdb

[Andreas Schneider]

Hi,

see attached patchset. A CI pipeline is here:

https://gitlab.com/samba-team/devel/samba/pipelines/38025491


Please review and comment. Push if OK.


Thanks,


	Andreas

-- 
Andreas Schneider                      asn at samba.org
Samba Team                             www.samba.org
GPG-ID:     8DFF53E18F2ABC8D8F3C92237EE0FC4DCC014E3D
-------------- next part --------------
From 0c063bc6d668a71cab4039d7edc511012404a6a8 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Fri, 23 Nov 2018 12:00:36 +0100
Subject: [PATCH 1/3] s3:lib: Fix uninitialized variable
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

util_tdb.c:116:7: error: ‘len’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
   buf += len;
       ^~
../../source3/lib/util_tdb.c:44:6: note: ‘len’ was declared here
  int len;
      ^~~

Signed-off-by: Andreas Schneider <asn at samba.org>
---
 source3/lib/util_tdb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/source3/lib/util_tdb.c b/source3/lib/util_tdb.c
index cbcca4df09f..4f2450c5773 100644
--- a/source3/lib/util_tdb.c
+++ b/source3/lib/util_tdb.c
@@ -41,7 +41,7 @@ static size_t tdb_pack_va(uint8_t *buf, int bufsize, const char *fmt, va_list ap
 	uint32_t d;
 	int i;
 	void *p;
-	int len;
+	int len = 0;
 	char *s;
 	char c;
 	uint8_t *buf0 = buf;
-- 
2.19.1


From 7c7cadda8ab039df4c5eedcf01dea445f737e16b Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Thu, 22 Nov 2018 13:33:11 +0100
Subject: [PATCH 2/3] s3:lib: Fix undefined behavior in tdb_pack()

util_tdb.c:98:5: runtime error: null pointer passed as argument 2, which
is declared to never be null

This means the second argument of memcpy() can't be NULL.

Signed-off-by: Andreas Schneider <asn at samba.org>
---
 source3/lib/util_tdb.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/source3/lib/util_tdb.c b/source3/lib/util_tdb.c
index 4f2450c5773..182b27e6376 100644
--- a/source3/lib/util_tdb.c
+++ b/source3/lib/util_tdb.c
@@ -76,14 +76,12 @@ static size_t tdb_pack_va(uint8_t *buf, int bufsize, const char *fmt, va_list ap
 				SIVAL(buf, 0, d);
 			break;
 		case 'P': /* null-terminated string */
-			s = va_arg(ap,char *);
-			w = strlen(s);
-			len = w + 1;
-			if (bufsize && bufsize >= len)
-				memcpy(buf, s, len);
-			break;
 		case 'f': /* null-terminated string */
 			s = va_arg(ap,char *);
+			if (s == NULL) {
+				len = 1;
+				break;
+			}
 			w = strlen(s);
 			len = w + 1;
 			if (bufsize && bufsize >= len)
@@ -95,7 +93,9 @@ static size_t tdb_pack_va(uint8_t *buf, int bufsize, const char *fmt, va_list ap
 			len = 4+i;
 			if (bufsize && bufsize >= len) {
 				SIVAL(buf, 0, i);
-				memcpy(buf+4, s, i);
+				if (s != NULL) {
+					memcpy(buf+4, s, i);
+				}
 			}
 			break;
 		default:
-- 
2.19.1


From 10d3fec4b973082c8329c4baac40cd40ac1a9b85 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Tue, 27 Nov 2018 08:23:25 +0100
Subject: [PATCH 3/3] s3:lib: Fix undefined behavior in tdb_unpack()

Signed-off-by: Andreas Schneider <asn at samba.org>
---
 source3/lib/util_tdb.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/source3/lib/util_tdb.c b/source3/lib/util_tdb.c
index 182b27e6376..4ba014a6923 100644
--- a/source3/lib/util_tdb.c
+++ b/source3/lib/util_tdb.c
@@ -192,9 +192,11 @@ int tdb_unpack(const uint8_t *buf, int in_bufsize, const char *fmt, ...)
 			len = strnlen((const char *)buf, bufsize) + 1;
 			if (bufsize < len)
 				goto no_space;
-			*ps = SMB_STRDUP((const char *)buf);
-			if (*ps == NULL) {
-				goto no_space;
+			if (ps != NULL) {
+				*ps = SMB_STRDUP((const char *)buf);
+				if (*ps == NULL) {
+					goto no_space;
+				}
 			}
 			break;
 		case 'f': /* null-terminated string */
@@ -202,7 +204,9 @@ int tdb_unpack(const uint8_t *buf, int in_bufsize, const char *fmt, ...)
 			len = strnlen((const char *)buf, bufsize) + 1;
 			if (bufsize < len || len > sizeof(fstring))
 				goto no_space;
-			memcpy(s, buf, len);
+			if (s != NULL) {
+				memcpy(s, buf, len);
+			}
 			break;
 		case 'B': /* fixed-length string */
 			i = va_arg(ap, uint32_t *);
@@ -221,10 +225,12 @@ int tdb_unpack(const uint8_t *buf, int in_bufsize, const char *fmt, ...)
 			}
 			if (bufsize < len)
 				goto no_space;
-			*b = (char *)SMB_MALLOC(*i);
-			if (! *b)
-				goto no_space;
-			memcpy(*b, buf+4, *i);
+			if (b != NULL) {
+				*b = (char *)SMB_MALLOC(*i);
+				if (! *b)
+					goto no_space;
+				memcpy(*b, buf+4, *i);
+			}
 			break;
 		default:
 			DEBUG(0,("Unknown tdb_unpack format %c in %s\n",
-- 
2.19.1