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

[Andrew Bartlett]

On Tue, 2015-04-28 at 17:22 -0700, Jeremy Allison wrote:
> On Tue, Apr 28, 2015 at 05:07:22PM -0700, Jeremy Allison wrote:
> > 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.
> 
> And here's a reviewable version. Please check *carefully* :-).

Thanks.  The fix is reviewed and in autobuild now. 

Andrew Bartlett

-- 
Andrew Bartlett
http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba