[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