Discussion:
[PATCHES BUG 13407] rpc_server: Fix NetSessEnum with stale sessions
Christof Schmitt via samba-technical
2018-04-25 17:21:31 UTC
Permalink
The problem here is that sessions entries in smbXsrv_global_session do
not expire. smbstatus -p filters out the stale sessions with a process
check. This is missing for NetSessEnum; adding this seems to be the
easiest fix, assuming that this does not happen often.

The other approach would be to run a clean up on the database, e.g. in
cleanupd after a process crash, but that would require traversing the
database and checking all processes.

Christof
Jeremy Allison via samba-technical
2018-04-25 17:58:20 UTC
Permalink
Post by Christof Schmitt via samba-technical
The problem here is that sessions entries in smbXsrv_global_session do
not expire. smbstatus -p filters out the stale sessions with a process
check. This is missing for NetSessEnum; adding this seems to be the
easiest fix, assuming that this does not happen often.
The other approach would be to run a clean up on the database, e.g. in
cleanupd after a process crash, but that would require traversing the
database and checking all processes.
Good (and correct IMHO) fix. Thanks Christof ! RB+ and pushed
to autobuild. Thanks a lot for the tests too.
Post by Christof Schmitt via samba-technical
From 8d936e431088805794bcce43720b81ba5b489568 Mon Sep 17 00:00:00 2001
Date: Tue, 24 Apr 2018 12:18:49 -0700
Subject: [PATCH 1/3] rpcclient: Print number of entries for NetSessEnum
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13407
---
source3/rpcclient/cmd_srvsvc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/source3/rpcclient/cmd_srvsvc.c b/source3/rpcclient/cmd_srvsvc.c
index e7929e7..f78fb9a1 100644
--- a/source3/rpcclient/cmd_srvsvc.c
+++ b/source3/rpcclient/cmd_srvsvc.c
@@ -883,6 +883,8 @@ static WERROR cmd_srvsvc_net_sess_enum(struct rpc_pipe_client *cli,
goto done;
}
+ d_printf("Received %d entries.\n", total_entries);
+
return result;
}
--
1.8.3.1
From e10b4c3a2e8e5345e3a1928a2e836986847754f7 Mon Sep 17 00:00:00 2001
Date: Tue, 24 Apr 2018 13:52:59 -0700
Subject: [PATCH 2/3] selftest: Add testcase for querying sessions after smbd
crash
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13407
---
selftest/knownfail | 1 +
source3/script/tests/test_rpcclient_netsessenum.sh | 55 ++++++++++++++++++++++
source3/selftest/tests.py | 6 +++
3 files changed, 62 insertions(+)
create mode 100755 source3/script/tests/test_rpcclient_netsessenum.sh
diff --git a/selftest/knownfail b/selftest/knownfail
index a2aeed2..44bdbd8 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -343,3 +343,4 @@
# Disabling NTLM means you can't use samr to change the password
^samba.tests.ntlmdisabled.python\(ktest\).ntlmdisabled.NtlmDisabledTests.test_samr_change_password\(ktest\)
^samba.tests.ntlmdisabled.python\(ad_dc_no_ntlm\).ntlmdisabled.NtlmDisabledTests.test_ntlm_connection\(ad_dc_no_ntlm\)
+^samba3.blackbox.rpcclient_netsessenum.count2\(ad_member\)
diff --git a/source3/script/tests/test_rpcclient_netsessenum.sh b/source3/script/tests/test_rpcclient_netsessenum.sh
new file mode 100755
index 0000000..9abb3ae
--- /dev/null
+++ b/source3/script/tests/test_rpcclient_netsessenum.sh
@@ -0,0 +1,55 @@
+#!/bin/sh
+#
+# Blackbox tests for the rpcclient srvsvc commands
+#
+# Copyright (C) 2018 Christof Schmitt
+
+if [ $# -lt 6 ]; then
+cat <<EOF
+Usage: $0 DOMAIN ADMIN_USER ADMIN_PASSWORD SERVER RPCCLIENT SMBTORTURE3 SHARE
+EOF
+exit 1;
+fi
+
+DOMAIN="$1"
+ADMIN_USER="$2"
+ADMIN_PASSWORD="$3"
+SERVER="$4"
+RPCCLIENT="$5"
+SMBTORTURE3="$6"
+SHARE="$7"
+
+USERPASS="-U$DOMAIN/$ADMIN_USER%$ADMIN_PASSWORD"
+RPCCLIENTCMD="$RPCCLIENT $SERVER $USERPASS"
+
+incdir=$(dirname $0)/../../../testprogs/blackbox
+. $incdir/subunit.sh
+
+failed=0
+
+#
+# Verify initial number of sessions.
+#
+$RPCCLIENTCMD -c NetSessEnum | grep Received
+RC=$?
+testit "netsessenum" test $RC = 0 || failed=$(expr $failed + 1)
+
+OUT=$($RPCCLIENTCMD -c NetSessEnum | grep Received)
+test "$OUT" = "Received 1 entries."
+RC=$?
+testit "count1" test $RC -eq 0 || failed=$(expr $failed + 1)
+
+#
+# Inject smbd crash
+#
+$SMBTORTURE3 //"$SERVER"/"$SHARE" "$USERPASS" CLEANUP1
+
+#
+# Verify number of sessions after crash
+#
+OUT=$($RPCCLIENTCMD -c NetSessEnum | grep Received)
+test "$OUT" = "Received 1 entries."
+RC=$?
+testit "count2" test $RC -eq 0 || failed=$(expr $failed + 1)
+
+testok $0 $failed
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index 06bda70..810cb5e 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -639,6 +639,12 @@ plantestsuite("samba3.blackbox.smbclient.encryption_off", "simpleserver",
"$USERNAME", "$PASSWORD", "$SERVER",
smbclient3])
+plantestsuite("samba3.blackbox.rpcclient_netsessenum", "ad_member",
+ [os.path.join(samba3srcdir,
+ "script/tests/test_rpcclient_netsessenum.sh"),
+ "$DOMAIN", "$DC_USERNAME", "$DC_PASSWORD", "$SERVER",
+ os.path.join(bindir(), "rpcclient"), smbtorture3, "tmp"])
+
# server min protocol = SMB3_00
# client max protocol = SMB3
--
1.8.3.1
From eab0119b464622945d0fb4130027baec495eb193 Mon Sep 17 00:00:00 2001
Date: Tue, 24 Apr 2018 13:53:41 -0700
Subject: [PATCH 3/3] rpc_server: Fix NetSessEnum with stale sessions
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13407
---
selftest/knownfail | 1 -
source3/smbd/session.c | 4 ++++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/selftest/knownfail b/selftest/knownfail
index 44bdbd8..a2aeed2 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -343,4 +343,3 @@
# Disabling NTLM means you can't use samr to change the password
^samba.tests.ntlmdisabled.python\(ktest\).ntlmdisabled.NtlmDisabledTests.test_samr_change_password\(ktest\)
^samba.tests.ntlmdisabled.python\(ad_dc_no_ntlm\).ntlmdisabled.NtlmDisabledTests.test_ntlm_connection\(ad_dc_no_ntlm\)
-^samba3.blackbox.rpcclient_netsessenum.count2\(ad_member\)
diff --git a/source3/smbd/session.c b/source3/smbd/session.c
index 4ddb856..8b4c660 100644
--- a/source3/smbd/session.c
+++ b/source3/smbd/session.c
@@ -139,6 +139,10 @@ static int gather_sessioninfo(const char *key, struct sessionid *session,
return 0;
}
+ if (!process_exists(session->pid)) {
+ return 0;
+ }
+
sesslist->sessions = talloc_realloc(
sesslist->mem_ctx, sesslist->sessions, struct sessionid,
sesslist->count+1);
--
1.8.3.1
Volker Lendecke via samba-technical
2018-04-25 19:25:46 UTC
Permalink
Post by Christof Schmitt via samba-technical
The problem here is that sessions entries in smbXsrv_global_session do
not expire. smbstatus -p filters out the stale sessions with a process
check. This is missing for NetSessEnum; adding this seems to be the
easiest fix, assuming that this does not happen often.
The other approach would be to run a clean up on the database, e.g. in
cleanupd after a process crash, but that would require traversing the
database and checking all processes.
What about sending the required info to cleanupd when you come across
it in gather_sessioninfo?

Volker
--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:***@sernet.de
Christof Schmitt via samba-technical
2018-04-25 20:19:33 UTC
Permalink
Post by Volker Lendecke via samba-technical
Post by Christof Schmitt via samba-technical
The problem here is that sessions entries in smbXsrv_global_session do
not expire. smbstatus -p filters out the stale sessions with a process
check. This is missing for NetSessEnum; adding this seems to be the
easiest fix, assuming that this does not happen often.
The other approach would be to run a clean up on the database, e.g. in
cleanupd after a process crash, but that would require traversing the
database and checking all processes.
What about sending the required info to cleanupd when you come across
it in gather_sessioninfo?
Do you mean sending a messageg to cleanupd with the key, in case the
process no longer exists? And then cleanupd will delete the entry? I can
look into adding that.

Christof
Christof Schmitt via samba-technical
2018-04-25 20:24:10 UTC
Permalink
Post by Christof Schmitt via samba-technical
Post by Volker Lendecke via samba-technical
Post by Christof Schmitt via samba-technical
The problem here is that sessions entries in smbXsrv_global_session do
not expire. smbstatus -p filters out the stale sessions with a process
check. This is missing for NetSessEnum; adding this seems to be the
easiest fix, assuming that this does not happen often.
The other approach would be to run a clean up on the database, e.g. in
cleanupd after a process crash, but that would require traversing the
database and checking all processes.
What about sending the required info to cleanupd when you come across
it in gather_sessioninfo?
Do you mean sending a messageg to cleanupd with the key, in case the
process no longer exists? And then cleanupd will delete the entry? I can
look into adding that.
Or storing the key in the cleanupd tdb...

Christof
Christof Schmitt via samba-technical
2018-04-27 21:38:25 UTC
Permalink
Post by Christof Schmitt via samba-technical
Post by Christof Schmitt via samba-technical
Post by Volker Lendecke via samba-technical
Post by Christof Schmitt via samba-technical
The problem here is that sessions entries in smbXsrv_global_session do
not expire. smbstatus -p filters out the stale sessions with a process
check. This is missing for NetSessEnum; adding this seems to be the
easiest fix, assuming that this does not happen often.
The other approach would be to run a clean up on the database, e.g. in
cleanupd after a process crash, but that would require traversing the
database and checking all processes.
What about sending the required info to cleanupd when you come across
it in gather_sessioninfo?
Do you mean sending a messageg to cleanupd with the key, in case the
process no longer exists? And then cleanupd will delete the entry? I can
look into adding that.
Or storing the key in the cleanupd tdb...
First, i think it would make sense to treat this as a separate change.
The patches to ignore the stale entries in the database are already in
master and on the way to the stable branches, and they fix the data
returned from NetSessEnum which is important.

I started looking into this, and storing the keys in the cleanup
database and this is a bit more involved. My current stumbling block is
how to send a message to cleanupd after finding stale entries in
list_sessions. Maybe that has to go through the parent smbd, as only the
parent has a reference to the cleanupd process.

On the other hand, a simpler implementation would be to traverse the
database in list_sessions/gather_sessioninfo and keep a local list of
the stale sessions. Then as a final step in the list process, delete the
stale entries. The downside is that this would delay the enumeration, in
case many entries need to be deleted; that would be a reason to defer
this to cleanupd.

Christof

Loading...