Empy or false string conversion (was :fix string to integer conversion)

swen swen at linux.ibm.com
Thu Apr 11 19:19:42 UTC 2019


Hi Christof

as suggested and agreed on by all involved parties off-list.
Please find attached the patch-set with the split patch as 
suggested by you.

Please review and push if, happy.

Cheers Swen
-------------- next part --------------
From 35ebf30506906509c6277f7fc129c14cb3bc9916 Mon Sep 17 00:00:00 2001
From: Swen Schillig <swen at linux.ibm.com>
Date: Wed, 6 Mar 2019 09:03:27 +0100
Subject: [PATCH 01/10] lib: modify string conversion wrapper to handle invalid
 strings

The standard string conversion routines convert a "non-number string"
to zero and do not flag an error.
This is changed now by returning EINVAL if no conversion occured.

Signed-off-by: Swen Schillig <swen at linux.ibm.com>
Reviewed-by: Ralph Boehme <slow at samba.org>
---
 lib/util/util.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/lib/util/util.c b/lib/util/util.c
index b8eed3ca28c..83af14cac1e 100644
--- a/lib/util/util.c
+++ b/lib/util/util.c
@@ -7,7 +7,8 @@
    Copyright (C) Jim McDonough (jmcd at us.ibm.com)  2003.
    Copyright (C) James J Myers 2003
    Copyright (C) Volker Lendecke 2010
-   
+   Copyright (C) Swen Schillig 2019
+
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
    the Free Software Foundation; either version 3 of the License, or
@@ -60,6 +61,7 @@
  * - wrong base
  * - value overflow
  * - string with a leading "-" indicating a negative number
+ * - no conversion due to empty string or not representing a number
  */
 unsigned long int
 strtoul_err(const char *nptr, char **endptr, int base, int *err)
@@ -84,6 +86,13 @@ strtoul_err(const char *nptr, char **endptr, int base, int *err)
 		return val;
 	}
 
+	/* got an invalid number-string resulting in no conversion */
+	if (nptr == tmp_endptr) {
+		*err = EINVAL;
+		errno = saved_errno;
+		return val;
+	}
+
 	/* did we convert a negative "number" ? */
 	needle = strchr(nptr, '-');
 	if (needle != NULL && needle < tmp_endptr) {
@@ -107,6 +116,7 @@ strtoul_err(const char *nptr, char **endptr, int base, int *err)
  * - wrong base
  * - value overflow
  * - string with a leading "-" indicating a negative number
+ * - no conversion due to empty string or not representing a number
  */
 unsigned long long int
 strtoull_err(const char *nptr, char **endptr, int base, int *err)
@@ -131,6 +141,13 @@ strtoull_err(const char *nptr, char **endptr, int base, int *err)
 		return val;
 	}
 
+	/* got an invalid number-string resulting in no conversion */
+	if (nptr == tmp_endptr) {
+		*err = EINVAL;
+		errno = saved_errno;
+		return val;
+	}
+
 	/* did we convert a negative "number" ? */
 	needle = strchr(nptr, '-');
 	if (needle != NULL && needle < tmp_endptr) {
-- 
2.20.1


From 92e71ec838c880fe4cd50e56ca46030f3a986f4c Mon Sep 17 00:00:00 2001
From: Swen Schillig <swen at linux.ibm.com>
Date: Wed, 6 Mar 2019 09:07:13 +0100
Subject: [PATCH 02/10] lib: Update error check for new string conversion
 wrapper

The new string conversion wrappers detect and flag errors
which occured during the string to integer conversion.
Those modifications required an update of the callees
error checks.

Signed-off-by: Swen Schillig <swen at linux.ibm.com>
Reviewed-by: Ralph Boehme <slow at samba.org>
---
 source3/lib/interface.c | 2 +-
 source3/lib/util_str.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/source3/lib/interface.c b/source3/lib/interface.c
index 342c92a61a2..31066b8b5cc 100644
--- a/source3/lib/interface.c
+++ b/source3/lib/interface.c
@@ -527,7 +527,7 @@ static void interpret_interface(char *token)
 		unsigned long val;
 
 		val = strtoul_err(p, &endp, 0, &error);
-		if (p == endp || (endp && *endp != '\0') || error != 0) {
+		if (error != 0 || *endp != '\0') {
 			DEBUG(2,("interpret_interface: "
 				"can't determine netmask value from %s\n",
 				p));
diff --git a/source3/lib/util_str.c b/source3/lib/util_str.c
index a0095d23978..1a27227fe41 100644
--- a/source3/lib/util_str.c
+++ b/source3/lib/util_str.c
@@ -859,7 +859,7 @@ uint64_t conv_str_size(const char * str)
 
 	lval = strtoull_err(str, &end, 10, &error);
 
-        if (end == NULL || end == str || error != 0) {
+        if (error != 0) {
                 return 0;
         }
 
-- 
2.20.1


From 2f84415036f0b6b949638b0096e877273c456cb3 Mon Sep 17 00:00:00 2001
From: Swen Schillig <swen at linux.ibm.com>
Date: Wed, 6 Mar 2019 09:29:13 +0100
Subject: [PATCH 03/10] utils: Update error check for new string conversion
 wrapper

The new string conversion wrappers detect and flag errors
which occured during the string to integer conversion.
Those modifications required an update of the callees
error checks.

Signed-off-by: Swen Schillig <swen at linux.ibm.com>
Reviewed-by: Ralph Boehme <slow at samba.org>
---
 source3/utils/net_idmap.c      | 10 ++--------
 source3/utils/net_sam.c        |  2 +-
 source3/utils/pdbedit.c        |  5 +----
 source3/utils/regedit_dialog.c |  8 +++-----
 4 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/source3/utils/net_idmap.c b/source3/utils/net_idmap.c
index f6c9f3a1a3d..d1a6db95921 100644
--- a/source3/utils/net_idmap.c
+++ b/source3/utils/net_idmap.c
@@ -631,16 +631,10 @@ static bool parse_uint32(const char *str, uint32_t *result)
 	int error = 0;
 
 	val = strtoul_err(str, &endptr, 10, &error);
-
-	if (str == endptr) {
-		return false;
-	}
-	if (*endptr != '\0') {
-		return false;
-	}
-	if (error != 0) {
+	if (error != 0 || *endptr != '\0') {
 		return false;
 	}
+
 	*result = val;		/* Potential crop */
 	return true;
 }
diff --git a/source3/utils/net_sam.c b/source3/utils/net_sam.c
index fc2a7baacef..164d9408c56 100644
--- a/source3/utils/net_sam.c
+++ b/source3/utils/net_sam.c
@@ -503,7 +503,7 @@ static int net_sam_policy_set(struct net_context *c, int argc, const char **argv
 	else {
 		value = strtoul_err(argv[1], &endptr, 10, &err);
 
-		if ((endptr == argv[1]) || (endptr[0] != '\0') || (err != 0)) {
+		if (err != 0 || *endptr != '\0') {
 			d_printf(_("Unable to set policy \"%s\"! Invalid value "
 				 "\"%s\".\n"),
 				 account_policy, argv[1]);
diff --git a/source3/utils/pdbedit.c b/source3/utils/pdbedit.c
index c80d5411b00..462c753217e 100644
--- a/source3/utils/pdbedit.c
+++ b/source3/utils/pdbedit.c
@@ -602,10 +602,7 @@ static int set_user_info(const char *username, const char *fullname,
 			uint32_t num;
 
 			num = strtoul_err(kickoff_time, &endptr, 10, &error);
-			if ((endptr == kickoff_time) ||
-			    (endptr[0] != '\0') ||
-			    (error != 0))
-			{
+			if (error != 0 || *endptr != '\0') {
 				fprintf(stderr, "Failed to parse kickoff time\n");
 				return -1;
 			}
diff --git a/source3/utils/regedit_dialog.c b/source3/utils/regedit_dialog.c
index aeea70ac22e..7dd9f0fadea 100644
--- a/source3/utils/regedit_dialog.c
+++ b/source3/utils/regedit_dialog.c
@@ -1029,7 +1029,6 @@ bool dialog_section_text_field_get_int(struct dialog_section *section,
 bool dialog_section_text_field_get_uint(struct dialog_section *section,
 				        unsigned long long *out)
 {
-	bool rv;
 	const char *buf;
 	char *endp;
 	int error = 0;
@@ -1043,12 +1042,11 @@ bool dialog_section_text_field_get_uint(struct dialog_section *section,
 		return false;
 	}
 	*out = strtoull_err(buf, &endp, 0, &error);
-	rv = true;
-	if (endp == buf || endp == NULL || endp[0] != '\0' || error != 0) {
-		rv = false;
+	if (error != 0 || *endp != '\0') {
+		return false;
 	}
 
-	return rv;
+	return true;
 }
 
 /* hex editor field */
-- 
2.20.1


From 42ef1d74ac18d87e279e1ede4d368511b48c16f1 Mon Sep 17 00:00:00 2001
From: Swen Schillig <swen at linux.ibm.com>
Date: Wed, 6 Mar 2019 09:34:10 +0100
Subject: [PATCH 04/10] modules: Update error check for new string conversion
 wrapper

The new string conversion wrappers detect and flag errors
which occured during the string to integer conversion.
Those modifications required an update of the callees
error checks.

Signed-off-by: Swen Schillig <swen at linux.ibm.com>
Reviewed-by: Ralph Boehme <slow at samba.org>
---
 source3/modules/vfs_snapper.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/source3/modules/vfs_snapper.c b/source3/modules/vfs_snapper.c
index fac4aa698de..39b8ccb097a 100644
--- a/source3/modules/vfs_snapper.c
+++ b/source3/modules/vfs_snapper.c
@@ -1216,7 +1216,6 @@ static NTSTATUS snapper_snap_path_to_id(TALLOC_CTX *mem_ctx,
 {
 	char *path_dup;
 	char *str_idx;
-	char *str_end;
 	uint32_t snap_id;
 	int error = 0;
 
@@ -1251,8 +1250,8 @@ static NTSTATUS snapper_snap_path_to_id(TALLOC_CTX *mem_ctx,
 	}
 
 	str_idx++;
-	snap_id = strtoul_err(str_idx, &str_end, 10, &error);
-	if (error != 0 || str_idx == str_end) {
+	snap_id = strtoul_err(str_idx, NULL, 10, &error);
+	if (error != 0) {
 		talloc_free(path_dup);
 		return NT_STATUS_INVALID_PARAMETER;
 	}
-- 
2.20.1


From 68646bbcdc9c59ee59b45e50aecadc81cca50abf Mon Sep 17 00:00:00 2001
From: Swen Schillig <swen at linux.ibm.com>
Date: Wed, 6 Mar 2019 09:43:53 +0100
Subject: [PATCH 05/10] ctdb-protocol: Update error check for new string
 conversion wrapper

The new string conversion wrappers detect and flag errors
which occured during the string to integer conversion.
Those modifications required an update of the callees
error checks.

Signed-off-by: Swen Schillig <swen at linux.ibm.com>
Reviewed-by: Ralph Boehme <slow at samba.org>
---
 ctdb/protocol/protocol_util.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ctdb/protocol/protocol_util.c b/ctdb/protocol/protocol_util.c
index f97c64665df..d120213b70a 100644
--- a/ctdb/protocol/protocol_util.c
+++ b/ctdb/protocol/protocol_util.c
@@ -291,7 +291,7 @@ int ctdb_sock_addr_from_string(const char *str,
 	}
 
 	port = strtoul_err(p+1, &endp, 10, &ret);
-	if (endp == p+1 || *endp != '\0' || ret != 0) {
+	if (ret != 0 || *endp != '\0') {
 		/* Empty string or trailing garbage */
 		return EINVAL;
 	}
@@ -330,7 +330,7 @@ int ctdb_sock_addr_mask_from_string(const char *str,
 	}
 
 	m = strtoul_err(p+1, &endp, 10, &ret);
-	if (endp == p+1 || *endp != '\0' || ret != 0) {
+	if (ret != 0 || *endp != '\0') {
 		/* Empty string or trailing garbage */
 		return EINVAL;
 	}
-- 
2.20.1


From 4d966e2a99831a8afb591d603726731b1d00c499 Mon Sep 17 00:00:00 2001
From: Swen Schillig <swen at linux.ibm.com>
Date: Wed, 6 Mar 2019 09:48:24 +0100
Subject: [PATCH 06/10] ctdb-tools: Update error check for new string
 conversion wrapper

The new string conversion wrappers detect and flag errors
which occured during the string to integer conversion.
Those modifications required an update of the callees
error checks.

Signed-off-by: Swen Schillig <swen at linux.ibm.com>
Reviewed-by: Ralph Boehme <slow at samba.org>
---
 ctdb/tools/ctdb.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/ctdb/tools/ctdb.c b/ctdb/tools/ctdb.c
index 6b82bf779a9..8db0ec8e766 100644
--- a/ctdb/tools/ctdb.c
+++ b/ctdb/tools/ctdb.c
@@ -325,10 +325,9 @@ static bool parse_nodestring(TALLOC_CTX *mem_ctx, struct ctdb_context *ctdb,
 		tok = strtok(ns, ",");
 		while (tok != NULL) {
 			uint32_t pnn;
-			char *endptr;
 
-			pnn = (uint32_t)strtoul_err(tok, &endptr, 0, &error);
-			if (error != 0 || (pnn == 0 && tok == endptr)) {
+			pnn = (uint32_t)strtoul_err(tok, NULL, 0, &error);
+			if (error != 0) {
 				fprintf(stderr, "Invalid node %s\n", tok);
 					return false;
 			}
-- 
2.20.1


From ea9b689ee3bb1b387970b5b3e0c1e662dd6fac0b Mon Sep 17 00:00:00 2001
From: Swen Schillig <swen at linux.ibm.com>
Date: Wed, 6 Mar 2019 10:02:53 +0100
Subject: [PATCH 07/10] common-lib: Update error check for new string
 conversion wrapper

The new string conversion wrappers detect and flag errors
which occured during the string to integer conversion.
Those modifications required an update of the callees
error checks.

Signed-off-by: Swen Schillig <swen at linux.ibm.com>
Reviewed-by: Ralph Boehme <slow at samba.org>
---
 lib/ldb-samba/ldb_matching_rules.c | 14 ++------------
 lib/util/access.c                  |  2 +-
 lib/util/util_str.c                |  4 ++--
 3 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/lib/ldb-samba/ldb_matching_rules.c b/lib/ldb-samba/ldb_matching_rules.c
index 7387c12f10d..0754e7e066b 100644
--- a/lib/ldb-samba/ldb_matching_rules.c
+++ b/lib/ldb-samba/ldb_matching_rules.c
@@ -393,12 +393,7 @@ static int dsdb_match_for_dns_to_tombstone_time(struct ldb_context *ldb,
 			return LDB_ERR_INVALID_ATTRIBUTE_SYNTAX;
 		}
 		tombstone_time = strtoull_err(s, &p, 10, &error);
-		if (p == NULL ||
-		    p == s ||
-		    *p != '\0' ||
-		    error != 0 ||
-		    tombstone_time == ULLONG_MAX)
-		{
+		if (error != 0 || *p != '\0') {
 			DBG_ERR("Invalid timestamp string passed\n");
 			return LDB_ERR_INVALID_ATTRIBUTE_SYNTAX;
 		}
@@ -529,12 +524,7 @@ static int dsdb_match_for_expunge(struct ldb_context *ldb,
 			return LDB_ERR_INVALID_ATTRIBUTE_SYNTAX;
 		}
 		tombstone_time = strtoull_err(s, &p, 10, &error);
-		if (p == NULL ||
-		    p == s ||
-		    *p != '\0' ||
-		    error != 0 ||
-		    tombstone_time == ULLONG_MAX)
-		{
+		if (error != 0 || *p != '\0') {
 			return LDB_ERR_INVALID_ATTRIBUTE_SYNTAX;
 		}
 	}
diff --git a/lib/util/access.c b/lib/util/access.c
index a05a47c15b2..960fe4b066c 100644
--- a/lib/util/access.c
+++ b/lib/util/access.c
@@ -75,7 +75,7 @@ static bool masked_match(const char *tok, const char *slash, const char *s)
 		unsigned long val;
 
 		val = strtoul_err(slash+1, &endp, 0, &error);
-		if (slash+1 == endp || (endp && *endp != '\0') || error != 0) {
+		if (error != 0 || *endp != '\0') {
 			return false;
 		}
 		if (!make_netmask(&ss_mask, &ss_tok, val)) {
diff --git a/lib/util/util_str.c b/lib/util/util_str.c
index 447919b087b..29a44836bde 100644
--- a/lib/util/util_str.c
+++ b/lib/util/util_str.c
@@ -70,7 +70,7 @@ _PUBLIC_ bool conv_str_size_error(const char * str, uint64_t * val)
 	}
 
 	lval = strtoull_err(str, &end, 10, &error);
-	if (end == NULL || end == str || error != 0) {
+	if (error != 0) {
 		return false;
 	}
 
@@ -112,7 +112,7 @@ _PUBLIC_ bool conv_str_u64(const char * str, uint64_t * val)
 	}
 
 	lval = strtoull_err(str, &end, 10, &error);
-	if (end == NULL || *end != '\0' || end == str || error != 0) {
+	if (error != 0 || *end != '\0') {
 		return false;
 	}
 
-- 
2.20.1


From a61a07b0dc238dd82dc4b3376fc1490cf1cfb604 Mon Sep 17 00:00:00 2001
From: Swen Schillig <swen at linux.ibm.com>
Date: Wed, 6 Mar 2019 10:06:35 +0100
Subject: [PATCH 08/10] libcli: Update error check for new string conversion
 wrapper

The new string conversion wrappers detect and flag errors
which occured during the string to integer conversion.
Those modifications required an update of the callees
error checks.

Signed-off-by: Swen Schillig <swen at linux.ibm.com>
Reviewed-by: Ralph Boehme <slow at samba.org>
---
 libcli/security/dom_sid.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libcli/security/dom_sid.c b/libcli/security/dom_sid.c
index ca7fd874752..ac34a92f19c 100644
--- a/libcli/security/dom_sid.c
+++ b/libcli/security/dom_sid.c
@@ -149,7 +149,7 @@ bool dom_sid_parse_endp(const char *sidstr,struct dom_sid *sidout,
 	}
 
 	conv = strtoul_err(p, &q, 10, &error);
-	if (!q || (*q != '-') || conv > UINT8_MAX || error != 0) {
+	if (error != 0 || (*q != '-') || conv > UINT8_MAX) {
 		goto format_error;
 	}
 	sidout->sid_rev_num = (uint8_t) conv;
@@ -161,7 +161,7 @@ bool dom_sid_parse_endp(const char *sidstr,struct dom_sid *sidout,
 
 	/* get identauth */
 	conv = strtoull_err(q, &q, 0, &error);
-	if (!q || conv & AUTHORITY_MASK || error != 0) {
+	if (conv & AUTHORITY_MASK || error != 0) {
 		goto format_error;
 	}
 
@@ -190,7 +190,7 @@ bool dom_sid_parse_endp(const char *sidstr,struct dom_sid *sidout,
 		}
 
 		conv = strtoull_err(q, &end, 10, &error);
-		if (end == q || conv > UINT32_MAX || error != 0) {
+		if (conv > UINT32_MAX || error != 0) {
 			goto format_error;
 		}
 
-- 
2.20.1


From 2d37c52afc89071caacb15ce424bc33ba9f974b1 Mon Sep 17 00:00:00 2001
From: Swen Schillig <swen at linux.ibm.com>
Date: Wed, 6 Mar 2019 10:11:39 +0100
Subject: [PATCH 09/10] source4: Update error check for new string conversion
 wrapper

The new string conversion wrappers detect and flag errors
which occured during the string to integer conversion.
Those modifications required an update of the callees
error checks.

Signed-off-by: Swen Schillig <swen at linux.ibm.com>
Reviewed-by: Ralph Boehme <slow at samba.org>
---
 source4/dsdb/samdb/ldb_modules/samldb.c | 2 +-
 source4/lib/socket/interface.c          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/source4/dsdb/samdb/ldb_modules/samldb.c b/source4/dsdb/samdb/ldb_modules/samldb.c
index a8ea6c88459..4c773da5b19 100644
--- a/source4/dsdb/samdb/ldb_modules/samldb.c
+++ b/source4/dsdb/samdb/ldb_modules/samldb.c
@@ -3337,7 +3337,7 @@ static int verify_cidr(const char *cidr)
 		goto error;
 	}
 
-	if (*endptr != '\0' || endptr == slash + 1 || error != 0){
+	if (error != 0 || *endptr != '\0'){
 		DBG_INFO("CIDR mask is not a proper integer: %s\n", cidr);
 		goto error;
 	}
diff --git a/source4/lib/socket/interface.c b/source4/lib/socket/interface.c
index 494cbd4fdd2..9bf002c2f01 100644
--- a/source4/lib/socket/interface.c
+++ b/source4/lib/socket/interface.c
@@ -228,7 +228,7 @@ static void interpret_interface(TALLOC_CTX *mem_ctx,
 		int error = 0;
 
 		unsigned long val = strtoul_err(p, &endp, 0, &error);
-		if (p == endp || (endp && *endp != '\0') || error != 0) {
+		if (error != 0 || *endp != '\0') {
 			DEBUG(2,("interpret_interface: "
 				"can't determine netmask value from %s\n",
 				p));
-- 
2.20.1


From 078f9a657de4179fdc10d76374b3db113c060672 Mon Sep 17 00:00:00 2001
From: Swen Schillig <swen at linux.ibm.com>
Date: Thu, 11 Apr 2019 09:52:05 +0200
Subject: [PATCH 10/10] lib: remove duplicate check

This check was supposed to be removed by c9f4b92a613.

Signed-off-by: Swen Schillig <swen at linux.ibm.com>
Reviewed-by: Ralph Boehme <slow at samba.org>
---
 source3/lib/messages_dgm.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/source3/lib/messages_dgm.c b/source3/lib/messages_dgm.c
index d73a6ad6a7c..5a79a234271 100644
--- a/source3/lib/messages_dgm.c
+++ b/source3/lib/messages_dgm.c
@@ -1470,10 +1470,6 @@ static int messaging_dgm_read_unique(int fd, uint64_t *punique)
 	buf[rw_ret] = '\0';
 
 	unique = strtoull_err(buf, &endptr, 10, &error);
-	if ((unique == 0) && (errno == EINVAL)) {
-		return EINVAL;
-	}
-
 	if (error != 0) {
 		return error;
 	}
-- 
2.20.1

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20190411/6699b16d/signature.sig>


More information about the samba-technical mailing list