[PATCH] Messaging improvements and fixes needed for auth logging

classic Classic list List threaded Threaded
26 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCH] Messaging improvements and fixes needed for auth logging

Samba - samba-technical mailing list
To test the auth and authz logging, we decided to use the messaging
layer (details in our auth-logging branch in Catalyst's repo), as we
have bindings for that in python.

It turned out that there were a number of issues in these layers, both
the incomplete python bindings (they never previously worked: the tests
were not actually tests) and an issue in the serverid database due to
the way tdb_fetch_talloc() behaved with zero-length records.

The attached patches addresses both issues and allows us to build a
robust testsuite for authentication and authorization logging!

Please review!

Andrew Bartlett
--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba

auth-log-messaging.patch (40K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Messaging improvements and fixes needed for auth logging

Samba - samba-technical mailing list
On Mon, Mar 20, 2017 at 08:21:13PM +1300, Andrew Bartlett via samba-technical wrote:

> To test the auth and authz logging, we decided to use the messaging
> layer (details in our auth-logging branch in Catalyst's repo), as we
> have bindings for that in python.
>
> It turned out that there were a number of issues in these layers, both
> the incomplete python bindings (they never previously worked: the tests
> were not actually tests) and an issue in the serverid database due to
> the way tdb_fetch_talloc() behaved with zero-length records.
>
> The attached patches addresses both issues and allows us to build a
> robust testsuite for authentication and authorization logging!
>
> Please review!

Looking...

Volker

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Messaging improvements and fixes needed for auth logging

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Mon, Mar 20, 2017 at 08:21:13PM +1300, Andrew Bartlett via samba-technical wrote:

> To test the auth and authz logging, we decided to use the messaging
> layer (details in our auth-logging branch in Catalyst's repo), as we
> have bindings for that in python.
>
> It turned out that there were a number of issues in these layers, both
> the incomplete python bindings (they never previously worked: the tests
> were not actually tests) and an issue in the serverid database due to
> the way tdb_fetch_talloc() behaved with zero-length records.
>
> The attached patches addresses both issues and allows us to build a
> robust testsuite for authentication and authorization logging!
>
> Please review!

Standard initial comment: Please keep below 80 columns per line. Do
you need me to do that, or will you be able to do it yourself?

Thanks, Volker

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Messaging improvements and fixes needed for auth logging

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Mon, Mar 20, 2017 at 08:21:13PM +1300, Andrew Bartlett via samba-technical wrote:
> From eef04949817bab65aa3c3268cf8690e7753e174e Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <[hidden email]>
> Date: Tue, 14 Mar 2017 15:22:01 +1300
> Subject: [PATCH 4/7] lib/util: Do not return an unterminated pointer in
>  tdb_fetch_talloc()
>
> Otherwise, if a TDB entry is truncated to 0 length, this will return
> uninitialised memory!

Can you explain a bit more what is going on here? I would like to
avoid DATA_BLOB and/or TDB_DATA where it makes sense. Here we always
return a talloc'ed object that carries its own length. I think that a
talloc objects is just as expressive as a DATA_BLOB, you can always
query its length with talloc_get_size.

I would like to understand the bug that this fixes that is not fixable
with keeping just the uint8_t* return from tdb_fetch_talloc().

Thanks,

Volker

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Messaging improvements and fixes needed for auth logging

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
This test condition is odd.
Add a comment explaining why && dsize != 0?

On 3/20/17 03:21, Andrew Bartlett via samba-technical wrote:
> @@ -509,7 +510,7 @@ int tdb_fetch_talloc(struct tdb_context *tdb, TDB_DATA key,
>   return map_unix_error_from_tdb(err);
>   }
>  
> - if (state.buf == NULL) {
> + if (state.buf.dptr == NULL && state.buf.dsize != 0) {
>   return ENOMEM;
>   }


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Messaging improvements and fixes needed for auth logging

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Mon, 2017-03-20 at 11:06 +0100, Volker Lendecke wrote:

> On Mon, Mar 20, 2017 at 08:21:13PM +1300, Andrew Bartlett via samba-
> technical wrote:
> > From eef04949817bab65aa3c3268cf8690e7753e174e Mon Sep 17 00:00:00
> > 2001
> > From: Andrew Bartlett <[hidden email]>
> > Date: Tue, 14 Mar 2017 15:22:01 +1300
> > Subject: [PATCH 4/7] lib/util: Do not return an unterminated
> > pointer in
> >  tdb_fetch_talloc()
> >
> > Otherwise, if a TDB entry is truncated to 0 length, this will
> > return
> > uninitialised memory!
>
> Can you explain a bit more what is going on here?

Certainly.  Have you run the test I've added?  (make test
TESTS=messaging triggers it nicely).

>  I would like to
> avoid DATA_BLOB and/or TDB_DATA where it makes sense. Here we always
> return a talloc'ed object that carries its own length. I think that a
> talloc objects is just as expressive as a DATA_BLOB, you can always
> query its length with talloc_get_size.
>
> I would like to understand the bug that this fixes that is not
> fixable
> with keeping just the uint8_t* return from tdb_fetch_talloc().

The callers otherwise assumed it was a NULL terminated string, and
wandered off the end of the string.  

I understand your passion of talloc_get_size(), and it certainly can be
very useful in inline code and loops, but I don't think that idiom is
as reasonable for for return values.

My view is that in Samba, byte buffers should always be encoded as a
DATA_BLOB, struct ldb_val or TDB_DATA.  This makes it clear to the
caller that before passing to further downstream users (such as
strv_count() taking a const char *) that a check for or addition of
termination is required.

That is, we always look suspiciously at code like:
        ids = (char *)data.dptr;
while the original
        ids = (char *)data;
triggers much less suspicion.

I hope this makes my analysis clear.

Thanks,

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Messaging improvements and fixes needed for auth logging

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Mon, 2017-03-20 at 09:05 -0400, Jim Brown via samba-technical wrote:

> This test condition is odd.
> Add a comment explaining why && dsize != 0?
>
> On 3/20/17 03:21, Andrew Bartlett via samba-technical wrote:
> > @@ -509,7 +510,7 @@ int tdb_fetch_talloc(struct tdb_context *tdb,
> > TDB_DATA key,
> >    return map_unix_error_from_tdb(err);
> >    }
> >   
> > - if (state.buf == NULL) {
> > + if (state.buf.dptr == NULL && state.buf.dsize != 0) {
> >    return ENOMEM;
> >    }

Thanks.  That is me forgetting that talloc(ctx, 0) returns a pointer,
rather than returning NULL, so I don't need that exception.

I'll fix it up!

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Messaging improvements and fixes needed for auth logging

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, Mar 21, 2017 at 09:02:10AM +1300, Andrew Bartlett wrote:
> > Can you explain a bit more what is going on here?
>
> Certainly.  Have you run the test I've added?  (make test
> TESTS=messaging triggers it nicely).

No, I haven't run the test. Can you explain in English words what the
bug is please?

> >  I would like to
> > avoid DATA_BLOB and/or TDB_DATA where it makes sense. Here we always
> > return a talloc'ed object that carries its own length. I think that a
> > talloc objects is just as expressive as a DATA_BLOB, you can always
> > query its length with talloc_get_size.
> >
> > I would like to understand the bug that this fixes that is not
> > fixable
> > with keeping just the uint8_t* return from tdb_fetch_talloc().
>
> The callers otherwise assumed it was a NULL terminated string, and
> wandered off the end of the string.  

I could understand that if talloc_get_size returned a char*. But it's a
uint8_t* that is returned. There I would assume the caller to be smart
and understand this is basically a blob.

Volker

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Messaging improvements and fixes needed for auth logging

Samba - samba-technical mailing list
On Mon, Mar 20, 2017 at 09:52:04PM +0100, Volker Lendecke via samba-technical wrote:

> On Tue, Mar 21, 2017 at 09:02:10AM +1300, Andrew Bartlett wrote:
> > > Can you explain a bit more what is going on here?
> >
> > Certainly.  Have you run the test I've added?  (make test
> > TESTS=messaging triggers it nicely).
>
> No, I haven't run the test. Can you explain in English words what the
> bug is please?
>
> > >  I would like to
> > > avoid DATA_BLOB and/or TDB_DATA where it makes sense. Here we always
> > > return a talloc'ed object that carries its own length. I think that a
> > > talloc objects is just as expressive as a DATA_BLOB, you can always
> > > query its length with talloc_get_size.
> > >
> > > I would like to understand the bug that this fixes that is not
> > > fixable
> > > with keeping just the uint8_t* return from tdb_fetch_talloc().
> >
> > The callers otherwise assumed it was a NULL terminated string, and
> > wandered off the end of the string.  
>
> I could understand that if talloc_get_size returned a char*. But it's a
                             ^^^^^^^^^^^^^^^
                             tdb_fetch_talloc of course :-)

tdb_fetch_talloc returns a uint8_t*, so a blob.

Volker

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Messaging improvements and fixes needed for auth logging

Samba - samba-technical mailing list
On Mon, Mar 20, 2017 at 09:55:37PM +0100, Andrew Bartlett via samba-technical wrote:

> On Mon, Mar 20, 2017 at 09:52:04PM +0100, Volker Lendecke via samba-technical wrote:
> > On Tue, Mar 21, 2017 at 09:02:10AM +1300, Andrew Bartlett wrote:
> > > > Can you explain a bit more what is going on here?
> > >
> > > Certainly.  Have you run the test I've added?  (make test
> > > TESTS=messaging triggers it nicely).
> >
> > No, I haven't run the test. Can you explain in English words what the
> > bug is please?
> >
> > > >  I would like to
> > > > avoid DATA_BLOB and/or TDB_DATA where it makes sense. Here we always
> > > > return a talloc'ed object that carries its own length. I think that a
> > > > talloc objects is just as expressive as a DATA_BLOB, you can always
> > > > query its length with talloc_get_size.
> > > >
> > > > I would like to understand the bug that this fixes that is not
> > > > fixable
> > > > with keeping just the uint8_t* return from tdb_fetch_talloc().
> > >
> > > The callers otherwise assumed it was a NULL terminated string, and
> > > wandered off the end of the string.  
> >
> > I could understand that if talloc_get_size returned a char*. But it's a
>                              ^^^^^^^^^^^^^^^
>                              tdb_fetch_talloc of course :-)
>
> tdb_fetch_talloc returns a uint8_t*, so a blob.

Unbelievable, I fooled with a wrong "From" line -- sorry for that. How
did I do that? :-)

Volker

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Messaging improvements and fixes needed for auth logging

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Mon, Mar 20, 2017 at 08:21:13PM +1300, Andrew Bartlett via samba-technical wrote:

> To test the auth and authz logging, we decided to use the messaging
> layer (details in our auth-logging branch in Catalyst's repo), as we
> have bindings for that in python.
>
> It turned out that there were a number of issues in these layers, both
> the incomplete python bindings (they never previously worked: the tests
> were not actually tests) and an issue in the serverid database due to
> the way tdb_fetch_talloc() behaved with zero-length records.
>
> The attached patches addresses both issues and allows us to build a
> robust testsuite for authentication and authorization logging!
>
> Please review!

Can you split this into separate patchsets for the serverid database
problem and the incomplete python problem ? It's a little confusing
having them mixed together like this.

In addition I'm assuming we'll need a bug logged for the serverid database
bug, and the commit messages for that fix should be tagged with the
BUG: https://.... url.

Thanks for finding and fixing this !

Cheers,

        Jeremy.

> --
> Andrew Bartlett                       http://samba.org/~abartlet/
> Authentication Developer, Samba Team  http://samba.org
> Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba

> From 48ec03952ab0663257a846838272ba213683ee6a Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <[hidden email]>
> Date: Wed, 8 Mar 2017 14:53:26 +1300
> Subject: [PATCH 1/7] pymessaging: Add support for irpc_add_name
>
> Signed-off-by: Andrew Bartlett <[hidden email]>
> Pair-Programmed-by: Gary Lockyer <[hidden email]>
> Signed-off-by: Gary Lockyer <[hidden email]>
> ---
>  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.9.3
>
>
> From 461fc025fba11d3ff6b723b8aa421c7d94083ce7 Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <[hidden email]>
> Date: Tue, 14 Mar 2017 13:39:00 +1300
> Subject: [PATCH 2/7] pymessaging: Add irpc_remove_name
>
> Signed-off-by: Andrew Bartlett <[hidden email]>
> ---
>  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.9.3
>
>
> From c540d344b987f688c8fbdcbef87c85b0c8cd525f Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <[hidden email]>
> Date: Tue, 14 Mar 2017 16:07:46 +1300
> Subject: [PATCH 3/7] selftest: Test server_id database add and removal
>
> Signed-off-by: Andrew Bartlett <[hidden email]>
> ---
>  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.9.3
>
>
> From eef04949817bab65aa3c3268cf8690e7753e174e Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <[hidden email]>
> Date: Tue, 14 Mar 2017 15:22:01 +1300
> Subject: [PATCH 4/7] lib/util: Do not return an unterminated pointer in
>  tdb_fetch_talloc()
>
> Otherwise, if a TDB entry is truncated to 0 length, this will return
> uninitialised memory!
>
> Signed-off-by: Andrew Bartlett <[hidden email]>
> ---
>  lib/util/server_id_db.c | 42 +++++++++++++++++++++++++++++++++---------
>  lib/util/util_tdb.c     |  9 +++++----
>  lib/util/util_tdb.h     |  2 +-
>  selftest/knownfail      |  1 -
>  4 files changed, 39 insertions(+), 15 deletions(-)
>
> diff --git a/lib/util/server_id_db.c b/lib/util/server_id_db.c
> index e0b8476..937de89 100644
> --- a/lib/util/server_id_db.c
> +++ b/lib/util/server_id_db.c
> @@ -137,7 +137,8 @@ int server_id_db_prune_name(struct server_id_db *db, const char *name,
>   size_t idbuf_len = server_id_str_buf_unique(server, NULL, 0);
>   char idbuf[idbuf_len];
>   TDB_DATA key;
> - uint8_t *data;
> + TDB_DATA data;
> + TDB_DATA data_store;
>   char *ids, *id;
>   int ret;
>  
> @@ -156,18 +157,32 @@ int server_id_db_prune_name(struct server_id_db *db, const char *name,
>   return ret;
>   }
>  
> - ids = (char *)data;
> + if (data.dsize == 0) {
> + tdb_chainunlock(tdb, key);
> + TALLOC_FREE(data.dptr);
> + return ENOENT;
> + }
> +
> + /* We assert that the DB contains a NULL-terminated string */
> + ids = (char *)data.dptr;
>  
>   id = strv_find(ids, idbuf);
>   if (id == NULL) {
>   tdb_chainunlock(tdb, key);
> - TALLOC_FREE(data);
> + TALLOC_FREE(data.dptr);
>   return ENOENT;
>   }
>  
>   strv_delete(&ids, id);
> - ret = tdb_store(tdb, key, talloc_tdb_data(ids), TDB_MODIFY);
> - TALLOC_FREE(data);
> +
> + data_store = talloc_tdb_data(ids);
> +
> + if (data_store.dsize == 0) {
> + ret = tdb_delete(tdb, key);
> + } else {
> + ret = tdb_store(tdb, key, data_store, TDB_MODIFY);
> + }
> + TALLOC_FREE(data.dptr);
>  
>   tdb_chainunlock(tdb, key);
>  
> @@ -199,7 +214,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;
> + TDB_DATA data;
>   char *ids, *id;
>   unsigned num_servers;
>   struct server_id *servers;
> @@ -212,12 +227,17 @@ int server_id_db_lookup(struct server_id_db *db, const char *name,
>   return ret;
>   }
>  
> - ids = (char *)data;
> + if (data.dsize == 0) {
> + return ENOENT;
> + }
> +
> + /* We assert that the DB contains a NULL-terminated string */
> + ids = (char *)data.dptr;
>   num_servers = strv_count(ids);
>  
>   servers = talloc_array(mem_ctx, struct server_id, num_servers);
>   if (servers == NULL) {
> - TALLOC_FREE(data);
> + TALLOC_FREE(data.dptr);
>   return ENOMEM;
>   }
>  
> @@ -227,7 +247,7 @@ int server_id_db_lookup(struct server_id_db *db, const char *name,
>   servers[i++] = server_id_from_string(NONCLUSTER_VNN, id);
>   }
>  
> - TALLOC_FREE(data);
> + TALLOC_FREE(data.dptr);
>  
>   *pnum_servers = num_servers;
>   *pservers = servers;
> @@ -283,6 +303,10 @@ static int server_id_db_traverse_fn(struct tdb_context *tdb,
>   }
>   name = (const char *)key.dptr;
>  
> + if (data.dsize == 0) {
> + return 0;
> + }
> +
>   ids = (char *)talloc_memdup(state->mem_ctx, data.dptr, data.dsize);
>   if (ids == NULL) {
>   return 0;
> diff --git a/lib/util/util_tdb.c b/lib/util/util_tdb.c
> index 9bf18dc..8ff89ff 100644
> --- a/lib/util/util_tdb.c
> +++ b/lib/util/util_tdb.c
> @@ -486,19 +486,20 @@ int map_unix_error_from_tdb(enum TDB_ERROR err)
>  
>  struct tdb_fetch_talloc_state {
>   TALLOC_CTX *mem_ctx;
> - uint8_t *buf;
> + TDB_DATA buf;
>  };
>  
>  static int tdb_fetch_talloc_parser(TDB_DATA key, TDB_DATA data,
>                                     void *private_data)
>  {
>   struct tdb_fetch_talloc_state *state = private_data;
> - state->buf = talloc_memdup(state->mem_ctx, data.dptr, data.dsize);
> + state->buf.dptr = talloc_memdup(state->mem_ctx, data.dptr, data.dsize);
> + state->buf.dsize = data.dsize;
>   return 0;
>  }
>  
>  int tdb_fetch_talloc(struct tdb_context *tdb, TDB_DATA key,
> -     TALLOC_CTX *mem_ctx, uint8_t **buf)
> +     TALLOC_CTX *mem_ctx, TDB_DATA *buf)
>  {
>   struct tdb_fetch_talloc_state state = { .mem_ctx = mem_ctx };
>   int ret;
> @@ -509,7 +510,7 @@ int tdb_fetch_talloc(struct tdb_context *tdb, TDB_DATA key,
>   return map_unix_error_from_tdb(err);
>   }
>  
> - if (state.buf == NULL) {
> + if (state.buf.dptr == NULL && state.buf.dsize != 0) {
>   return ENOMEM;
>   }
>  
> diff --git a/lib/util/util_tdb.h b/lib/util/util_tdb.h
> index 3b50789..1f56341 100644
> --- a/lib/util/util_tdb.h
> +++ b/lib/util/util_tdb.h
> @@ -146,6 +146,6 @@ NTSTATUS map_nt_error_from_tdb(enum TDB_ERROR err);
>  int map_unix_error_from_tdb(enum TDB_ERROR err);
>  
>  int tdb_fetch_talloc(struct tdb_context *tdb, TDB_DATA key,
> -     TALLOC_CTX *mem_ctx, uint8_t **buf);
> +     TALLOC_CTX *mem_ctx, TDB_DATA *buf);
>  
>  #endif /* _____LIB_UTIL_UTIL_TDB_H__ */
> 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.9.3
>
>
> From fd22a3fdbd6e3561f76dbad05730c78f7500a135 Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <[hidden email]>
> Date: Mon, 20 Mar 2017 14:57:41 +1300
> Subject: [PATCH 5/7] server_id: Add runtime check for null termination
>
> Signed-off-by: Andrew Bartlett <[hidden email]>
> ---
>  lib/util/server_id_db.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/lib/util/server_id_db.c b/lib/util/server_id_db.c
> index 937de89..8aeda98 100644
> --- a/lib/util/server_id_db.c
> +++ b/lib/util/server_id_db.c
> @@ -164,6 +164,10 @@ int server_id_db_prune_name(struct server_id_db *db, const char *name,
>   }
>  
>   /* We assert that the DB contains a NULL-terminated string */
> + if (data.dptr[data.dsize - 1] != '\0') {
> + return EINVAL;
> + }
> +
>   ids = (char *)data.dptr;
>  
>   id = strv_find(ids, idbuf);
> @@ -232,6 +236,10 @@ int server_id_db_lookup(struct server_id_db *db, const char *name,
>   }
>  
>   /* We assert that the DB contains a NULL-terminated string */
> + if (data.dptr[data.dsize - 1] != '\0') {
> + return EINVAL;
> + }
> +
>   ids = (char *)data.dptr;
>   num_servers = strv_count(ids);
>  
> --
> 2.9.3
>
>
> From a78ec79a012c4b0e8face1efa4b043180b26e001 Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <[hidden email]>
> Date: Tue, 14 Mar 2017 12:39:13 +1300
> Subject: [PATCH 6/7] pymessaging: Add a hook to run the event loop, make
>  callbacks practical
>
> These change allow us to write a messaging server in python.
>
> The previous ping_speed test did not actually test anything, so
> we use .loop_once() to make it actually work.  To enable practial use
> a context is supplied in the tuple with the callback, and the server_id
> for the reply is not placed inside an additional tuple.
>
> In order to get at the internal event context on which to loop, we
> expose imessaging_context in messaging_internal.h and allow the python
> bindings to use that header.
>
> Signed-off-by: Andrew Bartlett <[hidden email]>
> ---
>  python/samba/tests/messaging.py            | 52 +++++++++++++------
>  source4/lib/messaging/messaging.c          | 17 +------
>  source4/lib/messaging/messaging_internal.h | 36 ++++++++++++++
>  source4/lib/messaging/pymessaging.c        | 80 +++++++++++++++++++++++++++---
>  4 files changed, 147 insertions(+), 38 deletions(-)
>  create mode 100644 source4/lib/messaging/messaging_internal.h
>
> diff --git a/python/samba/tests/messaging.py b/python/samba/tests/messaging.py
> index a70be96..6ee18e7 100644
> --- a/python/samba/tests/messaging.py
> +++ b/python/samba/tests/messaging.py
> @@ -21,8 +21,9 @@
>  import samba
>  from samba.messaging import Messaging
>  from samba.tests import TestCase
> -from samba.dcerpc.server_id import server_id
> +import time
>  from samba.ndr import ndr_print
> +from samba.dcerpc import server_id
>  import random
>  
>  class MessagingTests(TestCase):
> @@ -35,7 +36,8 @@ class MessagingTests(TestCase):
>          x = self.get_context()
>          def callback():
>              pass
> -        msg_type = x.register(callback)
> +        msg_type = x.register((callback, None))
> +        self.assertTrue(isinstance(msg_type, long))
>          x.deregister(callback, msg_type)
>  
>      def test_all_servers(self):
> @@ -54,7 +56,7 @@ class MessagingTests(TestCase):
>  
>      def test_assign_server_id(self):
>          x = self.get_context()
> -        self.assertTrue(isinstance(x.server_id, server_id))
> +        self.assertTrue(isinstance(x.server_id, server_id.server_id))
>  
>      def test_add_remove_name(self):
>          x = self.get_context()
> @@ -69,19 +71,41 @@ class MessagingTests(TestCase):
>                            x.irpc_servers_byname, name)
>  
>      def test_ping_speed(self):
> +        got_ping = {"count": 0}
> +        got_pong = {"count": 0}
> +        timeout = False
> +
> +        msg_pong = 0
> +        msg_ping = 0
> +
>          server_ctx = self.get_context((0, 1))
> -        def ping_callback(src, data):
> -                server_ctx.send(src, data)
> -        def exit_callback():
> -                print "received exit"
> -        msg_ping = server_ctx.register(ping_callback)
> -        msg_exit = server_ctx.register(exit_callback)
> -
> -        def pong_callback():
> -                print "received pong"
> +        def ping_callback(got_ping, msg_type, src, data):
> +            got_ping["count"] += 1
> +            server_ctx.send(src, msg_pong, data)
> +
> +        msg_ping = server_ctx.register((ping_callback, got_ping))
> +
> +        def pong_callback(got_pong, msg_type, src, data):
> +            got_pong["count"] += 1
> +
>          client_ctx = self.get_context((0, 2))
> -        msg_pong = client_ctx.register(pong_callback)
> +        msg_pong = client_ctx.register((pong_callback, got_pong))
>  
> +        # Try both server_id forms (structure and tuple)
>          client_ctx.send((0, 1), msg_ping, "testing")
> -        client_ctx.send((0, 1), msg_ping, "")
>  
> +        client_ctx.send((0, 1), msg_ping, "testing2")
> +
> +        start_time = time.time()
> +
> +        # NOTE WELL: If debugging this with GDB, then the timeout will
> +        # fire while you are trying to understand it.
> +
> +        while (got_ping["count"] < 2 or got_pong["count"] < 2) and not timeout:
> +            client_ctx.loop_once(0.1)
> +            server_ctx.loop_once(0.1)
> +            if time.time() - start_time > 1:
> +                timeout = True
> +
> +        self.assertEqual(got_ping["count"], 2)
> +        self.assertEqual(got_pong["count"], 2)
> diff --git a/source4/lib/messaging/messaging.c b/source4/lib/messaging/messaging.c
> index 84df934..4d75f09 100644
> --- a/source4/lib/messaging/messaging.c
> +++ b/source4/lib/messaging/messaging.c
> @@ -24,6 +24,7 @@
>  #include "lib/util/server_id.h"
>  #include "system/filesys.h"
>  #include "messaging/messaging.h"
> +#include "messaging/messaging_internal.h"
>  #include "../lib/util/dlinklist.h"
>  #include "lib/socket/socket.h"
>  #include "librpc/gen_ndr/ndr_irpc.h"
> @@ -55,22 +56,6 @@ struct irpc_request {
>   } incoming;
>  };
>  
> -struct imessaging_context {
> - struct imessaging_context *prev, *next;
> - struct tevent_context *ev;
> - struct server_id server_id;
> - const char *sock_dir;
> - const char *lock_dir;
> - struct dispatch_fn **dispatch;
> - uint32_t num_types;
> - struct idr_context *dispatch_tree;
> - struct irpc_list *irpc;
> - struct idr_context *idr;
> - struct server_id_db *names;
> - struct timeval start_time;
> - void *msg_dgm_ref;
> -};
> -
>  /* we have a linked list of dispatch handlers for each msg_type that
>     this messaging server can deal with */
>  struct dispatch_fn {
> diff --git a/source4/lib/messaging/messaging_internal.h b/source4/lib/messaging/messaging_internal.h
> new file mode 100644
> index 0000000..93c5c4b
> --- /dev/null
> +++ b/source4/lib/messaging/messaging_internal.h
> @@ -0,0 +1,36 @@
> +/*
> +   Unix SMB/CIFS implementation.
> +
> +   Samba internal messaging functions
> +
> +   Copyright (C) Andrew Tridgell 2004
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +struct imessaging_context {
> + struct imessaging_context *prev, *next;
> + struct tevent_context *ev;
> + struct server_id server_id;
> + const char *sock_dir;
> + const char *lock_dir;
> + struct dispatch_fn **dispatch;
> + uint32_t num_types;
> + struct idr_context *dispatch_tree;
> + struct irpc_list *irpc;
> + struct idr_context *idr;
> + struct server_id_db *names;
> + struct timeval start_time;
> + void *msg_dgm_ref;
> +};
> diff --git a/source4/lib/messaging/pymessaging.c b/source4/lib/messaging/pymessaging.c
> index b317955..84ceff5 100644
> --- a/source4/lib/messaging/pymessaging.c
> +++ b/source4/lib/messaging/pymessaging.c
> @@ -34,6 +34,7 @@
>  #include "librpc/rpc/dcerpc.h"
>  #include "librpc/gen_ndr/server_id.h"
>  #include <pytalloc.h>
> +#include "messaging_internal.h"
>  
>  void initmessaging(void);
>  
> @@ -173,7 +174,8 @@ static void py_msg_callback_wrapper(struct imessaging_context *msg, void *privat
>         uint32_t msg_type,
>         struct server_id server_id, DATA_BLOB *data)
>  {
> - PyObject *py_server_id, *callback = (PyObject *)private_data;
> + PyObject *py_server_id, *callback_and_tuple = (PyObject *)private_data;
> + PyObject *callback, *py_private;
>  
>   struct server_id *p_server_id = talloc(NULL, struct server_id);
>   if (!p_server_id) {
> @@ -182,10 +184,18 @@ static void py_msg_callback_wrapper(struct imessaging_context *msg, void *privat
>   }
>   *p_server_id = server_id;
>  
> + if (!PyArg_ParseTuple(callback_and_tuple, "OO",
> +      &callback,
> +      &py_private)) {
> + return;
> + }
> +
>   py_server_id = py_return_ndr_struct("samba.dcerpc.server_id", "server_id", p_server_id, p_server_id);
>   talloc_unlink(NULL, p_server_id);
>  
> - PyObject_CallFunction(callback, discard_const_p(char, "i(O)s#"), msg_type,
> + PyObject_CallFunction(callback, discard_const_p(char, "OiOs#"),
> +      py_private,
> +      msg_type,
>        py_server_id,
>        data->data, data->length);
>  }
> @@ -194,24 +204,30 @@ static PyObject *py_imessaging_register(PyObject *self, PyObject *args, PyObject
>  {
>   imessaging_Object *iface = (imessaging_Object *)self;
>   int msg_type = -1;
> - PyObject *callback;
> + PyObject *callback_and_context;
>   NTSTATUS status;
> - const char *kwnames[] = { "callback", "msg_type", NULL };
> + const char *kwnames[] = { "callback_and_context", "msg_type", NULL };
>  
>   if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O|i:register",
> - discard_const_p(char *, kwnames), &callback, &msg_type)) {
> + discard_const_p(char *, kwnames),
> + &callback_and_context, &msg_type)) {
> + return NULL;
> + }
> + if (!PyTuple_Check(callback_and_context)
> +    || PyTuple_Size(callback_and_context) != 2) {
> + PyErr_SetString(PyExc_ValueError, "Expected of size 2 for callback_and_context");
>   return NULL;
>   }
>  
> - Py_INCREF(callback);
> + Py_INCREF(callback_and_context);
>  
>   if (msg_type == -1) {
>   uint32_t msg_type32 = msg_type;
> - status = imessaging_register_tmp(iface->msg_ctx, callback,
> + status = imessaging_register_tmp(iface->msg_ctx, callback_and_context,
>   py_msg_callback_wrapper, &msg_type32);
>   msg_type = msg_type32;
>   } else {
> - status = imessaging_register(iface->msg_ctx, callback,
> + status = imessaging_register(iface->msg_ctx, callback_and_context,
>      msg_type, py_msg_callback_wrapper);
>   }
>   if (NT_STATUS_IS_ERR(status)) {
> @@ -241,6 +257,52 @@ static PyObject *py_imessaging_deregister(PyObject *self, PyObject *args, PyObje
>   Py_RETURN_NONE;
>  }
>  
> +static void simple_timer_handler(struct tevent_context *ev,
> + struct tevent_timer *te,
> + struct timeval current_time,
> + void *private_data)
> +{
> + return;
> +}
> +
> +static PyObject *py_imessaging_loop_once(PyObject *self, PyObject *args, PyObject *kwargs)
> +{
> + imessaging_Object *iface = (imessaging_Object *)self;
> + double offset;
> + int seconds;
> + struct timeval next_event;
> + struct tevent_timer *timer = NULL;
> + const char *kwnames[] = { "timeout", NULL };
> +
> + TALLOC_CTX *frame = talloc_stackframe();
> +
> + if (!PyArg_ParseTupleAndKeywords(args, kwargs, "d",
> + discard_const_p(char *, kwnames), &offset)) {
> + TALLOC_FREE(frame);
> + return NULL;
> + }
> +
> + if (offset != 0.0) {
> + seconds = offset;
> + offset -= seconds;
> + next_event = tevent_timeval_current_ofs(seconds, (int)(offset*1000000));
> +
> + timer = tevent_add_timer(iface->msg_ctx->ev, frame, next_event, simple_timer_handler,
> + NULL);
> + if (timer == NULL) {
> + PyErr_NoMemory();
> + TALLOC_FREE(frame);
> + return NULL;
> + }
> + }
> +
> + tevent_loop_once(iface->msg_ctx->ev);
> +
> + TALLOC_FREE(frame);
> +
> + Py_RETURN_NONE;
> +}
> +
>  static PyObject *py_irpc_add_name(PyObject *self, PyObject *args, PyObject *kwargs)
>  {
>   imessaging_Object *iface = (imessaging_Object *)self;
> @@ -374,6 +436,8 @@ 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" },
> + { "loop_once", (PyCFunction)py_imessaging_loop_once, METH_VARARGS|METH_KEYWORDS,
> + "S.loop_once(timeout) -> None\nLoop on the internal event context until we get an event (which might be a message calling the callback), timeout after timeout seconds (if not 0)" },
>   { "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,
> --
> 2.9.3
>
>
> From 02d35eda70ea0ad8bf097f4dec32470bf55a0e66 Mon Sep 17 00:00:00 2001
> From: Gary Lockyer <[hidden email]>
> Date: Thu, 16 Mar 2017 16:26:01 +1300
> Subject: [PATCH 7/7] pymessaging: add single element tupple form of the
>  server_id
>
> This avoids the python code needing to call getpid() internally,
> while declaring a stable task_id.
>
> Signed-off-by: Gary Lockyer <[hidden email]>
> ---
>  python/samba/tests/messaging.py     | 42 +++++++++++++++++++++++++++++++++++++
>  source4/lib/messaging/pymessaging.c |  9 +++++++-
>  2 files changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/python/samba/tests/messaging.py b/python/samba/tests/messaging.py
> index 6ee18e7..e9e6bf1 100644
> --- a/python/samba/tests/messaging.py
> +++ b/python/samba/tests/messaging.py
> @@ -25,6 +25,7 @@ import time
>  from samba.ndr import ndr_print
>  from samba.dcerpc import server_id
>  import random
> +import os
>  
>  class MessagingTests(TestCase):
>  
> @@ -109,3 +110,44 @@ class MessagingTests(TestCase):
>  
>          self.assertEqual(got_ping["count"], 2)
>          self.assertEqual(got_pong["count"], 2)
> +
> +    def test_pid_defaulting(self):
> +        got_ping = {"count": 0}
> +        got_pong = {"count": 0}
> +        timeout = False
> +
> +        msg_pong = 0
> +        msg_ping = 0
> +
> +        pid = os.getpid()
> +        server_ctx = self.get_context((pid, 1))
> +        def ping_callback(got_ping, msg_type, src, data):
> +            got_ping["count"] += 1
> +            server_ctx.send(src, msg_pong, data)
> +
> +        msg_ping = server_ctx.register((ping_callback, got_ping))
> +
> +        def pong_callback(got_pong, msg_type, src, data):
> +            got_pong["count"] += 1
> +
> +        client_ctx = self.get_context((2,))
> +        msg_pong = client_ctx.register((pong_callback, got_pong))
> +
> +        # Try 1 an two element tuple forms
> +        client_ctx.send((pid, 1), msg_ping, "testing")
> +
> +        client_ctx.send((1,), msg_ping, "testing2")
> +
> +        start_time = time.time()
> +
> +        # NOTE WELL: If debugging this with GDB, then the timeout will
> +        # fire while you are trying to understand it.
> +
> +        while (got_ping["count"] < 2 or got_pong["count"] < 2) and not timeout:
> +            client_ctx.loop_once(0.1)
> +            server_ctx.loop_once(0.1)
> +            if time.time() - start_time > 1:
> +                timeout = True
> +
> +        self.assertEqual(got_ping["count"], 2)
> +        self.assertEqual(got_pong["count"], 2)
> diff --git a/source4/lib/messaging/pymessaging.c b/source4/lib/messaging/pymessaging.c
> index 84ceff5..54920dd 100644
> --- a/source4/lib/messaging/pymessaging.c
> +++ b/source4/lib/messaging/pymessaging.c
> @@ -62,13 +62,20 @@ static bool server_id_from_py(PyObject *object, struct server_id *server_id)
>   server_id->task_id = task_id;
>   server_id->vnn = vnn;
>   return true;
> - } else {
> + } else if (PyTuple_Size(object) == 2) {
>   unsigned long long pid;
>   int task_id;
>   if (!PyArg_ParseTuple(object, "KI", &pid, &task_id))
>   return false;
>   *server_id = cluster_id(pid, task_id);
>   return true;
> + } else {
> + unsigned long long pid = getpid();
> + int task_id;
> + if (!PyArg_ParseTuple(object, "I", &task_id))
> + return false;
> + *server_id = cluster_id(pid, task_id);
> + return true;
>   }
>  }
>  
> --
> 2.9.3
>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Messaging improvements and fixes needed for auth logging

Samba - samba-technical mailing list
On Mon, 2017-03-20 at 15:08 -0700, Jeremy Allison wrote:

> On Mon, Mar 20, 2017 at 08:21:13PM +1300, Andrew Bartlett via samba-
> technical wrote:
> > To test the auth and authz logging, we decided to use the messaging
> > layer (details in our auth-logging branch in Catalyst's repo), as
> > we
> > have bindings for that in python.
> >
> > It turned out that there were a number of issues in these layers,
> > both
> > the incomplete python bindings (they never previously worked: the
> > tests
> > were not actually tests) and an issue in the serverid database due
> > to
> > the way tdb_fetch_talloc() behaved with zero-length records.
> >
> > The attached patches addresses both issues and allows us to build a
> > robust testsuite for authentication and authorization logging!
> >
> > Please review!
>
> Can you split this into separate patchsets for the serverid database
> problem and the incomplete python problem ? It's a little confusing
> having them mixed together like this.

I'll amend the commit messages, but the patches in the series I posted
here are (essentially) the unit tests for the bug in the server_id
database, because it was adding the tests for the bindings that found
the bug.

> In addition I'm assuming we'll need a bug logged for the serverid
> database
> bug, and the commit messages for that fix should be tagged with the
> BUG: https://.... url.
>
> Thanks for finding and fixing this !

Thanks.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12705

> Cheers,
>
> Jeremy.
>
> > -- 
> > Andrew Bartlett                       http://samba.org/~abartlet/
> > Authentication Developer, Samba Team  http://samba.org
> > Samba Developer, Catalyst IT          http://catalyst.net.nz/servic
> > es/samba
> > From 48ec03952ab0663257a846838272ba213683ee6a Mon Sep 17 00:00:00
> > 2001
> > From: Andrew Bartlett <[hidden email]>
> > Date: Wed, 8 Mar 2017 14:53:26 +1300
> > Subject: [PATCH 1/7] pymessaging: Add support for irpc_add_name
> >
> > Signed-off-by: Andrew Bartlett <[hidden email]>
> > Pair-Programmed-by: Gary Lockyer <[hidden email]>
> > Signed-off-by: Gary Lockyer <[hidden email]>
> > ---
> >  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.9.3
> >
> >
> > From 461fc025fba11d3ff6b723b8aa421c7d94083ce7 Mon Sep 17 00:00:00
> > 2001
> > From: Andrew Bartlett <[hidden email]>
> > Date: Tue, 14 Mar 2017 13:39:00 +1300
> > Subject: [PATCH 2/7] pymessaging: Add irpc_remove_name
> >
> > Signed-off-by: Andrew Bartlett <[hidden email]>
> > ---
> >  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.9.3
> >
> >
> > From c540d344b987f688c8fbdcbef87c85b0c8cd525f Mon Sep 17 00:00:00
> > 2001
> > From: Andrew Bartlett <[hidden email]>
> > Date: Tue, 14 Mar 2017 16:07:46 +1300
> > Subject: [PATCH 3/7] selftest: Test server_id database add and
> > removal
> >
> > Signed-off-by: Andrew Bartlett <[hidden email]>
> > ---
> >  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_a
> > dd_remove_name
> > \ No newline at end of file
> > -- 
> > 2.9.3
> >
> >
> > From eef04949817bab65aa3c3268cf8690e7753e174e Mon Sep 17 00:00:00
> > 2001
> > From: Andrew Bartlett <[hidden email]>
> > Date: Tue, 14 Mar 2017 15:22:01 +1300
> > Subject: [PATCH 4/7] lib/util: Do not return an unterminated
> > pointer in
> >  tdb_fetch_talloc()
> >
> > Otherwise, if a TDB entry is truncated to 0 length, this will
> > return
> > uninitialised memory!
> >
> > Signed-off-by: Andrew Bartlett <[hidden email]>
> > ---
> >  lib/util/server_id_db.c | 42 +++++++++++++++++++++++++++++++++--
> > -------
> >  lib/util/util_tdb.c     |  9 +++++----
> >  lib/util/util_tdb.h     |  2 +-
> >  selftest/knownfail      |  1 -
> >  4 files changed, 39 insertions(+), 15 deletions(-)
> >
> > diff --git a/lib/util/server_id_db.c b/lib/util/server_id_db.c
> > index e0b8476..937de89 100644
> > --- a/lib/util/server_id_db.c
> > +++ b/lib/util/server_id_db.c
> > @@ -137,7 +137,8 @@ int server_id_db_prune_name(struct server_id_db
> > *db, const char *name,
> >   size_t idbuf_len = server_id_str_buf_unique(server, NULL,
> > 0);
> >   char idbuf[idbuf_len];
> >   TDB_DATA key;
> > - uint8_t *data;
> > + TDB_DATA data;
> > + TDB_DATA data_store;
> >   char *ids, *id;
> >   int ret;
> >  
> > @@ -156,18 +157,32 @@ int server_id_db_prune_name(struct
> > server_id_db *db, const char *name,
> >   return ret;
> >   }
> >  
> > - ids = (char *)data;
> > + if (data.dsize == 0) {
> > + tdb_chainunlock(tdb, key);
> > + TALLOC_FREE(data.dptr);
> > + return ENOENT;
> > + }
> > +
> > + /* We assert that the DB contains a NULL-terminated string
> > */
> > + ids = (char *)data.dptr;
> >  
> >   id = strv_find(ids, idbuf);
> >   if (id == NULL) {
> >   tdb_chainunlock(tdb, key);
> > - TALLOC_FREE(data);
> > + TALLOC_FREE(data.dptr);
> >   return ENOENT;
> >   }
> >  
> >   strv_delete(&ids, id);
> > - ret = tdb_store(tdb, key, talloc_tdb_data(ids),
> > TDB_MODIFY);
> > - TALLOC_FREE(data);
> > +
> > + data_store = talloc_tdb_data(ids);
> > +
> > + if (data_store.dsize == 0) {
> > + ret = tdb_delete(tdb, key);
> > + } else {
> > + ret = tdb_store(tdb, key, data_store, TDB_MODIFY);
> > + }
> > + TALLOC_FREE(data.dptr);
> >  
> >   tdb_chainunlock(tdb, key);
> >  
> > @@ -199,7 +214,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;
> > + TDB_DATA data;
> >   char *ids, *id;
> >   unsigned num_servers;
> >   struct server_id *servers;
> > @@ -212,12 +227,17 @@ int server_id_db_lookup(struct server_id_db
> > *db, const char *name,
> >   return ret;
> >   }
> >  
> > - ids = (char *)data;
> > + if (data.dsize == 0) {
> > + return ENOENT;
> > + }
> > +
> > + /* We assert that the DB contains a NULL-terminated string
> > */
> > + ids = (char *)data.dptr;
> >   num_servers = strv_count(ids);
> >  
> >   servers = talloc_array(mem_ctx, struct server_id,
> > num_servers);
> >   if (servers == NULL) {
> > - TALLOC_FREE(data);
> > + TALLOC_FREE(data.dptr);
> >   return ENOMEM;
> >   }
> >  
> > @@ -227,7 +247,7 @@ int server_id_db_lookup(struct server_id_db
> > *db, const char *name,
> >   servers[i++] =
> > server_id_from_string(NONCLUSTER_VNN, id);
> >   }
> >  
> > - TALLOC_FREE(data);
> > + TALLOC_FREE(data.dptr);
> >  
> >   *pnum_servers = num_servers;
> >   *pservers = servers;
> > @@ -283,6 +303,10 @@ static int server_id_db_traverse_fn(struct
> > tdb_context *tdb,
> >   }
> >   name = (const char *)key.dptr;
> >  
> > + if (data.dsize == 0) {
> > + return 0;
> > + }
> > +
> >   ids = (char *)talloc_memdup(state->mem_ctx, data.dptr,
> > data.dsize);
> >   if (ids == NULL) {
> >   return 0;
> > diff --git a/lib/util/util_tdb.c b/lib/util/util_tdb.c
> > index 9bf18dc..8ff89ff 100644
> > --- a/lib/util/util_tdb.c
> > +++ b/lib/util/util_tdb.c
> > @@ -486,19 +486,20 @@ int map_unix_error_from_tdb(enum TDB_ERROR
> > err)
> >  
> >  struct tdb_fetch_talloc_state {
> >   TALLOC_CTX *mem_ctx;
> > - uint8_t *buf;
> > + TDB_DATA buf;
> >  };
> >  
> >  static int tdb_fetch_talloc_parser(TDB_DATA key, TDB_DATA data,
> >                                     void *private_data)
> >  {
> >   struct tdb_fetch_talloc_state *state = private_data;
> > - state->buf = talloc_memdup(state->mem_ctx, data.dptr,
> > data.dsize);
> > + state->buf.dptr = talloc_memdup(state->mem_ctx, data.dptr,
> > data.dsize);
> > + state->buf.dsize = data.dsize;
> >   return 0;
> >  }
> >  
> >  int tdb_fetch_talloc(struct tdb_context *tdb, TDB_DATA key,
> > -      TALLOC_CTX *mem_ctx, uint8_t **buf)
> > +      TALLOC_CTX *mem_ctx, TDB_DATA *buf)
> >  {
> >   struct tdb_fetch_talloc_state state = { .mem_ctx = mem_ctx
> > };
> >   int ret;
> > @@ -509,7 +510,7 @@ int tdb_fetch_talloc(struct tdb_context *tdb,
> > TDB_DATA key,
> >   return map_unix_error_from_tdb(err);
> >   }
> >  
> > - if (state.buf == NULL) {
> > + if (state.buf.dptr == NULL && state.buf.dsize != 0) {
> >   return ENOMEM;
> >   }
> >  
> > diff --git a/lib/util/util_tdb.h b/lib/util/util_tdb.h
> > index 3b50789..1f56341 100644
> > --- a/lib/util/util_tdb.h
> > +++ b/lib/util/util_tdb.h
> > @@ -146,6 +146,6 @@ NTSTATUS map_nt_error_from_tdb(enum TDB_ERROR
> > err);
> >  int map_unix_error_from_tdb(enum TDB_ERROR err);
> >  
> >  int tdb_fetch_talloc(struct tdb_context *tdb, TDB_DATA key,
> > -      TALLOC_CTX *mem_ctx, uint8_t **buf);
> > +      TALLOC_CTX *mem_ctx, TDB_DATA *buf);
> >  
> >  #endif /* _____LIB_UTIL_UTIL_TDB_H__ */
> > 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_a
> > dd_remove_name
> > \ No newline at end of file
> > -- 
> > 2.9.3
> >
> >
> > From fd22a3fdbd6e3561f76dbad05730c78f7500a135 Mon Sep 17 00:00:00
> > 2001
> > From: Andrew Bartlett <[hidden email]>
> > Date: Mon, 20 Mar 2017 14:57:41 +1300
> > Subject: [PATCH 5/7] server_id: Add runtime check for null
> > termination
> >
> > Signed-off-by: Andrew Bartlett <[hidden email]>
> > ---
> >  lib/util/server_id_db.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/lib/util/server_id_db.c b/lib/util/server_id_db.c
> > index 937de89..8aeda98 100644
> > --- a/lib/util/server_id_db.c
> > +++ b/lib/util/server_id_db.c
> > @@ -164,6 +164,10 @@ int server_id_db_prune_name(struct
> > server_id_db *db, const char *name,
> >   }
> >  
> >   /* We assert that the DB contains a NULL-terminated string
> > */
> > + if (data.dptr[data.dsize - 1] != '\0') {
> > + return EINVAL;
> > + }
> > +
> >   ids = (char *)data.dptr;
> >  
> >   id = strv_find(ids, idbuf);
> > @@ -232,6 +236,10 @@ int server_id_db_lookup(struct server_id_db
> > *db, const char *name,
> >   }
> >  
> >   /* We assert that the DB contains a NULL-terminated string
> > */
> > + if (data.dptr[data.dsize - 1] != '\0') {
> > + return EINVAL;
> > + }
> > +
> >   ids = (char *)data.dptr;
> >   num_servers = strv_count(ids);
> >  
> > -- 
> > 2.9.3
> >
> >
> > From a78ec79a012c4b0e8face1efa4b043180b26e001 Mon Sep 17 00:00:00
> > 2001
> > From: Andrew Bartlett <[hidden email]>
> > Date: Tue, 14 Mar 2017 12:39:13 +1300
> > Subject: [PATCH 6/7] pymessaging: Add a hook to run the event loop,
> > make
> >  callbacks practical
> >
> > These change allow us to write a messaging server in python.
> >
> > The previous ping_speed test did not actually test anything, so
> > we use .loop_once() to make it actually work.  To enable practial
> > use
> > a context is supplied in the tuple with the callback, and the
> > server_id
> > for the reply is not placed inside an additional tuple.
> >
> > In order to get at the internal event context on which to loop, we
> > expose imessaging_context in messaging_internal.h and allow the
> > python
> > bindings to use that header.
> >
> > Signed-off-by: Andrew Bartlett <[hidden email]>
> > ---
> >  python/samba/tests/messaging.py            | 52 +++++++++++++-----
> > -
> >  source4/lib/messaging/messaging.c          | 17 +------
> >  source4/lib/messaging/messaging_internal.h | 36 ++++++++++++++
> >  source4/lib/messaging/pymessaging.c        | 80
> > +++++++++++++++++++++++++++---
> >  4 files changed, 147 insertions(+), 38 deletions(-)
> >  create mode 100644 source4/lib/messaging/messaging_internal.h
> >
> > diff --git a/python/samba/tests/messaging.py
> > b/python/samba/tests/messaging.py
> > index a70be96..6ee18e7 100644
> > --- a/python/samba/tests/messaging.py
> > +++ b/python/samba/tests/messaging.py
> > @@ -21,8 +21,9 @@
> >  import samba
> >  from samba.messaging import Messaging
> >  from samba.tests import TestCase
> > -from samba.dcerpc.server_id import server_id
> > +import time
> >  from samba.ndr import ndr_print
> > +from samba.dcerpc import server_id
> >  import random
> >  
> >  class MessagingTests(TestCase):
> > @@ -35,7 +36,8 @@ class MessagingTests(TestCase):
> >          x = self.get_context()
> >          def callback():
> >              pass
> > -        msg_type = x.register(callback)
> > +        msg_type = x.register((callback, None))
> > +        self.assertTrue(isinstance(msg_type, long))
> >          x.deregister(callback, msg_type)
> >  
> >      def test_all_servers(self):
> > @@ -54,7 +56,7 @@ class MessagingTests(TestCase):
> >  
> >      def test_assign_server_id(self):
> >          x = self.get_context()
> > -        self.assertTrue(isinstance(x.server_id, server_id))
> > +        self.assertTrue(isinstance(x.server_id,
> > server_id.server_id))
> >  
> >      def test_add_remove_name(self):
> >          x = self.get_context()
> > @@ -69,19 +71,41 @@ class MessagingTests(TestCase):
> >                            x.irpc_servers_byname, name)
> >  
> >      def test_ping_speed(self):
> > +        got_ping = {"count": 0}
> > +        got_pong = {"count": 0}
> > +        timeout = False
> > +
> > +        msg_pong = 0
> > +        msg_ping = 0
> > +
> >          server_ctx = self.get_context((0, 1))
> > -        def ping_callback(src, data):
> > -                server_ctx.send(src, data)
> > -        def exit_callback():
> > -                print "received exit"
> > -        msg_ping = server_ctx.register(ping_callback)
> > -        msg_exit = server_ctx.register(exit_callback)
> > -
> > -        def pong_callback():
> > -                print "received pong"
> > +        def ping_callback(got_ping, msg_type, src, data):
> > +            got_ping["count"] += 1
> > +            server_ctx.send(src, msg_pong, data)
> > +
> > +        msg_ping = server_ctx.register((ping_callback, got_ping))
> > +
> > +        def pong_callback(got_pong, msg_type, src, data):
> > +            got_pong["count"] += 1
> > +
> >          client_ctx = self.get_context((0, 2))
> > -        msg_pong = client_ctx.register(pong_callback)
> > +        msg_pong = client_ctx.register((pong_callback, got_pong))
> >  
> > +        # Try both server_id forms (structure and tuple)
> >          client_ctx.send((0, 1), msg_ping, "testing")
> > -        client_ctx.send((0, 1), msg_ping, "")
> >  
> > +        client_ctx.send((0, 1), msg_ping, "testing2")
> > +
> > +        start_time = time.time()
> > +
> > +        # NOTE WELL: If debugging this with GDB, then the timeout
> > will
> > +        # fire while you are trying to understand it.
> > +
> > +        while (got_ping["count"] < 2 or got_pong["count"] < 2) and
> > not timeout:
> > +            client_ctx.loop_once(0.1)
> > +            server_ctx.loop_once(0.1)
> > +            if time.time() - start_time > 1:
> > +                timeout = True
> > +
> > +        self.assertEqual(got_ping["count"], 2)
> > +        self.assertEqual(got_pong["count"], 2)
> > diff --git a/source4/lib/messaging/messaging.c
> > b/source4/lib/messaging/messaging.c
> > index 84df934..4d75f09 100644
> > --- a/source4/lib/messaging/messaging.c
> > +++ b/source4/lib/messaging/messaging.c
> > @@ -24,6 +24,7 @@
> >  #include "lib/util/server_id.h"
> >  #include "system/filesys.h"
> >  #include "messaging/messaging.h"
> > +#include "messaging/messaging_internal.h"
> >  #include "../lib/util/dlinklist.h"
> >  #include "lib/socket/socket.h"
> >  #include "librpc/gen_ndr/ndr_irpc.h"
> > @@ -55,22 +56,6 @@ struct irpc_request {
> >   } incoming;
> >  };
> >  
> > -struct imessaging_context {
> > - struct imessaging_context *prev, *next;
> > - struct tevent_context *ev;
> > - struct server_id server_id;
> > - const char *sock_dir;
> > - const char *lock_dir;
> > - struct dispatch_fn **dispatch;
> > - uint32_t num_types;
> > - struct idr_context *dispatch_tree;
> > - struct irpc_list *irpc;
> > - struct idr_context *idr;
> > - struct server_id_db *names;
> > - struct timeval start_time;
> > - void *msg_dgm_ref;
> > -};
> > -
> >  /* we have a linked list of dispatch handlers for each msg_type
> > that
> >     this messaging server can deal with */
> >  struct dispatch_fn {
> > diff --git a/source4/lib/messaging/messaging_internal.h
> > b/source4/lib/messaging/messaging_internal.h
> > new file mode 100644
> > index 0000000..93c5c4b
> > --- /dev/null
> > +++ b/source4/lib/messaging/messaging_internal.h
> > @@ -0,0 +1,36 @@
> > +/*
> > +   Unix SMB/CIFS implementation.
> > +
> > +   Samba internal messaging functions
> > +
> > +   Copyright (C) Andrew Tridgell 2004
> > +
> > +   This program is free software; you can redistribute it and/or
> > modify
> > +   it under the terms of the GNU General Public License as
> > published by
> > +   the Free Software Foundation; either version 3 of the License,
> > or
> > +   (at your option) any later version.
> > +
> > +   This program is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +   GNU General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public
> > License
> > +   along with this program.  If not, see <http://www.gnu.org/licen
> > ses/>.
> > +*/
> > +
> > +struct imessaging_context {
> > + struct imessaging_context *prev, *next;
> > + struct tevent_context *ev;
> > + struct server_id server_id;
> > + const char *sock_dir;
> > + const char *lock_dir;
> > + struct dispatch_fn **dispatch;
> > + uint32_t num_types;
> > + struct idr_context *dispatch_tree;
> > + struct irpc_list *irpc;
> > + struct idr_context *idr;
> > + struct server_id_db *names;
> > + struct timeval start_time;
> > + void *msg_dgm_ref;
> > +};
> > diff --git a/source4/lib/messaging/pymessaging.c
> > b/source4/lib/messaging/pymessaging.c
> > index b317955..84ceff5 100644
> > --- a/source4/lib/messaging/pymessaging.c
> > +++ b/source4/lib/messaging/pymessaging.c
> > @@ -34,6 +34,7 @@
> >  #include "librpc/rpc/dcerpc.h"
> >  #include "librpc/gen_ndr/server_id.h"
> >  #include <pytalloc.h>
> > +#include "messaging_internal.h"
> >  
> >  void initmessaging(void);
> >  
> > @@ -173,7 +174,8 @@ static void py_msg_callback_wrapper(struct
> > imessaging_context *msg, void *privat
> >          uint32_t msg_type, 
> >          struct server_id server_id,
> > DATA_BLOB *data)
> >  {
> > - PyObject *py_server_id, *callback = (PyObject
> > *)private_data;
> > + PyObject *py_server_id, *callback_and_tuple = (PyObject
> > *)private_data;
> > + PyObject *callback, *py_private;
> >  
> >   struct server_id *p_server_id = talloc(NULL, struct
> > server_id);
> >   if (!p_server_id) {
> > @@ -182,10 +184,18 @@ static void py_msg_callback_wrapper(struct
> > imessaging_context *msg, void *privat
> >   }
> >   *p_server_id = server_id;
> >  
> > + if (!PyArg_ParseTuple(callback_and_tuple, "OO",
> > +       &callback,
> > +       &py_private)) {
> > + return;
> > + }
> > +
> >   py_server_id =
> > py_return_ndr_struct("samba.dcerpc.server_id", "server_id",
> > p_server_id, p_server_id);
> >   talloc_unlink(NULL, p_server_id);
> >  
> > - PyObject_CallFunction(callback, discard_const_p(char,
> > "i(O)s#"), msg_type,
> > + PyObject_CallFunction(callback, discard_const_p(char,
> > "OiOs#"),
> > +       py_private,
> > +       msg_type,
> >         py_server_id,
> >         data->data, data->length);
> >  }
> > @@ -194,24 +204,30 @@ static PyObject
> > *py_imessaging_register(PyObject *self, PyObject *args, PyObject
> >  {
> >   imessaging_Object *iface = (imessaging_Object *)self;
> >   int msg_type = -1;
> > - PyObject *callback;
> > + PyObject *callback_and_context;
> >   NTSTATUS status;
> > - const char *kwnames[] = { "callback", "msg_type", NULL };
> > + const char *kwnames[] = { "callback_and_context",
> > "msg_type", NULL };
> >  
> >   if (!PyArg_ParseTupleAndKeywords(args, kwargs,
> > "O|i:register", 
> > - discard_const_p(char *, kwnames), &callback,
> > &msg_type)) {
> > + discard_const_p(char *, kwnames),
> > +  &callback_and_context,
> > &msg_type)) {
> > + return NULL;
> > + }
> > + if (!PyTuple_Check(callback_and_context)
> > +     || PyTuple_Size(callback_and_context) != 2) {
> > + PyErr_SetString(PyExc_ValueError, "Expected of
> > size 2 for callback_and_context");
> >   return NULL;
> >   }
> >  
> > - Py_INCREF(callback);
> > + Py_INCREF(callback_and_context);
> >  
> >   if (msg_type == -1) {
> >   uint32_t msg_type32 = msg_type;
> > - status = imessaging_register_tmp(iface->msg_ctx,
> > callback,
> > + status = imessaging_register_tmp(iface->msg_ctx,
> > callback_and_context,
> >   py_msg_callback_wr
> > apper, &msg_type32);
> >   msg_type = msg_type32;
> >   } else {
> > - status = imessaging_register(iface->msg_ctx,
> > callback,
> > + status = imessaging_register(iface->msg_ctx,
> > callback_and_context,
> >       msg_type,
> > py_msg_callback_wrapper);
> >   }
> >   if (NT_STATUS_IS_ERR(status)) {
> > @@ -241,6 +257,52 @@ static PyObject
> > *py_imessaging_deregister(PyObject *self, PyObject *args, PyObje
> >   Py_RETURN_NONE;
> >  }
> >  
> > +static void simple_timer_handler(struct tevent_context *ev,
> > +  struct tevent_timer *te,
> > +  struct timeval current_time,
> > +  void *private_data)
> > +{
> > + return;
> > +}
> > +
> > +static PyObject *py_imessaging_loop_once(PyObject *self, PyObject
> > *args, PyObject *kwargs)
> > +{
> > + imessaging_Object *iface = (imessaging_Object *)self;
> > + double offset;
> > + int seconds;
> > + struct timeval next_event;
> > + struct tevent_timer *timer = NULL;
> > + const char *kwnames[] = { "timeout", NULL };
> > +
> > + TALLOC_CTX *frame = talloc_stackframe();
> > +
> > + if (!PyArg_ParseTupleAndKeywords(args, kwargs, "d",
> > +  discard_const_p(char *,
> > kwnames), &offset)) {
> > + TALLOC_FREE(frame);
> > + return NULL;
> > + }
> > +
> > + if (offset != 0.0) {
> > + seconds = offset;
> > + offset -= seconds;
> > + next_event = tevent_timeval_current_ofs(seconds,
> > (int)(offset*1000000));
> > +
> > + timer = tevent_add_timer(iface->msg_ctx->ev,
> > frame, next_event, simple_timer_handler,
> > +  NULL);
> > + if (timer == NULL) {
> > + PyErr_NoMemory();
> > + TALLOC_FREE(frame);
> > + return NULL;
> > + }
> > + }
> > +
> > + tevent_loop_once(iface->msg_ctx->ev);
> > +
> > + TALLOC_FREE(frame);
> > +
> > + Py_RETURN_NONE;
> > +}
> > +
> >  static PyObject *py_irpc_add_name(PyObject *self, PyObject *args,
> > PyObject *kwargs)
> >  {
> >   imessaging_Object *iface = (imessaging_Object *)self;
> > @@ -374,6 +436,8 @@ 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" },
> > + { "loop_once", (PyCFunction)py_imessaging_loop_once,
> > METH_VARARGS|METH_KEYWORDS,
> > + "S.loop_once(timeout) -> None\nLoop on the
> > internal event context until we get an event (which might be a
> > message calling the callback), timeout after timeout seconds (if
> > not 0)" },
> >   { "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,
> > -- 
> > 2.9.3
> >
> >
> > From 02d35eda70ea0ad8bf097f4dec32470bf55a0e66 Mon Sep 17 00:00:00
> > 2001
> > From: Gary Lockyer <[hidden email]>
> > Date: Thu, 16 Mar 2017 16:26:01 +1300
> > Subject: [PATCH 7/7] pymessaging: add single element tupple form of
> > the
> >  server_id
> >
> > This avoids the python code needing to call getpid() internally,
> > while declaring a stable task_id.
> >
> > Signed-off-by: Gary Lockyer <[hidden email]>
> > ---
> >  python/samba/tests/messaging.py     | 42
> > +++++++++++++++++++++++++++++++++++++
> >  source4/lib/messaging/pymessaging.c |  9 +++++++-
> >  2 files changed, 50 insertions(+), 1 deletion(-)
> >
> > diff --git a/python/samba/tests/messaging.py
> > b/python/samba/tests/messaging.py
> > index 6ee18e7..e9e6bf1 100644
> > --- a/python/samba/tests/messaging.py
> > +++ b/python/samba/tests/messaging.py
> > @@ -25,6 +25,7 @@ import time
> >  from samba.ndr import ndr_print
> >  from samba.dcerpc import server_id
> >  import random
> > +import os
> >  
> >  class MessagingTests(TestCase):
> >  
> > @@ -109,3 +110,44 @@ class MessagingTests(TestCase):
> >  
> >          self.assertEqual(got_ping["count"], 2)
> >          self.assertEqual(got_pong["count"], 2)
> > +
> > +    def test_pid_defaulting(self):
> > +        got_ping = {"count": 0}
> > +        got_pong = {"count": 0}
> > +        timeout = False
> > +
> > +        msg_pong = 0
> > +        msg_ping = 0
> > +
> > +        pid = os.getpid()
> > +        server_ctx = self.get_context((pid, 1))
> > +        def ping_callback(got_ping, msg_type, src, data):
> > +            got_ping["count"] += 1
> > +            server_ctx.send(src, msg_pong, data)
> > +
> > +        msg_ping = server_ctx.register((ping_callback, got_ping))
> > +
> > +        def pong_callback(got_pong, msg_type, src, data):
> > +            got_pong["count"] += 1
> > +
> > +        client_ctx = self.get_context((2,))
> > +        msg_pong = client_ctx.register((pong_callback, got_pong))
> > +
> > +        # Try 1 an two element tuple forms
> > +        client_ctx.send((pid, 1), msg_ping, "testing")
> > +
> > +        client_ctx.send((1,), msg_ping, "testing2")
> > +
> > +        start_time = time.time()
> > +
> > +        # NOTE WELL: If debugging this with GDB, then the timeout
> > will
> > +        # fire while you are trying to understand it.
> > +
> > +        while (got_ping["count"] < 2 or got_pong["count"] < 2) and
> > not timeout:
> > +            client_ctx.loop_once(0.1)
> > +            server_ctx.loop_once(0.1)
> > +            if time.time() - start_time > 1:
> > +                timeout = True
> > +
> > +        self.assertEqual(got_ping["count"], 2)
> > +        self.assertEqual(got_pong["count"], 2)
> > diff --git a/source4/lib/messaging/pymessaging.c
> > b/source4/lib/messaging/pymessaging.c
> > index 84ceff5..54920dd 100644
> > --- a/source4/lib/messaging/pymessaging.c
> > +++ b/source4/lib/messaging/pymessaging.c
> > @@ -62,13 +62,20 @@ static bool server_id_from_py(PyObject *object,
> > struct server_id *server_id)
> >   server_id->task_id = task_id;
> >   server_id->vnn = vnn;
> >   return true;
> > - } else {
> > + } else if (PyTuple_Size(object) == 2) {
> >   unsigned long long pid;
> >   int task_id;
> >   if (!PyArg_ParseTuple(object, "KI", &pid,
> > &task_id))
> >   return false;
> >   *server_id = cluster_id(pid, task_id);
> >   return true;
> > + } else {
> > + unsigned long long pid = getpid();
> > + int task_id;
> > + if (!PyArg_ParseTuple(object, "I", &task_id))
> > + return false;
> > + *server_id = cluster_id(pid, task_id);
> > + return true;
> >   }
> >  }
> >  
> > -- 
> > 2.9.3
> >
>
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Messaging improvements and fixes needed for auth logging

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Mon, 2017-03-20 at 21:52 +0100, Volker Lendecke via samba-technical
wrote:
> On Tue, Mar 21, 2017 at 09:02:10AM +1300, Andrew Bartlett wrote:
> > > Can you explain a bit more what is going on here?
> >
> > Certainly.  Have you run the test I've added?  (make test
> > TESTS=messaging triggers it nicely). 
>
> No, I haven't run the test. Can you explain in English words what the
> bug is please?

Due to a cast in server_id_db_lookup() strv_count() will walk off the
end of a zero-length talloc pointer in search of the terminating NULL.

> > >  I would like to
> > > avoid DATA_BLOB and/or TDB_DATA where it makes sense. Here we
> > > always
> > > return a talloc'ed object that carries its own length. I think
> > > that a
> > > talloc objects is just as expressive as a DATA_BLOB, you can
> > > always
> > > query its length with talloc_get_size.
> > >
> > > I would like to understand the bug that this fixes that is not
> > > fixable
> > > with keeping just the uint8_t* return from tdb_fetch_talloc().
> >
> > The callers otherwise assumed it was a NULL terminated string, and
> > wandered off the end of the string.  
>
> I could understand that if talloc_get_size returned a char*. But it's
> a
> uint8_t* that is returned. There I would assume the caller to be
> smart
> and understand this is basically a blob.

I'm very sorry, I have to disagree.  The only caller of
tdb_fetch_talloc() (there are no tests) got it wrong, and the compiler
didn't notice either.  I think this shows that the API should be
clearer.  We have very well used abstractions for length-limited data,
and we should make the best use of them possible.

As context: because strv_delete() is based on talloc_realloc(),
talloc_get_size() returns 0 for the 'removed last entry' case (as it is
a NULL pointer), but strlen()+1 for all other cases.  This causes the
\0 terminator to be omitted when saved into the db via
talloc_tdb_data().

Thanks,

Andrew Bartlett

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Messaging improvements and fixes needed for auth logging

Samba - samba-technical mailing list
On Tue, Mar 21, 2017 at 12:22:42PM +1300, Andrew Bartlett wrote:

> On Mon, 2017-03-20 at 21:52 +0100, Volker Lendecke via samba-technical
> wrote:
> > On Tue, Mar 21, 2017 at 09:02:10AM +1300, Andrew Bartlett wrote:
> > > > Can you explain a bit more what is going on here?
> > >
> > > Certainly.  Have you run the test I've added?  (make test
> > > TESTS=messaging triggers it nicely). 
> >
> > No, I haven't run the test. Can you explain in English words what the
> > bug is please?
>
> Due to a cast in server_id_db_lookup() strv_count() will walk off the
> end of a zero-length talloc pointer in search of the terminating NULL.

That's a bug in server_id. It's great that you found it, thanks.
 

> I'm very sorry, I have to disagree.  The only caller of
> tdb_fetch_talloc() (there are no tests) got it wrong, and the compiler
> didn't notice either.  I think this shows that the API should be
> clearer.  We have very well used abstractions for length-limited data,
> and we should make the best use of them possible.
>
> As context: because strv_delete() is based on talloc_realloc(),
> talloc_get_size() returns 0 for the 'removed last entry' case (as it is
> a NULL pointer), but strlen()+1 for all other cases.  This causes the
> \0 terminator to be omitted when saved into the db via
> talloc_tdb_data().

Please don't change the API. Two reasons:

Better talloc integration: It's not as easy to make a DATA_BLOB a
talloc context as with just a uint8_t* that also carries its own
length, just like a DATA_BLOB does.

Easier re-use: With a DATA_BLOB in the API, it's just too easy to pull
in half of Samba. Even ctdb has not everything pulled in, and ctdb is
a very close partner of Samba.

Thanks,

Volker

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Messaging improvements and fixes needed for auth logging

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, Mar 21, 2017 at 11:59:45AM +1300, Andrew Bartlett via samba-technical wrote:
> I'll amend the commit messages, but the patches in the series I posted
> here are (essentially) the unit tests for the bug in the server_id
> database, because it was adding the tests for the bindings that found
> the bug.

Please make it obvious with two commits. First is the real bugfix,
and second one would be the API change with a comment "make the API
safer" or so. Then we can discuss the things separately.

Thanks,

Volker

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Messaging improvements and fixes needed for auth logging

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Mon, 2017-03-20 at 15:08 -0700, Jeremy Allison wrote:

> On Mon, Mar 20, 2017 at 08:21:13PM +1300, Andrew Bartlett via samba-
> technical wrote:
> > To test the auth and authz logging, we decided to use the messaging
> > layer (details in our auth-logging branch in Catalyst's repo), as
> > we
> > have bindings for that in python.
> >
> > It turned out that there were a number of issues in these layers,
> > both
> > the incomplete python bindings (they never previously worked: the
> > tests
> > were not actually tests) and an issue in the serverid database due
> > to
> > the way tdb_fetch_talloc() behaved with zero-length records.
> >
> > The attached patches addresses both issues and allows us to build a
> > robust testsuite for authentication and authorization logging!
> >
> > Please review!
>
> Can you split this into separate patchsets for the serverid database
> problem and the incomplete python problem ? It's a little confusing
> having them mixed together like this.
>
> In addition I'm assuming we'll need a bug logged for the serverid
> database
> bug, and the commit messages for that fix should be tagged with the
> BUG: https://.... url.
>
> Thanks for finding and fixing this !

Thanks.  I did that and addressed the 80 column yesterday, and they
have been up on the auth-logging branch since last night.  I've also
rebased on metze's patches (thanks!).

BTW, I realised later that you were entirely correct to not that
further pymessaging patches, not strictly required for this bug, where
included in the patch sent here.

Thanks,

Andrew Bartlett


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Messaging improvements and fixes needed for auth logging

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, Mar 21, 2017 at 07:48:16AM +0100, Volker Lendecke wrote:
> On Tue, Mar 21, 2017 at 11:59:45AM +1300, Andrew Bartlett via samba-technical wrote:
> > I'll amend the commit messages, but the patches in the series I posted
> > here are (essentially) the unit tests for the bug in the server_id
> > database, because it was adding the tests for the bindings that found
> > the bug.
>
> Please make it obvious with two commits. First is the real bugfix,
> and second one would be the API change with a comment "make the API
> safer" or so. Then we can discuss the things separately.

Attached is what I had in mind with "First is the real bugfix".

Review appreciated!

Thanks,

Volker

patch.txt (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Messaging improvements and fixes needed for auth logging

Samba - samba-technical mailing list
On Thu, Mar 23, 2017 at 04:36:02PM +0100, Volker Lendecke via samba-technical wrote:

> On Tue, Mar 21, 2017 at 07:48:16AM +0100, Volker Lendecke wrote:
> > On Tue, Mar 21, 2017 at 11:59:45AM +1300, Andrew Bartlett via samba-technical wrote:
> > > I'll amend the commit messages, but the patches in the series I posted
> > > here are (essentially) the unit tests for the bug in the server_id
> > > database, because it was adding the tests for the bindings that found
> > > the bug.
> >
> > Please make it obvious with two commits. First is the real bugfix,
> > and second one would be the API change with a comment "make the API
> > safer" or so. Then we can discuss the things separately.
>
> Attached is what I had in mind with "First is the real bugfix".
>
> Review appreciated!

Yep, that's the direct fix. LGTM. Pushed with
the added BUG: ref. in the commit message.

Can we also make sure the tests go in on top so
we don't regress here ?

Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Messaging improvements and fixes needed for auth logging

Samba - samba-technical mailing list
On Thu, Mar 23, 2017 at 08:56:52AM -0700, Jeremy Allison via samba-technical wrote:

> On Thu, Mar 23, 2017 at 04:36:02PM +0100, Volker Lendecke via samba-technical wrote:
> > On Tue, Mar 21, 2017 at 07:48:16AM +0100, Volker Lendecke wrote:
> > > On Tue, Mar 21, 2017 at 11:59:45AM +1300, Andrew Bartlett via samba-technical wrote:
> > > > I'll amend the commit messages, but the patches in the series I posted
> > > > here are (essentially) the unit tests for the bug in the server_id
> > > > database, because it was adding the tests for the bindings that found
> > > > the bug.
> > >
> > > Please make it obvious with two commits. First is the real bugfix,
> > > and second one would be the API change with a comment "make the API
> > > safer" or so. Then we can discuss the things separately.
> >
> > Attached is what I had in mind with "First is the real bugfix".
> >
> > Review appreciated!
>
> Yep, that's the direct fix. LGTM. Pushed with
> the added BUG: ref. in the commit message.

Shouldn't we give Andrew a chance to comment first?

Volker

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Messaging improvements and fixes needed for auth logging

Samba - samba-technical mailing list
On Thu, 2017-03-23 at 18:52 +0100, Volker Lendecke wrote:

> On Thu, Mar 23, 2017 at 08:56:52AM -0700, Jeremy Allison via samba-
> technical wrote:
> > On Thu, Mar 23, 2017 at 04:36:02PM +0100, Volker Lendecke via
> > samba-technical wrote:
> > > On Tue, Mar 21, 2017 at 07:48:16AM +0100, Volker Lendecke wrote:
> > > > On Tue, Mar 21, 2017 at 11:59:45AM +1300, Andrew Bartlett via
> > > > samba-technical wrote:
> > > > > I'll amend the commit messages, but the patches in the series
> > > > > I posted
> > > > > here are (essentially) the unit tests for the bug in the
> > > > > server_id
> > > > > database, because it was adding the tests for the bindings
> > > > > that found
> > > > > the bug.
> > > >
> > > > Please make it obvious with two commits. First is the real
> > > > bugfix,
> > > > and second one would be the API change with a comment "make the
> > > > API
> > > > safer" or so. Then we can discuss the things separately.
> > >
> > > Attached is what I had in mind with "First is the real bugfix".
> > >
> > > Review appreciated!
> >
> > Yep, that's the direct fix. LGTM. Pushed with
> > the added BUG: ref. in the commit message.
>
> Shouldn't we give Andrew a chance to comment first?

Volker,

If you could show this in combination with the tests in the same way my
 patches did, with a knownfail and then the knownfail being removed (so
we know the tests I wrote pass), that would be awesome.

Thanks,

Andrew Bartlett
--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


12