[SCM] Samba Shared Repository - branch v3-2-test updated - initial-v3-2-unstable-419-g9d0034f

[Jeremy Allison]

On Fri, Nov 30, 2007 at 11:48:19PM -0500, Derrell Lipman wrote:
> 
> > @@ -2744,7 +2743,14 @@ smbc_opendir_ctx(SMBCCTX *context,
> >                 return NULL;
> >         }
> >
> > -       if (user[0] == (char)0) fstrcpy(user, context->user);
> > +       if (!user || user[0] == (char)0) {
> > +               user = talloc_strdup(frame, context->user);
> > +               if (!user) {
> > +                       errno = ENOMEM;
> > +                       TALLOC_FREE(frame);
> > +                       return NULL;
> > +               }
> > +       }
> 
> I don't know how user[0] can become a null character, but if for some
> reason 'user' is non-null but user[0] is a null character, the if
> statement will be entered and user will be overwritten.  It's on
> context 'frame' so maybe that's ok, but it'd probably be cleaner if
> user were deallocated before being allocated if it was non-null.

If user isn't correctly parsed - or there's something
wrong with the url, then the parse function will return
user talloced as one byte of '\0'. This actually matches
what the previous code did (it wrote one byte of '\0'
into the fixed user buffer at the start of the function,
so would remain that way on any error return).

Yes, it would be cleaner to call TALLOC_FREE(user)
before overwriting, but will be cleaned up on the
exit from this function anyway (when the frame
gets freed).

> > @@ -2847,8 +2853,13 @@ smbc_opendir_ctx(SMBCCTX *context,
> >                        if ( !cli )
> >                                continue;
> >
> > -                       pstrcpy(workgroup, wg_ptr);
> > -                        fstrcpy(server, cli->desthost);
> > +                       workgroup = talloc_strdup(frame, wg_ptr);
> > +                       server = talloc_strdup(frame, cli->desthost);
> > +                       if (!workgroup || !server) {
> > +                               errno = ENOMEM;
> > +                               TALLOC_FREE(frame);
> > +                               return NULL;
> > +                       }
> 
> This is newly-added clean-up (error handling) code where none was
> previously needed.  Are we sure that's all the cleanup required before
> returning?

You're correct, I've missed the shutdown. I'll fix that,
thanks !

Jeremy.