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

[Jeremy Allison]

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

Cheers,

Jeremy.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-s4-Fix-bad-review-I-did-in-dom_sid_parse_length-code.patch
Type: text/x-diff
Size: 981 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20150428/d2fa506d/attachment.patch>