[PATCH] Introduce dom_sid_str_buf

[Jeremy Allison]

On Sat, Oct 27, 2018 at 10:08:44PM +0200, Volker Lendecke via samba-technical wrote:
> Hi!
> 
> Attached find a patch that makes our sid2string conversion a little
> safer than before. Eventually I'd like to move all of our
> sid_string_tos and dom_sid_string routines into dom_sid_str_buf
> wherever the use pattern allows it. That will be a lot of small
> patches that can easily be squashed if it spoils commit statistics too
> much.
> 
> 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

> From 2923b54ce04c2e43d3fed256c0fdfbc68468ea1e 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 01/19] 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 b876fc8b780..eac648e2f60 100644
> --- a/libcli/security/dom_sid.c
> +++ b/libcli/security/dom_sid.c
> @@ -491,3 +491,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) {
> +		strlcpy(dst->buf, "(INVALID SID)", sizeof(dst->buf));
> +	}
> +	return dst->buf;
> +}

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

The error condition for dom_sid_string_buf() should be:

+       ret = dom_sid_string_buf(sid, dst->buf, sizeof(dst->buf));
+       if (ret >= sizeof(dst->buf)) {
+               strlcpy(dst->buf, "(INVALID SID)", sizeof(dst->buf));
+       }

not 'if (ret < 0) {'

Can I change your code to use that error condition instead in patch
[PATCH 01/19] lib: Add dom_sid_str_buf, then review the rest as-is ?

Cheers,

	Jeremy.