[PATCH] Introduce dom_sid_str_buf

[Volker Lendecke]

On Thu, Nov 01, 2018 at 10:23:44AM -0700, Jeremy Allison wrote:
> For the second patch:
> 
> +       ret = dom_sid_string_buf(sid, dst->buf, sizeof(dst->buf));
> +       if ((ret < 0) > (ret >= sizeof(dst->buf))) {
> 
> the error check doesn't look right to me. Shouldn't it be:
> 
> +       if ((ret < 0) || (ret >= sizeof(dst->buf))) {

... how much can go wrong in a patch?... This is brown-paper-bag
quality :-)

Attached.

Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de
-------------- next part --------------
From 96e5899d5616079a8c353a1ba4fc384aba6fb0d1 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Thu, 1 Nov 2018 11:11:17 +0100
Subject: [PATCH 1/2] lib: Add error checks in dom_sid_string_buf

Also, avoid casts by using PRIxxx macros

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 libcli/security/dom_sid.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/libcli/security/dom_sid.c b/libcli/security/dom_sid.c
index b876fc8b780..f0d5afc21b0 100644
--- a/libcli/security/dom_sid.c
+++ b/libcli/security/dom_sid.c
@@ -431,7 +431,7 @@ bool dom_sid_is_valid_account_domain(const struct dom_sid *sid)
 */
 int dom_sid_string_buf(const struct dom_sid *sid, char *buf, int buflen)
 {
-	int i, ofs;
+	int i, ofs, ret;
 	uint64_t ia;
 
 	if (!sid) {
@@ -445,18 +445,32 @@ int dom_sid_string_buf(const struct dom_sid *sid, char *buf, int buflen)
 		((uint64_t)sid->id_auth[1] << 32) +
 		((uint64_t)sid->id_auth[0] << 40);
 
-	ofs = snprintf(buf, buflen, "S-%hhu-", (unsigned char)sid->sid_rev_num);
+	ret = snprintf(buf, buflen, "S-%"PRIu8"-", sid->sid_rev_num);
+	if (ret < 0) {
+		return ret;
+	}
+	ofs = ret;
+
 	if (ia >= UINT32_MAX) {
-		ofs += snprintf(buf + ofs, MAX(buflen - ofs, 0), "0x%llx",
-				(unsigned long long)ia);
+		ret = snprintf(buf+ofs, MAX(buflen-ofs, 0), "0x%"PRIx64, ia);
 	} else {
-		ofs += snprintf(buf + ofs, MAX(buflen - ofs, 0), "%llu",
-				(unsigned long long)ia);
+		ret = snprintf(buf+ofs, MAX(buflen-ofs, 0), "%"PRIu64, ia);
 	}
+	if (ret < 0) {
+		return ret;
+	}
+	ofs += ret;
 
 	for (i = 0; i < sid->num_auths; i++) {
-		ofs += snprintf(buf + ofs, MAX(buflen - ofs, 0), "-%u",
-				(unsigned int)sid->sub_auths[i]);
+		ret = snprintf(
+			buf+ofs,
+			MAX(buflen-ofs, 0),
+			"-%"PRIu32,
+			sid->sub_auths[i]);
+		if (ret < 0) {
+			return ret;
+		}
+		ofs += ret;
 	}
 	return ofs;
 }
-- 
2.11.0


From 2c244681d1184fea5eca90bda6e0a19823368bdc Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Thu, 18 Oct 2018 05:46:37 +0200
Subject: [PATCH 2/2] lib: Add dom_sid_str_buf

This is modeled after server_id_str_buf, which as an API to me is easier to
use: I can rely on the compiler to get the buffer size right.

It is designed to violate README.Coding's "Make use of helper variables", but
as this API is simple enough and the output should never be a surprise at all,
I think that's worth it.

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 libcli/security/dom_sid.c | 10 ++++++++++
 libcli/security/dom_sid.h |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/libcli/security/dom_sid.c b/libcli/security/dom_sid.c
index f0d5afc21b0..7a67d85e6e4 100644
--- a/libcli/security/dom_sid.c
+++ b/libcli/security/dom_sid.c
@@ -505,3 +505,13 @@ char *dom_sid_string(TALLOC_CTX *mem_ctx, const struct dom_sid *sid)
 	talloc_set_name_const(result, result);
 	return result;
 }
+
+char *dom_sid_str_buf(const struct dom_sid *sid, struct dom_sid_buf *dst)
+{
+	int ret;
+	ret = dom_sid_string_buf(sid, dst->buf, sizeof(dst->buf));
+	if ((ret < 0) || (ret >= sizeof(dst->buf))) {
+		strlcpy(dst->buf, "(INVALID SID)", sizeof(dst->buf));
+	}
+	return dst->buf;
+}
diff --git a/libcli/security/dom_sid.h b/libcli/security/dom_sid.h
index d9f4b3fc8a6..d132628da9f 100644
--- a/libcli/security/dom_sid.h
+++ b/libcli/security/dom_sid.h
@@ -102,6 +102,8 @@ bool dom_sid_is_valid_account_domain(const struct dom_sid *sid);
 int dom_sid_string_buf(const struct dom_sid *sid, char *buf, int buflen);
 char *dom_sid_string(TALLOC_CTX *mem_ctx, const struct dom_sid *sid);
 
+struct dom_sid_buf { char buf[DOM_SID_STR_BUFLEN]; };
+char *dom_sid_str_buf(const struct dom_sid *sid, struct dom_sid_buf *dst);
 
 const char *sid_type_lookup(uint32_t sid_type);
 const struct security_token *get_system_token(void);
-- 
2.11.0