[PATCH] Introduce dom_sid_str_buf

Volker Lendecke Volker.Lendecke at SerNet.DE
Thu Nov 1 10:29:24 UTC 2018


On Tue, Oct 30, 2018 at 05:06:36PM -0700, Jeremy Allison wrote:
> I really like this, but the error return checking on dom_sid_string_buf()
> isn't correct here (having said that, dom_sid_string_buf() is a
> *horribly* designed interface, it should take and return a size_t,
> not an int).

It was modeled after

http://pubs.opengroup.org/onlinepubs/9699919799/functions/fprintf.html

which has "int" as a return value for snprintf. This of course we
ignore(d) completely. Attached find two patches supposed to fix this.

Review appreciated!

Thanks, 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 3c2aa49fd3183a4f798077f23fddb1b2ae513329 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 54737e074873d404fb12064f8f5461d61fb549ca 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..6583652fd21 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



More information about the samba-technical mailing list