[PATCH] Fix bug #Bug 12831 - smbcacls got error NT_STATUS_NETWORK_NAME_DELETED

[Jeremy Allison]

On Fri, Jun 16, 2017 at 11:16:34AM -0700, Richard Sharpe wrote:
> On Wed, Jun 14, 2017 at 8:50 AM, Jeremy Allison via samba-technical
> <samba-technical at lists.samba.org> wrote:
> > It turns out that smbclient, smbcacls and smbtorture3 depend
> > on the ability to temporarily replace the client tcon connection
> > struct internally to the client with a new connection to IPC$,
> > do some calls on that new connection (usually lookupnames/lookupsids),
> > then replace the old values of the tcon and continue.
> >
> > This has some problems. (a). It only works for SMB1 (for SMB2 we
> > overwrote the tcon pointer) and (b). It didn't really work for SMB1
> > either - we ended up with a bastardized cli->smb1.tcon pointer that
> > contains type and string from the IPC$ connection whilst being
> > connected to the share connection. We only got away with this
> > due to the fact we don't use the type and string for anything
> > on the wire.
> >
> > This patch unifies the cli->smb[1|2].tcon pointer handling and
> > allows temporary replacement of the pointer, and also SMB2 access
> > to the get/set the 32-bit tid value.
> >
> > Most of this patch is fixing up the smbclient/smbcacls/smbtorture3
> > code so everything keeps working with make test, so I know this
> > is being tested (it took me a day or so to keep all our tests
> > passing :-).
> >
> > Please review and push if you're happy !
> 
> Minor nit. Here:
> ----------------------
> diff --git a/source3/torture/torture.c b/source3/torture/torture.c
> index 58f0ce0a31f..378cb8ba35f 100644
> --- a/source3/torture/torture.c
> +++ b/source3/torture/torture.c
> @@ -1302,6 +1302,7 @@ static bool run_tcon_test(int dummy)
>         const char *fname = "\\tcontest.tmp";
>         uint16_t fnum1;
>         uint32_t cnum1, cnum2, cnum3;
> +       struct smbXcli_tcon *orig_tcon = NULL;
>         uint16_t vuid1, vuid2;
>         char buf[4];
>         bool ret = True;
> @@ -1333,6 +1334,11 @@ static bool run_tcon_test(int dummy)
>                 return False;
>         }
> 
> +       orig_tcon = cli_state_save_tcon(cli);
> +       if (orig_tcon == NULL) {
> +               return false;
> +       }
> +
>         status = cli_tree_connect_creds(cli, share, "?????", torture_creds);
>         if (!NT_STATUS_IS_OK(status)) {
>                 printf("%s refused 2nd tree connect (%s)\n", host,
> @@ -1400,6 +1406,8 @@ static bool run_tcon_test(int dummy)
>                 return False;
>         }
> 
> +       cli_state_restore_tcon(cli, orig_tcon);
> +
>         cli_state_set_tid(cli, cnum1);
> 
>         if (!torture_close_connection(cli)) {
> -------------------------
> 
> it looks like maybe you can remove this line around 1327
> 
>        cnum1 = cli_state_get_tid(cli);
> 
> and this line around 1403 just after you restore the tcon:
> 
>        cli_state_set_tid(cli, cnum1);
> 
> You seem to have done that in at least
> source3/utils/net_rpc.c:show_userlist but not in
> source3/torture/torture.c:run_tcon_test. However, you might have a
> good reason for that.

Yes, I can remove it from net_rpc.c:show_userlist as that is
simply doing a "temp replace tcon with an IPC$ connection"
step. Inside torture.c:run_tcon_test() it's doing deliberate
manipulation of tcon values to check invalid numbers fail,
so I wanted to make as few changes as possible here.

It's possible that change might work, but I didn't want
to make passing a full "make test" any harder than it
already was :-).

> Apart from that: Reviewed-by: Richard Sharpe <realrichardsharpe at gmail.com>

Thanks a *lot* for the review Richard !

Jeremy.