Bug 13992 - SAMBA RPC share error in SAMBA Stretch 4.5.16 and Buster 4.9.5
Summary: SAMBA RPC share error in SAMBA Stretch 4.5.16 and Buster 4.9.5
Status: ASSIGNED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: x64 Linux
: P2 normal (vote)
Target Milestone: ---
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-06-14 06:38 UTC by Nick Paterakis
Modified: 2021-03-11 11:47 UTC (History)
4 users (show)

See Also:


Attachments
Error Log File (922.64 KB, text/plain)
2019-06-14 06:38 UTC, Nick Paterakis
no flags Details
samba debug output (15.24 KB, text/plain)
2019-06-27 06:17 UTC, Paul Wise
no flags Details
extra samba config file (172 bytes, text/plain)
2019-06-27 06:17 UTC, Paul Wise
no flags Details
git-am fix for master. (7.43 KB, patch)
2021-01-28 19:26 UTC, Jeremy Allison
no flags Details
git-am fix for master. (11.97 KB, patch)
2021-01-28 22:43 UTC, Jeremy Allison
no flags Details
git-am fix for master. (12.34 KB, patch)
2021-01-29 02:05 UTC, Jeremy Allison
no flags Details
backport of Jeremy Allison's latest patch (5.46 KB, patch)
2021-01-29 04:32 UTC, Paul Wise
no flags Details
git-am fix for 4.14.RCnext. (13.15 KB, patch)
2021-02-02 21:11 UTC, Jeremy Allison
asn: review+
Details
git-am fix for 4.12.next, 4.13.next. (13.40 KB, patch)
2021-02-02 21:12 UTC, Jeremy Allison
no flags Details
git-am fix for 4.12.next, 4.13.next. (12.06 KB, patch)
2021-02-02 22:05 UTC, Jeremy Allison
asn: review+
Details
Additional git-am fix for 4.13.next. (881 bytes, patch)
2021-02-04 01:45 UTC, Jeremy Allison
asn: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Paterakis 2019-06-14 06:38:14 UTC
Created attachment 15249 [details]
Error Log File

Hi there we recently upgraded some client appliances on Linux from Debian Jesse to Debian Stretch and encountered an issue in the process with SAMBA. We discovered that net rpc share allowedusers from Samba in Debian stretch (4.5.16) and buster (4.9.5) only returns info about which users are allowed to mount the first share on the server.

When running the command this is the significant error message:

> $ net usersidlist | net rpc share allowedusers -d10 -U user@password
> -S10.10.10.1 |& grep secdesc
> Could not query secdesc for share sharename
 
The difference in the protocol trace is that old samba sends one NetShareGetInfo RPC call for each share but the new one sends only the first share returned by the NetShareEnumAll RPC call, which returns the full list of shares in both cases.

When debugging this in gdb, it looks like dcerpc_srvsvc_NetShareGetInfo is called for each of the shares but returns 0xC000020C (NT_STATUS_CONNECTION_DISCONNECTED) for everything except the first share.

Also attaching a log file from our local test harness where we've replicated the issue.

The workaround, which works, is to downgrade to Jesse-SAMBA.
Comment 1 Louis 2019-06-26 10:02:30 UTC
Hai, 

I would start with update your smb.conf, you running version is not correct. 

Your on debian, can you run this script, that give us a better overview on you settings. 

wget https://raw.githubusercontent.com/thctlo/samba4/master/samba-collect-debug-info.sh |bash    
and post the output, anonimize it where needed. 

For the smb.conf, i suggest, go here: 
https://wiki.samba.org/index.php/Setting_up_Samba_as_a_Domain_Member 
Start reading as of : Setting up a Basic smb.conf File 

And since you are upgrading from jessie ( samba 4.2 ) read also : 
https://wiki.samba.org/index.php/Updating_Samba 

I'm not saying, this is not a bug, but we want correct settings before we say its a bug.
Comment 2 Paul Wise 2019-06-27 06:17:27 UTC
Created attachment 15266 [details]
samba debug output
Comment 3 Paul Wise 2019-06-27 06:17:46 UTC
Created attachment 15267 [details]
extra samba config file
Comment 4 Paul Wise 2019-06-27 06:23:36 UTC
I've attached the debug output from the script.

The script missed the include directive we use smb.conf so I attached that too.

The script seems to think that winbindd is not running but it is:

    $ systemctl status winbind
    ● winbind.service - Samba Winbind Daemon
       Loaded: loaded (/lib/systemd/system/winbind.service; enabled; vendor preset: enabled)
       Active: active (running) since Thu 2019-06-13 08:05:55 ACST; 2 weeks 0 days ago
         Docs: man:winbindd(8)
               man:samba(7)
               man:smb.conf(5)
     Main PID: 822 (winbindd)
       Status: "winbindd: ready to serve connections..."
        Tasks: 5 (limit: 4915)
       CGroup: /system.slice/winbind.service
               ├─ 822 /usr/sbin/winbindd
               ├─1023 /usr/sbin/winbindd
               ├─3152 /usr/sbin/winbindd
               ├─3153 /usr/sbin/winbindd
               └─3155 /usr/sbin/winbindd

As far as I can tell we have configured samba and joined the domain correctly.
Comment 5 Nick Paterakis 2019-07-11 00:44:27 UTC
Hi SAMBA team - are you able to update this ticket and assess the additional diag information provided for a solution please?
Comment 6 Nick Paterakis 2019-08-23 02:00:52 UTC
Hi - we've been patiently awaiting an updated to this ticket and followed the last set of diagnostice steps to provide additional info as requested. Can we now please escalate this for action?
Comment 7 Andrew Bartlett 2019-08-23 02:42:02 UTC
(In reply to Nick Paterakis from comment #6)

The escalation path is here:
https://www.samba.org/samba/support/
Comment 8 Nick Paterakis 2019-08-23 03:31:36 UTC
Would prefer to maintain resolution within the bug raised - we provided diag information as request and are keen for the dev team to assess and respond as to when a solution wil be rendered?
Comment 9 Andrew Bartlett 2019-08-27 00:41:08 UTC
(In reply to Nick Paterakis from comment #8)
To be clear, the Samba Bugzilla is a bug tracking system, not a resource allocation system.

However, to help progress this:

Nothing in the info provided so far suggests a trivial fix, but perhaps more detail could be found if you turn up the log level.

My best suggestion, if you have the time, is to bisect between the two versions using git bisect and determine which patch broke the behaviour. 

That may help make the issue clear, which may in turn make this an attractive bug for a developer to take on.

Otherwise, my previous comment holds, Samba is significantly supported by those who support the companies that employ Samba developers.
Comment 10 Paul Wise 2019-08-27 01:11:44 UTC
Turning up the log level doesn't add any more messages.

I'll take a look at bisecting the issue.
Comment 11 Paul Wise 2019-08-27 04:43:59 UTC
The results of the bisect are that the following three commits are involved:

The first one (dc4a6a980a1) fails in the same way as our initial tests.

The second one (c939552b7e5) segfaults after printing the first user.

The third one (0d0d9820531) succeeds just like samba 4.2 from Debian jessie.

I'm inclined to think that the second one is responsible for this issue, since it also has a failure after the first user, but a failure of a different kind; SEGFAULT instead of connection errors.

commit dc4a6a980a16f7effb2b422ad6332936f457546c (refs/bisect/bad)
Author: Jeremy Allison <jra@samba.org>
Date:   Tue Jun 13 16:56:48 2017 -0700

    s3: libsmb: Correctly save and restore connection tcon in smbclient, smbcacls and smbtorture3.
    
    BUG: https://bugzilla.samba.org/show_bug.cgi?id=12831
    
    Signed-off-by: Jeremy Allison <jra@samba.org>
    Reviewed-by: Richard Sharpe <realrichardsharpe@gmail.com>
    (cherry picked from commit bd31d538a26bb21cbb53986a6105204da4392e2d)

commit c939552b7e52396ab78419ae0706759ff3ca30a3 (refs/bisect/skip-c939552b7e52396ab78419ae0706759ff3ca30a3)
Author: Jeremy Allison <jra@samba.org>
Date:   Tue Jun 13 16:37:39 2017 -0700

    s3: libsmb: Correctly do lifecycle management on cli->smb1.tcon and cli->smb2.tcon.
    
    Treat them identically. Create them on demand after for a tcon call,
    and delete them on a tdis call.
    
    BUG: https://bugzilla.samba.org/show_bug.cgi?id=12831
    
    Signed-off-by: Jeremy Allison <jra@samba.org>
    Reviewed-by: Richard Sharpe <realrichardsharpe@gmail.com>
    (cherry picked from commit 50f50256aa8805921c42d0f9f2f8f89d06d9bd93)

commit 0d0d9820531aca17a5300f4e4eb47f07a999aaca (refs/bisect/good-0d0d9820531aca17a5300f4e4eb47f07a999aaca)
Author: Jeremy Allison <jra@samba.org>
Date:   Tue Jun 13 16:36:54 2017 -0700

    s3: libsmb: Fix cli_state_has_tcon() to cope with SMB2 connections.
    
    BUG: https://bugzilla.samba.org/show_bug.cgi?id=12831
    
    Signed-off-by: Jeremy Allison <jra@samba.org>
    Reviewed-by: Richard Sharpe <realrichardsharpe@gmail.com>
    (cherry picked from commit c9178ed9cc69b9089292db28ac1a0b7a0519bc2c)
Comment 12 Paul Wise 2019-08-27 04:52:01 UTC
These commits are between 4.5.10 and 4.5.11.
Comment 13 Andrew Bartlett 2019-08-27 06:23:58 UTC
(In reply to Paul Wise from comment #12)
Can you still reproduce on current master.  I have a feeling I've seen this fixed already, but can't find it trivially.  

If not a tour of the git history for similar issues fixes elsewhere recently *might* give a clue as to the fix.
Comment 14 Paul Wise 2019-08-27 07:27:46 UTC
I'm still able to reproduce the issue with git master (b406b928242).
Comment 15 Paul Wise 2019-08-27 07:50:37 UTC
I've take a look at recent history but I didn't see anything related.
Comment 16 Paul Wise 2019-08-28 05:43:17 UTC
If I check out the first commit (dc4a6a980a1) and then revert the second one (c939552b7e5) then I get the correct results, so I think the issue is definitely caused by the second commit (c939552b7e5).
Comment 17 Paul Wise 2019-08-28 05:55:32 UTC
Looking at the first commit, I see that in it, the show_userlist function is missing a call to cli_state_restore_tcon in the error path while other functions do not miss that call. Testing if that changes anything.
Comment 18 Paul Wise 2019-08-28 05:56:01 UTC
That didn't fix the issue.
Comment 19 Paul Wise 2019-08-28 08:49:17 UTC
After some debugging:

Tt appears that the NT_STATUS_CONNECTION_DISCONNECTED value originates from rpccli_bh_raw_call_send when it detects that the stream is not connected. The stream is marked as not connected when rpccli_bh_is_connected calls rpccli_is_connected, which indirectly calls rpc_tstream_is_connected, which calls tstream_pending_bytes, which indirectly calls tstream_smbXcli_np_pending_bytes which calls smbXcli_conn_is_connected, which notices that the cli_nps->conn pointer is NULL and returns false to indicate the stream is not connected.

The conn pointer appears to become NULL in tstream_smbXcli_np_ref_destructor, which is called from TALLOC_FREE(cli->smb2.tcon) in cli_tree_connect_send called from cli_tree_connect. Commenting out that line somehow fixes the issue, but that clearly isn't the correct fix.
Comment 20 Paul Wise 2019-08-31 08:54:29 UTC
In summary:

The issue is that in cli_tree_connect_send, TALLOC_FREE(cli->smb2.tcon) clears the connection but smbXcli_tcon_create(cli) on the next line doesn't set the connection to something other than NULL.

The workaround for this issue is to pass one share on the command-line instead of passing no shares or passing more than one share.
Comment 21 Paul Wise 2019-09-08 05:52:42 UTC
I tried saving and restoring the conn pointer before/after the problematic free but that didn't help.
Comment 22 Nick Paterakis 2019-12-09 02:22:52 UTC
Hi Samba team - it's been a little while since we provided you with tech/debug detail affirming the bug condition - are we likely to see a resolution soon? This has blocked us from moving our clients to a new platfrom?
Comment 23 Rowland Penny 2020-12-22 08:50:15 UTC
(In reply to Nick Paterakis from comment #22)
Is this still a valid bug ?
From the output from Louis's test script it doesn't look like the samba package is installed or that winbind is set in /etc/nsswitch.conf, there doesn't seem to be any 'idmap config' lines for the DOMAIN, unless they are in the include file.
'security = ADS' is set, along with 'server role = standalone server', it cannot be both.

All in all, I think this should have been sorted out on the samba mailing list, before logging a bug.

If you are still having problems Nick, lets try to sort it out.
Comment 24 Paul Wise 2021-01-06 06:42:33 UTC
We are now working around this issue by running `net rpc share allowedusers` in a loop once per share instead of just once to get the info for all shares. This is obviously slightly less efficient, so we definitely would like to see this fixed eventually.

This bug is still present as we are still running the Debian bullseye version.

smbd is not installed because these are client machines with no need for serving files, they only join the domain and access SMB/CIFS shares on other machines.

Looking at our code, we only enable winbind in /etc/nsswitch.conf when the machine needs to have SSH logins from users in AD. Most of the time our appliances don't use AD logins for SSH, they only have a couple of hardcoded users.

The 'idmap config' stuff is definitely enabled like this:

   idmap config * : range = 10000-4000000000
   idmap config * : backend = tdb

The 'security = ADS' line is set in an include as noted in comment#3.

I assume that winbind does not look at the 'server role = standalone server' and we are not running any smbd or other samba components.
Comment 25 Paul Wise 2021-01-06 06:43:02 UTC
Er, I meant the Debian buster version.
Comment 26 Rowland Penny 2021-01-07 20:10:58 UTC
(In reply to Paul Wise from comment #24)
OK, remove the 'server role = standalone server' line and then set up your 'idmap config' lines correctly, what you have is insufficient, you also need lines similar to these:

   idmap config FRISKDEMO : backend = rid
   idmap config FRISKDEMO : range = 4000000001-5000000000

Though the ranges might be incorrect and you may be able to use the 'ad' backend if you have uidNumber & gidNumber attributes in AD. You cannot just use the default (*) domain, not unless you use the 'autorid' backend. I would assume that what ever you do now, you are now going to have ID problems.
Comment 27 Paul Wise 2021-01-28 08:21:23 UTC
I've re-setup our test system for this issue and reproduced the bug again. Now I'll make the proposed configuration changes and report the results.
Comment 28 Paul Wise 2021-01-28 08:24:00 UTC
Commenting out 'server role = standalone server' and restarting winbind had no effect.
Comment 29 Paul Wise 2021-01-28 08:27:56 UTC
Adding the suggested idmap lines and restarting winbind had no effect.
Comment 30 Paul Wise 2021-01-28 08:37:36 UTC
(In reply to Rowland Penny from comment #26)
I've applied the suggested changes and there was no effect.

As I've described earlier in the bug I've already done a protocol trace (comment #0) and pinpointed the issue. I've already done a git bisect and pinpointed the commit causing the issue (comment #16) and the problem in the code causing the issue (comment #20).

After the first share, the connection gets disconnected. The problem is in the 'net' command not winbind, since repeating the command has the same results; works for the first share, fails for the rest.
Comment 31 Paul Wise 2021-01-28 08:42:31 UTC
PS: if you want to discuss this in realtime, I am on the #samba IRC channel on freenode, my timezone is Australia/Perth (UTC+8).
Comment 32 Jeremy Allison 2021-01-28 17:25:22 UTC
Just became aware of this bug.

In comment #20 you state:

> The issue is that in cli_tree_connect_send, TALLOC_FREE(cli->smb2.tcon) clears
> the connection but smbXcli_tcon_create(cli) on the next line doesn't set the 
> connection to something other than NULL.

In current master, cli_tree_connect_send() has:

source3/libsmb/cliconnect.c:cli_tree_connect_send()

2293         if (smbXcli_conn_protocol(cli->conn) >= PROTOCOL_SMB2_02) {
2294                 char *unc;
2295 
2296                 TALLOC_FREE(cli->smb2.tcon);
2297                 cli->smb2.tcon = smbXcli_tcon_create(cli);
2298                 if (tevent_req_nomem(cli->smb2.tcon, req)) {
2299                         return tevent_req_post(req, ev);
2300                 }
2301 
2302                 unc = talloc_asprintf(state, "\\\\%s\\%s",
2303                                       smbXcli_conn_remote_name(cli->conn),
2304                                       share);
2305                 if (tevent_req_nomem(unc, req)) {
2306                         return tevent_req_post(req, ev);
2307                 }
2308 
2309                 subreq = smb2cli_tcon_send(state, ev, cli->conn, cli->timeout,
2310                                            cli->smb2.session, cli->smb2.tcon,
2311                                            0, /* flags */
2312                                            unc);
2313                 if (tevent_req_nomem(subreq, req)) {
2314                         return tevent_req_post(req, ev);
2315                 }
2316                 tevent_req_set_callback(subreq, cli_tree_connect_smb2_done,
2317                                         req);
2318                 return req;
2319         }

Line 2297 creates a new struct smbXcli_tcon, which is passed to smb2cli_tcon_send() in line 2309 (2310 is the parameter).

Now look at:

libcli/smb/smb2cli_tcon.c:smb2cli_tcon_send()

The passed in tcon parameter (which is the struct smbXcli_tcon *)

 59         state->ev = ev;
 60         state->conn = conn;
 61         state->timeout_msec = timeout_msec;
 62         state->session = session;
 63         state->tcon = tcon;

is set in state->tcon in line 63. The callback function is then set:

 96         subreq = smb2cli_req_send(state, ev, conn, SMB2_OP_TCON,
 97                                   additional_flags, clear_flags,
 98                                   timeout_msec,
 99                                   NULL, /* tcon */
100                                   session,
101                                   state->fixed, sizeof(state->fixed),
102                                   dyn, dyn_len,
103                                   0); /* max_dyn_len */
104         if (tevent_req_nomem(subreq, req)) {
105                 return tevent_req_post(req, ev);
106         }
107         tevent_req_set_callback(subreq, smb2cli_tcon_done, req);

on line 107. This gets called back when the server
responds to the SMB2_OP_TCON request.

In libcli/smb/smb2cli_tcon.c:smb2cli_tcon_done()

we set the values inside the struct smbXcli_tcon * pointer
stored in the state struct below:

143         tcon_id = IVAL(iov[0].iov_base, SMB2_HDR_TID);
144 
145         body = (uint8_t *)iov[1].iov_base;
146         share_type              = CVAL(body, 0x02);
147         share_flags             = IVAL(body, 0x04);
148         share_capabilities      = IVAL(body, 0x08);
149         maximal_access          = IVAL(body, 0x0C);
150 
151         smb2cli_tcon_set_values(state->tcon,
152                                 state->session,
153                                 tcon_id,
154                                 share_type,
155                                 share_flags,
156                                 share_capabilities,
157                                 maximal_access);

libcli/smb/smbXcli_base.c:smb2cli_tcon_set_values()

is simple and just sets the passed in values to the pointer.

6622 void smb2cli_tcon_set_values(struct smbXcli_tcon *tcon,
6623                              struct smbXcli_session *session,
6624                              uint32_t tcon_id,
6625                              uint8_t type,
6626                              uint32_t flags,
6627                              uint32_t capabilities,
6628                              uint32_t maximal_access)
6629 {
6630         tcon->is_smb1 = false;
6631         tcon->fs_attributes = 0;
6632         tcon->smb2.tcon_id = tcon_id;
6633         tcon->smb2.type = type;
6634         tcon->smb2.flags = flags;
6635         tcon->smb2.capabilities = capabilities;
6636         tcon->smb2.maximal_access = maximal_access;
6637 
6638         tcon->smb2.should_sign = false;
6639         tcon->smb2.should_encrypt = false;
6640 
6641         if (session == NULL) {
6642                 return;
6643         }
6644 
6645         tcon->smb2.should_sign = session->smb2->should_sign;
6646         tcon->smb2.should_encrypt = session->smb2->should_encrypt;
6647 
6648         if (flags & SMB2_SHAREFLAG_ENCRYPT_DATA) {
6649                 tcon->smb2.should_encrypt = true;
6650         }
6651 }

So that's where the returned cli->smb2.tcon struct smbXcli_tcon * pointer
values get set in the SMB2 client case.

Note I'm assuming here this is an SMB2 case.

Nick, your error analysis is really good but I can't see how
this is happening in current master.
Comment 33 Jeremy Allison 2021-01-28 17:38:35 UTC
Sorry, credit where it's due. Error analysis from Paul.
Comment 34 Jeremy Allison 2021-01-28 17:53:47 UTC
Also note that your error log file in attachment:

https://bugzilla.samba.org/attachment.cgi?id=15249

does not show any NT_STATUS_CONNECTION_DISCONNECTED errors in the log.
Comment 35 Jeremy Allison 2021-01-28 18:30:35 UTC
Ah, I see the problem. It's in libcli/smb/tstream_smbXcli_np.c:_tstream_smbXcli_np_open_recv().

 342         cli_nps->tcon_ref = talloc_zero(state->tcon,
 343                                         struct tstream_smbXcli_np_ref);

cli_nps->tcon_ref has a parent of state->tcon, which comes from cli->smb2.tcon, being set here in:

libcli/smb/tstream_smbXcli_np.c:tstream_smbXcli_np_open_send()

 207         state->conn = conn;
 208         state->tcon = tcon;
 209         state->session = session;
 210         state->pid = pid;
 211         state->timeout = timeout;

A talloc destructor is then attached to cli_nps->tcon_ref here:

 367         talloc_set_destructor(cli_nps->tcon_ref,
 368                               tstream_smbXcli_np_ref_destructor);

and tstream_smbXcli_np_ref_destructor() does the following:

 146 static int tstream_smbXcli_np_ref_destructor(struct tstream_smbXcli_np_ref *ref)
 147 {
 148         if (ref->cli_nps == NULL) {
 149                 return 0;
 150         }
 151 
 152         if (ref->cli_nps->conn == NULL) {
 153                 return 0;
 154         }
 155 
 156         ref->cli_nps->conn = NULL;
 157         ref->cli_nps->session = NULL;
 158         ref->cli_nps->tcon = NULL;
 159 
 160         TALLOC_FREE(ref->cli_nps->conn_ref);
 161         TALLOC_FREE(ref->cli_nps->session_ref);
 162         TALLOC_FREE(ref->cli_nps->tcon_ref);
 163 
 164         return 0;
 165 };

So when TALLOC_FREE(cli->smb2.tcon) is called this shuts down the pipe.
Comment 36 Jeremy Allison 2021-01-28 18:33:59 UTC
The save/restore tcon code does the following:

struct smbXcli_tcon *cli_state_save_tcon(struct cli_state *cli)
{
        if (smbXcli_conn_protocol(cli->conn) >= PROTOCOL_SMB2_02) {
                return smbXcli_tcon_copy(cli, cli->smb2.tcon);
        } else {
                return smbXcli_tcon_copy(cli, cli->smb1.tcon);
        }
}

void cli_state_restore_tcon(struct cli_state *cli, struct smbXcli_tcon *tcon)
{
        if (smbXcli_conn_protocol(cli->conn) >= PROTOCOL_SMB2_02) {
                TALLOC_FREE(cli->smb2.tcon);
                cli->smb2.tcon = tcon;
        } else {
                TALLOC_FREE(cli->smb1.tcon);
                cli->smb1.tcon = tcon;
        }
}

which is making a *copy* of the data inside cli->smb2.tcon or cli->smb1.tcon, but this will destroy any talloc children of the tcon struct.

Instead it should just return the pointer itself I think and then set the cli->smbX.tcon pointer to NULL.
Comment 37 Jeremy Allison 2021-01-28 19:26:26 UTC
Created attachment 16412 [details]
git-am fix for master.

Paul and Nick, can you test this please ? I'm pretty sure this will fix the problem. I need to write a regression test before pushing upstream but it would be good to get confirmation first.

Thanks,

Jeremy.
Comment 38 Jeremy Allison 2021-01-28 22:43:44 UTC
Created attachment 16414 [details]
git-am fix for master.

Includes regression test showing the problem. Pushing to gitlab CI now.
Comment 39 Paul Wise 2021-01-29 00:59:25 UTC
Thanks for the extra analysis and the patch, I will test the fix today, probably backported to the old samba version we are using, or possibly the one in Debian bullseye.
Comment 40 Jeremy Allison 2021-01-29 01:14:07 UTC
Hmmm. Never mind, I still need to do some work to ensure samba3.smbtorture_s3.plain.UID-REGRESSION-TEST keeps working.

That does some seriously horrible things in the test env that our real code doesn't do..
Comment 41 Jeremy Allison 2021-01-29 02:05:16 UTC
Created attachment 16415 [details]
git-am fix for master.

OK, this version one passes:

make test TESTS=TCON
make test TESTS=samba3.blackbox.net_rpc_share_allowedusers
make test TESTS=UID-REGRESSION-TEST

I had to tweak the code run_uid_regression_test() as that does some *HORRIBLE* things to the tcon struct values and has to remain as a deep copy. It doesn't do any pipe opens or actual real tcon re-opens which the actual libsmb code does in real-world use so I was able to move the use of the horrible struct deep-copy code only into source3/torture/torture.c.

Unfortunately that means I can't remove the smbXcli_tcon_copy() function code as it's now still used inside source3/torture/torture.c but c'est la vie !

Now in gitlab-ci:

https://gitlab.com/samba-team/devel/samba/-/pipelines/248446630
Comment 43 Paul Wise 2021-01-29 04:32:45 UTC
Created attachment 16416 [details]
backport of Jeremy Allison's latest patch

I've backported the latest patchset from the MR (see attachment) and I am building samba and testing the fix on our test system.
Comment 44 Paul Wise 2021-01-29 05:16:23 UTC
Turns out there is a misconfiguration on the Windows side in our new test setup, so I'm going to try to get that rectified before retesting both with and without the patch.
Comment 45 Paul Wise 2021-02-01 01:54:17 UTC
We resolved the test appliance misconfiguration, confirmed the bug was present, installed the patched version and then confirmed that the backported patch fixes the problem! Thanks for your work Jeremy! Please merge the patch into the main samba branch and backport it to all the supported releases.
Comment 46 Samba QA Contact 2021-02-02 21:06:03 UTC
This bug was referenced in samba master:

068f4a977f0539f790809d580bf22d2362032e3d
faba89ad59eaa189f325be17377645862080a965
dc701959cad7bf15aa47cad6451212606520f67f
f9ca91bd293e9f2710c4449c5d4f5d016a066049
e93e6108837eff0cebad8dc26d055c0e1386093a
4f80f5f9046b64a9e5e0503b1cb54f1492c4faec
Comment 47 Jeremy Allison 2021-02-02 21:11:46 UTC
Created attachment 16422 [details]
git-am fix for 4.14.RCnext.
Comment 48 Jeremy Allison 2021-02-02 21:12:59 UTC
Created attachment 16423 [details]
git-am fix for 4.12.next, 4.13.next.
Comment 49 Jeremy Allison 2021-02-02 22:02:50 UTC
Comment on attachment 16422 [details]
git-am fix for 4.14.RCnext.

Cherry-picked from master.
Comment 50 Jeremy Allison 2021-02-02 22:05:58 UTC
Created attachment 16424 [details]
git-am fix for 4.12.next, 4.13.next.

Cherry-picked from master minus one fix that isn't relevent.
Comment 51 Andrew Bartlett 2021-02-03 08:08:22 UTC
Thanks so much Jeremy for following up on this, this was a tricky one.  Thanks also to Paul for your patience and extensive debugging.
Comment 52 Andreas Schneider 2021-02-03 08:51:36 UTC
Karolin, please apply the patches to the relevant branches. Thanks!
Comment 53 Karolin Seeger 2021-02-03 10:01:50 UTC
(In reply to Andreas Schneider from comment #52)
Pushed to autobuild-v4-{14,13,12}-test.
Comment 54 Samba QA Contact 2021-02-03 21:24:06 UTC
This bug was referenced in samba v4-13-test:

47581202cf32c2f9103103d987e6d8e694cee532
2a6ba7ab9eb7d5371e1de2a50d5e63ed214f88f0
643fcfd5566c64001df2ed7d6b313e31f218789c
1b609f046615abb69bf5a59f1df915f46e9853bc
d78648963ed0475dcce2f87b9dc969661fcbfc06
Comment 55 Samba QA Contact 2021-02-03 22:33:03 UTC
This bug was referenced in samba v4-14-test:

42f41c5ca5e7138ef4eb9ad428e05e7c2760d528
7125792f0e15852c09482c6035b176f92cec6741
55294ccdeca3a5758256427324da6cfc1b87acd4
b6183a479ca2fdebba123aaa966c2d8041036a62
b6a9277beaeb7dd113ee6eb95243af8701985216
df0dd2ae007e96261fb98e3cf858543c116b81ab
Comment 56 Paul Wise 2021-02-04 01:21:14 UTC
I noticed that one of the changes is missing from the samba v4-13-test branch:

    s3: libsmb: Ensure we disconnect the temporary SMB1 tcon pointer on failure to set up encryption.
    
    BUG: https://bugzilla.samba.org/show_bug.cgi?id=13992
    
    Signed-off-by: Jeremy Allison <jra@samba.org>

Is this intentional? It appears to apply fine, here is my backport of it:

https://gitlab.com/pabs3/samba/-/compare/v4-13-test...fix-net-rpc-share-allowedusers/bullseye.patch
Comment 57 Jeremy Allison 2021-02-04 01:27:48 UTC
Yes, that was intentional for 4.13.

That specific patch didn't apply to my v4-13-test branch, the code inside cli_check_msdfs_proxy() in v4-13-test doesn't look like the 4.14.x code.
Comment 58 Paul Wise 2021-02-04 01:37:03 UTC
I don't know anything about the cli_check_msdfs_proxy() function, but the "if (force_encrypt) {" condition in 4.13 seemed to be similar to or the same as the "case SMB_ENCRYPTION_REQUIRED:" condition in 4.14, so I copied the comment and cli_tdis(cli) call to the 4.13 failure condition just before the cli_state_restore_tcon(cli, orig_tcon) call, same as in 4.14.
Comment 59 Jeremy Allison 2021-02-04 01:40:10 UTC
The missing patch changed the cli_check_msdfs_proxy() code in 4.14. In 4.13 the save/restore is already being done correctly so I don't think it's needed.
Comment 60 Jeremy Allison 2021-02-04 01:42:44 UTC
Oh, now I see it. It's a missing cli_tdis(cli) before the cli_state_restore_tcon(cli, orig_tcon) call in the cli_check_msdfs_proxy() function.

This is in a rarely used error code path (encryption forced on client side, but server doesn't support) so it's not a critical fix.
Comment 61 Jeremy Allison 2021-02-04 01:45:08 UTC
Created attachment 16426 [details]
Additional git-am fix for 4.13.next.

I missed this error code path fixup in the original backport of the code from master to 4.13. Andreas, give it a +1 if you think it's required. Thanks, Jeremy.
Comment 62 Paul Wise 2021-02-04 01:46:26 UTC
Yes, thats what I was referring to, thanks for re-checking.
Comment 63 Samba QA Contact 2021-02-04 08:26:42 UTC
This bug was referenced in samba v4-14-stable (Release samba-4.14.0rc2):

42f41c5ca5e7138ef4eb9ad428e05e7c2760d528
7125792f0e15852c09482c6035b176f92cec6741
55294ccdeca3a5758256427324da6cfc1b87acd4
b6183a479ca2fdebba123aaa966c2d8041036a62
b6a9277beaeb7dd113ee6eb95243af8701985216
df0dd2ae007e96261fb98e3cf858543c116b81ab
Comment 64 Andreas Schneider 2021-02-05 10:24:18 UTC
Comment on attachment 16426 [details]
Additional git-am fix for 4.13.next.

LGTM
Comment 65 Karolin Seeger 2021-02-05 10:32:14 UTC
(In reply to Andreas Schneider from comment #64)
Thx! Pushed to autobuild-v4-13-test.
Comment 66 Samba QA Contact 2021-02-05 12:15:04 UTC
This bug was referenced in samba v4-12-test:

a00ff4345157f8d04b7c58325a3e877a9d1f9a95
80d2c3e4725689b9bf353c2f2356a451f0ee6711
a19f94c644d552466fd9f15f1b1d9c04d6fac0e9
bab7f2ae28e5bd0a44e121c9b7e9d868271fac70
6e6aa90b87b5ddaa6b92160eb467090705e25ff6
Comment 67 Paul Wise 2021-02-08 10:33:45 UTC
Should that extra patch get backported to 4.12 too?
Comment 68 Samba QA Contact 2021-02-08 11:43:20 UTC
This bug was referenced in samba v4-13-test:

4914efd0cc4818a7334344ce417f3ddfbaac8e2d
Comment 69 Karolin Seeger 2021-02-09 12:38:11 UTC
Hi Jeremy,

would you please comment if backporting to v4-12 makes sense?

Thanks!

Karo
Comment 70 Jeremy Allison 2021-02-09 16:50:53 UTC
Yes, it applies to 4.12.next also and should be added there too. Sorry for not being clear enough initially.
Comment 71 Samba QA Contact 2021-03-09 11:01:43 UTC
This bug was referenced in samba v4-13-stable (Release samba-4.13.5):

47581202cf32c2f9103103d987e6d8e694cee532
2a6ba7ab9eb7d5371e1de2a50d5e63ed214f88f0
643fcfd5566c64001df2ed7d6b313e31f218789c
1b609f046615abb69bf5a59f1df915f46e9853bc
d78648963ed0475dcce2f87b9dc969661fcbfc06
4914efd0cc4818a7334344ce417f3ddfbaac8e2d
Comment 72 Samba QA Contact 2021-03-11 11:47:21 UTC
This bug was referenced in samba v4-12-stable (Release samba-4.12.12):

a00ff4345157f8d04b7c58325a3e877a9d1f9a95
80d2c3e4725689b9bf353c2f2356a451f0ee6711
a19f94c644d552466fd9f15f1b1d9c04d6fac0e9
bab7f2ae28e5bd0a44e121c9b7e9d868271fac70
6e6aa90b87b5ddaa6b92160eb467090705e25ff6