Fwd: [PATCHES] simple gpo patches fixes

Thu Apr 12 02:34:29 UTC 2018

hi David,

On 12/04/18 07:04, David Mulder via samba-technical wrote:
> Douglas could you take a look at the current set of patches on:
> 
> https://github.com/samba-team/samba/pull/154

I'm skipping patches 1-6, which are still OK.

> From 5fc6676a3e57181e58a981085d224172a8026e9d Mon Sep 17 00:00:00 2001
> From: David Mulder <dmulder at suse.com>
> Date: Tue, 10 Apr 2018 15:07:34 -0600
> Subject: [PATCH 07/11] param: Add python binding for lpcfg_cache_path

and this one looks good now too, except for one thing...

> --- a/source4/param/pyparam.c
> +++ b/source4/param/pyparam.c
> @@ -358,6 +358,27 @@ static PyObject *py_samdb_url(PyObject *self, PyObject *unused)
>  	return PyStr_FromFormat("tdb://%s/sam.ldb", lpcfg_private_dir(lp_ctx));
>  }
>  
> +static PyObject *py_cache_path(PyObject *self, PyObject *args)
> +{
> +	struct loadparm_context *lp_ctx = PyLoadparmContext_AsLoadparmContext(self);
> +	char *name, *path;
> +	PyObject *ret;

These uninitialised pointers should really be initialised to NULL.
Sorry for not noticing before, and you wouldn't guess by looking
around pyparam.c, but our coding standard requires this.

In the interests of getting this in I have fixed this for you.

> +
> +	if (!PyArg_ParseTuple(args, "s", &name)) {
> +		return NULL;
> +	}
> +
> +	path = lpcfg_cache_path(NULL, lp_ctx, name);
> +	if (!path) {
> +		PyErr_Format(PyExc_RuntimeError,
> +			     "Unable to access cache %s", name);
> +		return NULL;
> +	}
> +	ret = PyStr_FromString(path);
> +	talloc_free(path);
> +
> +	return ret;
> +}
>  
>  static PyMethodDef py_lp_ctx_methods[] = {
>  	{ "load", py_lp_ctx_load, METH_VARARGS,
> @@ -394,6 +415,9 @@ static PyMethodDef py_lp_ctx_methods[] = {
>  	{ "samdb_url", py_samdb_url, METH_NOARGS,
>  	        "S.samdb_url() -> string\n"
>  	        "Returns the current URL for sam.ldb." },
> +	{ "cache_path", py_cache_path, METH_VARARGS,
> +		"S.cache_path(name) -> string\n"
> +		"Returns a path in the Samba cache directory." },
>  	{ NULL }
>  };
>  
> 
> From 7ed5e92f26c4343ebbe03492e0f959557197696c Mon Sep 17 00:00:00 2001
> From: David Mulder <dmulder at suse.com>
> Date: Wed, 11 Apr 2018 12:40:18 -0600
> Subject: [PATCH 08/11] libgpo: gpo_copy_file() shouldn't explicitly call smb1
> 
> Don't call cli_openx directly to open a file this
> calls smb1 code explicitly, which fails if we did
> a multi-protocol negotiate and negotiated smb2+.
> Use the higher level cli_open() instead.
> 
> Signed-off-by: David Mulder <dmulder at suse.com>
> ---
>  libgpo/gpo_filesync.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libgpo/gpo_filesync.c b/libgpo/gpo_filesync.c
> index 6e3efdaf6c1..bf0bb5381fc 100644
> --- a/libgpo/gpo_filesync.c
> +++ b/libgpo/gpo_filesync.c
> @@ -50,7 +50,7 @@ NTSTATUS gpo_copy_file(TALLOC_CTX *mem_ctx,
>  	int read_size = io_bufsize;
>  	off_t nread = 0;
>  
> -	result = cli_openx(cli, nt_path, O_RDONLY, DENY_NONE, &fnum);
> +	result = cli_open(cli, nt_path, O_RDONLY, DENY_NONE, &fnum);

This one seems very sensible, but frankly I have no idea about this
SMB stuff. Andrew says it is OK, and on this rare occasion I will
trust him.

>  	if (!NT_STATUS_IS_OK(result)) {
>  		goto out;
>  	}
> 
> From 27bebbd0e2f2061a11f9a707a0b679431e093585 Mon Sep 17 00:00:00 2001
> From: David Mulder <dmulder at suse.com>
> Date: Tue, 10 Apr 2018 15:04:53 -0600
> Subject: [PATCH 09/11] libgpo: Add python bindings for check_refresh_gpo_list
> 
> Signed-off-by: David Mulder <dmulder at suse.com>
> ---
>  libgpo/pygpo.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/libgpo/pygpo.c b/libgpo/pygpo.c
> index ac6e3237a82..e5f63e1f440 100644
> --- a/libgpo/pygpo.c
> +++ b/libgpo/pygpo.c
> @@ -290,6 +290,46 @@ static PyObject* py_ads_connect(ADS *self)
>  /* Parameter mapping and functions for the GP_EXT struct */
>  void initgpo(void);
>  
> +static PyObject *py_check_refresh_gpo_list(PyObject * self,
> +					   PyObject * args)
> +{
> +	TALLOC_CTX *frame = talloc_stackframe();
> +	ADS *ads;
> +	const char *cache_dir;
> +	struct GROUP_POLICY_OBJECT *gpo_ptr = NULL;
> +	PyObject *gpo_list;
> +	PyObject *gpo_obj;
> +	NTSTATUS status;

More uninitialised pointers.

In this case it *really* matters for cache_dir, because if a value
isn't provided by python...

> +	PyObject *ret = NULL;
> +
> +	if (!PyArg_ParseTuple(args, "OO|s", &ads, &gpo_list, &cache_dir)) {

...it will be left uninitialised here (being after the "|").

> +		goto out;
> +	}
> +	gpo_obj = PyList_GetItem(gpo_list, 0);

You should check for NULL here and exit (PyList_GetItem sets the
IndexError for you).

> +	gpo_ptr = (struct GROUP_POLICY_OBJECT *)pytalloc_get_ptr(gpo_obj);
> +
> +	if (!cache_dir) {
> +		cache_dir = cache_path(GPO_CACHE_DIR);
> +		if (!cache_dir) {
> +			PyErr_SetString(PyExc_MemoryError,
> +				"Failed to determine gpo cache dir");

I'm not convinced that MemoryError is the right thing here. The
failure *could* be a talloc error, but it could also be an I/O error,
and I suspect the latter is more likely -- but as pygpo.c seems to
follow this pattern I guess we're stuck.

> +                  goto out;
> +		}
> +	}
> +
> +	status = check_refresh_gpo_list(ads->ads_ptr, frame, cache_dir, 0,
> +					gpo_ptr);
> +	if (!NT_STATUS_IS_OK(status)) {
> +		PyErr_SetNTSTATUS(status);
> +		goto out;
> +	}
> +
> +	ret = Py_True;
> +out:
> +	TALLOC_FREE(frame);
> +	return ret;
> +}
> +
>  /* Global methods aka do not need a special pyobject type */
>  static PyObject *py_gpo_get_sysvol_gpt_version(PyObject * self,
>  					       PyObject * args)
> @@ -497,6 +537,9 @@ static PyMethodDef py_gpo_methods[] = {
>  	{"gpo_get_sysvol_gpt_version",
>  		(PyCFunction)py_gpo_get_sysvol_gpt_version,
>  		METH_VARARGS, NULL},
> +	{"check_refresh_gpo_list",
> +		(PyCFunction)py_check_refresh_gpo_list,
> +		METH_VARARGS, NULL},
>  	{NULL}
>  };
>  

> From 6ffe43e3286a5f938df86b3bbfafbb4c5d9a4efa Mon Sep 17 00:00:00 2001
> From: David Mulder <dmulder at suse.com>
> Date: Wed, 11 Apr 2018 12:45:40 -0600
> Subject: [PATCH 10/11] gpo: python chardet is not a dep of samba

Good.

> From dfdb5aedbfff632ec9c33763c0d8bda11a4bc654 Mon Sep 17 00:00:00 2001
> From: David Mulder <dmulder at suse.com>
> Date: Mon, 8 Jan 2018 07:17:29 -0700
> Subject: [PATCH 11/11] gpo: Read GPO versions locally, not from sysvol

And I think this one is OK too.

So I will get Andrew to look at these and hopefully we'll push them
all except the "refresh_gpo_list" one that needs work and the "read
gpos locally" one that depends on it.

cheers,
Douglas