[PATCH] Introduce dom_sid_str_buf

[Jeremy Allison]

On Fri, Nov 02, 2018 at 11:47:16AM +0100, Volker Lendecke wrote:
> 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.

LGTM :-). Pushed (with the additional squash). Thanks !

Jeremy.

> -- 
> 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

> 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
>