[PATCH] Messaging improvements and fixes needed for auth logging

Jeremy Allison jra at samba.org
Thu Mar 23 19:26:40 UTC 2017


On Thu, Mar 23, 2017 at 11:58:06AM -0700, Jeremy Allison via samba-technical wrote:
> On Fri, Mar 24, 2017 at 07:52:46AM +1300, Andrew Bartlett wrote:
> > > Sorry, thought it was a pretty obvious fix. The
> > > tests on top should make sure we don't regress here.
> > 
> > While entirely reasonable to split up, and we can differ on the API, I
> > think we should keep the logic changes in the original patch. 
> > 
> > Otherwise I don't see how we delete names once the last user is de-
> > registered.
> 
> Yeah, you're probably right, sorry. I'll restructure this so your
> tests go in first + the knownfail, then the fix, then
> the -knownfail. Once all the make tests pass I'll repost
> for review. That should make sure we don't regress.

And here it is ! Contents are:

Patches 1-3 - Additional tests from Andrew showing the bug + knownfail. RB+ me.
Patch 4 - Fix for the bug from both Volker + Andrew - knownfail entry.

As patch #4 contains elements of both Volker's and Andrew's patches I've
marked it:

Signed-off-by: Andrew Bartlett <abartlet at samba.org>
Signed-off-by: Volker Lendecke <vl at samba.org>

and rb+ me.

With this we pass the new test demonstrating the bug.

Sorry for the long process, but we all got there in the
end :-).

Please review and push if happy !

Jeremy.
-------------- next part --------------
>From 4e39d7864c71bb3e992bf879a5bf574b3f8ac8b3 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Wed, 8 Mar 2017 14:53:26 +1300
Subject: [PATCH 1/4] pymessaging: Add support for irpc_add_name

Signed-off-by: Andrew Bartlett <abartlet at samba.org>
Pair-Programmed-by: Gary Lockyer <gary at catalyst.net.nz>
Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
Reviewed-by: Jeremy Allison <jra at samba.org>
---
 python/samba/tests/messaging.py     |  4 ++++
 source4/lib/messaging/pymessaging.c | 23 ++++++++++++++++++++++-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/python/samba/tests/messaging.py b/python/samba/tests/messaging.py
index 5d32d60..1c5dfe5 100644
--- a/python/samba/tests/messaging.py
+++ b/python/samba/tests/messaging.py
@@ -49,6 +49,10 @@ class MessagingTests(TestCase):
         x = self.get_context()
         self.assertTrue(isinstance(x.server_id, server_id))
 
+    def test_add_name(self):
+        x = self.get_context()
+        x.irpc_add_name("samba.messaging test")
+
     def test_ping_speed(self):
         server_ctx = self.get_context((0, 1))
         def ping_callback(src, data):
diff --git a/source4/lib/messaging/pymessaging.c b/source4/lib/messaging/pymessaging.c
index f62354b..9d0997f 100644
--- a/source4/lib/messaging/pymessaging.c
+++ b/source4/lib/messaging/pymessaging.c
@@ -241,6 +241,25 @@ static PyObject *py_imessaging_deregister(PyObject *self, PyObject *args, PyObje
 	Py_RETURN_NONE;
 }
 
+static PyObject *py_irpc_add_name(PyObject *self, PyObject *args, PyObject *kwargs)
+{
+	imessaging_Object *iface = (imessaging_Object *)self;
+	char *server_name;
+	NTSTATUS status;
+
+	if (!PyArg_ParseTuple(args, "s", &server_name)) {
+		return NULL;
+	}
+
+	status = irpc_add_name(iface->msg_ctx, server_name);
+	if (!NT_STATUS_IS_OK(status)) {
+		PyErr_SetNTSTATUS(status);
+		return NULL;
+	}
+
+	Py_RETURN_NONE;
+}
+
 static PyObject *py_irpc_servers_byname(PyObject *self, PyObject *args, PyObject *kwargs)
 {
 	imessaging_Object *iface = (imessaging_Object *)self;
@@ -341,10 +360,12 @@ static PyMethodDef py_imessaging_methods[] = {
 		"S.register(callback, msg_type=None) -> msg_type\nRegister a message handler" },
 	{ "deregister", (PyCFunction)py_imessaging_deregister, METH_VARARGS|METH_KEYWORDS,
 		"S.deregister(callback, msg_type) -> None\nDeregister a message handler" },
+	{ "irpc_add_name", (PyCFunction)py_irpc_add_name, METH_VARARGS,
+		"S.irpc_add_name(name) -> None\nAdd this context to the list of server_id values that are registered for a particular name" },
 	{ "irpc_servers_byname", (PyCFunction)py_irpc_servers_byname, METH_VARARGS,
 		"S.irpc_servers_byname(name) -> list\nGet list of server_id values that are registered for a particular name" },
 	{ "irpc_all_servers", (PyCFunction)py_irpc_all_servers, METH_NOARGS,
-		"S.irpc_servers_byname() -> list\nGet list of all registered names and the associated server_id values" },
+		"S.irpc_all_servers() -> list\nGet list of all registered names and the associated server_id values" },
 	{ NULL, NULL, 0, NULL }
 };
 
-- 
2.7.4


>From eeb1d057953487bea9dd0cfb9f79dba2ae477ee1 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Tue, 14 Mar 2017 13:39:00 +1300
Subject: [PATCH 2/4] pymessaging: Add irpc_remove_name

Signed-off-by: Andrew Bartlett <abartlet at samba.org>
Reviewed-by: Jeremy Allison <jra at samba.org>
---
 python/samba/tests/messaging.py     |  8 ++++++++
 source4/lib/messaging/pymessaging.c | 16 ++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/python/samba/tests/messaging.py b/python/samba/tests/messaging.py
index 1c5dfe5..3eeab52 100644
--- a/python/samba/tests/messaging.py
+++ b/python/samba/tests/messaging.py
@@ -22,6 +22,7 @@ import samba
 from samba.messaging import Messaging
 from samba.tests import TestCase
 from samba.dcerpc.server_id import server_id
+from samba.ndr import ndr_print
 
 class MessagingTests(TestCase):
 
@@ -52,6 +53,13 @@ class MessagingTests(TestCase):
     def test_add_name(self):
         x = self.get_context()
         x.irpc_add_name("samba.messaging test")
+        name_list = x.irpc_servers_byname("samba.messaging test")
+        self.assertEqual(len(name_list), 1)
+        self.assertEqual(ndr_print(x.server_id),
+                         ndr_print(name_list[0]))
+        x.irpc_remove_name("samba.messaging test")
+        self.assertEqual([],
+                         x.irpc_servers_byname("samba.messaging test"))
 
     def test_ping_speed(self):
         server_ctx = self.get_context((0, 1))
diff --git a/source4/lib/messaging/pymessaging.c b/source4/lib/messaging/pymessaging.c
index 9d0997f..b317955 100644
--- a/source4/lib/messaging/pymessaging.c
+++ b/source4/lib/messaging/pymessaging.c
@@ -260,6 +260,20 @@ static PyObject *py_irpc_add_name(PyObject *self, PyObject *args, PyObject *kwar
 	Py_RETURN_NONE;
 }
 
+static PyObject *py_irpc_remove_name(PyObject *self, PyObject *args, PyObject *kwargs)
+{
+	imessaging_Object *iface = (imessaging_Object *)self;
+	char *server_name;
+
+	if (!PyArg_ParseTuple(args, "s", &server_name)) {
+		return NULL;
+	}
+
+	irpc_remove_name(iface->msg_ctx, server_name);
+
+	Py_RETURN_NONE;
+}
+
 static PyObject *py_irpc_servers_byname(PyObject *self, PyObject *args, PyObject *kwargs)
 {
 	imessaging_Object *iface = (imessaging_Object *)self;
@@ -362,6 +376,8 @@ static PyMethodDef py_imessaging_methods[] = {
 		"S.deregister(callback, msg_type) -> None\nDeregister a message handler" },
 	{ "irpc_add_name", (PyCFunction)py_irpc_add_name, METH_VARARGS,
 		"S.irpc_add_name(name) -> None\nAdd this context to the list of server_id values that are registered for a particular name" },
+	{ "irpc_remove_name", (PyCFunction)py_irpc_remove_name, METH_VARARGS,
+		"S.irpc_remove_name(name) -> None\nAdd this context to the list of server_id values that are registered for a particular name" },
 	{ "irpc_servers_byname", (PyCFunction)py_irpc_servers_byname, METH_VARARGS,
 		"S.irpc_servers_byname(name) -> list\nGet list of server_id values that are registered for a particular name" },
 	{ "irpc_all_servers", (PyCFunction)py_irpc_all_servers, METH_NOARGS,
-- 
2.7.4


>From 064371fb1a98a16f976169f377768b1f01e44e9c Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Tue, 14 Mar 2017 16:07:46 +1300
Subject: [PATCH 3/4] selftest: Test server_id database add and removal

Signed-off-by: Andrew Bartlett <abartlet at samba.org>
Reviewed-by: Jeremy Allison <jra at samba.org>
---
 python/samba/tests/messaging.py | 19 +++++++++++++------
 selftest/knownfail              |  1 +
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/python/samba/tests/messaging.py b/python/samba/tests/messaging.py
index 3eeab52..a70be96 100644
--- a/python/samba/tests/messaging.py
+++ b/python/samba/tests/messaging.py
@@ -23,6 +23,7 @@ from samba.messaging import Messaging
 from samba.tests import TestCase
 from samba.dcerpc.server_id import server_id
 from samba.ndr import ndr_print
+import random
 
 class MessagingTests(TestCase):
 
@@ -46,20 +47,26 @@ class MessagingTests(TestCase):
         for name in x.irpc_all_servers():
             self.assertTrue(isinstance(x.irpc_servers_byname(name.name), list))
 
+    def test_unknown_name(self):
+        x = self.get_context()
+        self.assertRaises(KeyError,
+                          x.irpc_servers_byname, "samba.messaging test NONEXISTING")
+
     def test_assign_server_id(self):
         x = self.get_context()
         self.assertTrue(isinstance(x.server_id, server_id))
 
-    def test_add_name(self):
+    def test_add_remove_name(self):
         x = self.get_context()
-        x.irpc_add_name("samba.messaging test")
-        name_list = x.irpc_servers_byname("samba.messaging test")
+        name = "samba.messaging test-%d" % random.randint(1, 1000000)
+        x.irpc_add_name(name)
+        name_list = x.irpc_servers_byname(name)
         self.assertEqual(len(name_list), 1)
         self.assertEqual(ndr_print(x.server_id),
                          ndr_print(name_list[0]))
-        x.irpc_remove_name("samba.messaging test")
-        self.assertEqual([],
-                         x.irpc_servers_byname("samba.messaging test"))
+        x.irpc_remove_name(name)
+        self.assertRaises(KeyError,
+                          x.irpc_servers_byname, name)
 
     def test_ping_speed(self):
         server_ctx = self.get_context((0, 1))
diff --git a/selftest/knownfail b/selftest/knownfail
index cfd4b35..2f3b22b 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -317,3 +317,4 @@
 ^samba3.smb2.credits.skipped_mid.*
 ^samba4.blackbox.dbcheck-links.release-4-5-0-pre1.dangling_multi_valued_dbcheck
 ^samba4.blackbox.dbcheck-links.release-4-5-0-pre1.dangling_multi_valued_check_missing
+^samba.tests.messaging.samba.tests.messaging.MessagingTests.test_add_remove_name
\ No newline at end of file
-- 
2.7.4


>From d2ca4e3e43d791341f9f94ae8d26912d19d01fd1 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Thu, 23 Mar 2017 15:48:25 +0100
Subject: [PATCH 4/4] server_id_db: Protect against non-0-terminated data
 records

Remove the failing test from knownfail.

Signed-off-by: Andrew Bartlett <abartlet at samba.org>
Signed-off-by: Volker Lendecke <vl at samba.org>
Reviewed-by: Jeremy Allison <jra at samba.org>
---
 lib/util/server_id_db.c | 22 +++++++++++++++++++++-
 selftest/knownfail      |  1 -
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/lib/util/server_id_db.c b/lib/util/server_id_db.c
index e0b8476..e190f45 100644
--- a/lib/util/server_id_db.c
+++ b/lib/util/server_id_db.c
@@ -138,6 +138,7 @@ int server_id_db_prune_name(struct server_id_db *db, const char *name,
 	char idbuf[idbuf_len];
 	TDB_DATA key;
 	uint8_t *data;
+	size_t datalen;
 	char *ids, *id;
 	int ret;
 
@@ -156,6 +157,13 @@ int server_id_db_prune_name(struct server_id_db *db, const char *name,
 		return ret;
 	}
 
+	datalen = talloc_get_size(data);
+	if ((datalen == 0) || (data[datalen-1] != '\0')) {
+		tdb_chainunlock(tdb, key);
+		TALLOC_FREE(data);
+		return EINVAL;
+	}
+
 	ids = (char *)data;
 
 	id = strv_find(ids, idbuf);
@@ -166,7 +174,12 @@ int server_id_db_prune_name(struct server_id_db *db, const char *name,
 	}
 
 	strv_delete(&ids, id);
-	ret = tdb_store(tdb, key, talloc_tdb_data(ids), TDB_MODIFY);
+
+	if (talloc_get_size(ids) == 0) {
+		ret = tdb_delete(tdb, key);
+	} else {
+		ret = tdb_store(tdb, key, talloc_tdb_data(ids), TDB_MODIFY);
+	}
 	TALLOC_FREE(data);
 
 	tdb_chainunlock(tdb, key);
@@ -200,6 +213,7 @@ int server_id_db_lookup(struct server_id_db *db, const char *name,
 	struct tdb_context *tdb = db->tdb->tdb;
 	TDB_DATA key;
 	uint8_t *data;
+	size_t datalen;
 	char *ids, *id;
 	unsigned num_servers;
 	struct server_id *servers;
@@ -212,6 +226,12 @@ int server_id_db_lookup(struct server_id_db *db, const char *name,
 		return ret;
 	}
 
+	datalen = talloc_get_size(data);
+	if ((datalen == 0) || (data[datalen-1] != '\0')) {
+		TALLOC_FREE(data);
+		return EINVAL;
+	}
+
 	ids = (char *)data;
 	num_servers = strv_count(ids);
 
diff --git a/selftest/knownfail b/selftest/knownfail
index 2f3b22b..cfd4b35 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -317,4 +317,3 @@
 ^samba3.smb2.credits.skipped_mid.*
 ^samba4.blackbox.dbcheck-links.release-4-5-0-pre1.dangling_multi_valued_dbcheck
 ^samba4.blackbox.dbcheck-links.release-4-5-0-pre1.dangling_multi_valued_check_missing
-^samba.tests.messaging.samba.tests.messaging.MessagingTests.test_add_remove_name
\ No newline at end of file
-- 
2.7.4



More information about the samba-technical mailing list