dom_sid_parse_length (was: Re: [PATCH] Cleanup patches)

[Jeremy Allison]

On Tue, Apr 28, 2015 at 05:00:58PM -0700, Jeremy Allison wrote:
> On Wed, Apr 29, 2015 at 11:24:24AM +1200, Andrew Bartlett wrote:
> > On Tue, 2015-04-28 at 15:32 +0200, Volker Lendecke wrote:
> > > From a76d79222518d239aa24d62ca205821ea02c14a6 Mon Sep 17 00:00:00 2001
> > > From: Volker Lendecke <vl at samba.org>
> > > Date: Fri, 2 Jan 2015 11:02:45 +0100
> > > Subject: [PATCH 2/3] lib: Simplify dom_sid_parse_length
> > > 
> > > Signed-off-by: Volker Lendecke <vl at samba.org>
> > > ---
> > >  libcli/security/dom_sid.c |   11 +++--------
> > >  1 file changed, 3 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/libcli/security/dom_sid.c b/libcli/security/dom_sid.c
> > > index 836979e..2910434 100644
> > > --- a/libcli/security/dom_sid.c
> > > +++ b/libcli/security/dom_sid.c
> > > @@ -243,14 +243,9 @@ struct dom_sid *dom_sid_parse_talloc(TALLOC_CTX *mem_ctx, const char *sidstr)
> > >  */
> > >  struct dom_sid *dom_sid_parse_length(TALLOC_CTX *mem_ctx, const DATA_BLOB *sid)
> > >  {
> > > -       struct dom_sid *ret;
> > > -       char *p = talloc_strndup(mem_ctx, (char *)sid->data, sid->length);
> > > -       if (!p) {
> > > -               return NULL;
> > > -       }
> > > -       ret = dom_sid_parse_talloc(mem_ctx, p);
> > > -       talloc_free(p);
> > > -       return ret;
> > > +       char p[sid->length+1];
> > > +       memcpy(p, sid->data, sizeof(p));
> > > +       return dom_sid_parse_talloc(mem_ctx, p);
> > >  }
> > 
> > Can you please explain to me how this works?  
> > 
> > As I see it, we now read 1 byte beyond the DATA_BLOB length, copy that,
> > and then presume this uninitialised value is a NULL terminator for
> > dom_sid_parse_talloc(), but without explicitly setting it.
> > 
> > What am I missing?
> 
> Oh, I reviewed that. Sorry, looks like I missed that :-(.

So it probably should be:

+       char p[sid->length+1];
+       memcpy(p, sid->data, sid->length);
+	p[sid->length] = 0;
+       return dom_sid_parse_talloc(mem_ctx, p);

Looks better ? Sorry for the mistake, that was my fault
for not reviewing carefully enough :-(. I'll try and be
more careful in future.

Jeremy.