Fwd: [PATCHES] simple gpo patches fixes

Douglas Bagnall douglas.bagnall at catalyst.net.nz
Thu Apr 5 23:57:20 UTC 2018


On 30/03/18 07:44, David Mulder via samba-technical wrote:
> I've broken down that patch into sizable chunks. I've pushed them to the
> git repo: https://github.com/samba-team/samba/pull/154

Good. You can add my reviewed-by to all of them except the last one
where I've got a question or two (these questions may be stupid: it
would be good if the other reviewer knows *something* about GPO).

> +def gpo_version(lp, conn, path):
> +    # gpo.gpo_get_sysvol_gpt_version() reads the GPT.INI from a local file.
> +    # If we don't have a sysvol path locally (if we're not a kdc), then
> +    # store the file locally here so it can be read on a client.
> +    sysvol = lp.get("path", "sysvol")
> +    if sysvol:
> +        local_path = os.path.join(sysvol, path, 'GPT.INI')

Is there ever going to be a chance that 'path' here could contain "../"
or start with "/"? That would lead to the wrong result.

The existing use looks OK, but I feel a bit nervous about having a
function around that opens files for writing without checking the
path.

> +    else:
> +        gpt_path = lp.cache_path(os.path.join('gpt', path))
> +        local_path = os.path.join(gpt_path, 'GPT.INI')
> +        if not os.path.exists(os.path.dirname(local_path)):
> +            os.makedirs(os.path.dirname(local_path), 0o700)
> +        data = conn.loadfile(os.path.join(path, 'GPT.INI').replace('/', '\\'))
> +        encoding = chardet.detect(data)
                      ^^^^^^^
BTW, AFAICT, chardet is not a standard Python module, and is not listed
as a dependency anywhere in Samba -- it just happens to be on every
machine we've tried. This problem wasn't introduced with this
patchset, but it should be fixed.

> +        open(local_path, 'w').write(data.decode(encoding['encoding']))
> +    return int(gpo.gpo_get_sysvol_gpt_version(os.path.dirname(local_path))[1])
> +

> --- a/source4/param/pyparam.c
> +++ b/source4/param/pyparam.c

> +static PyObject *py_cache_path(PyObject *self, PyObject *args)
> +{
> +	struct loadparm_context *lp_ctx = PyLoadparmContext_AsLoadparmContext(self);
> +	char *name, *path;
> +	PyObject *ret;
> +
> +	if (!PyArg_ParseTuple(args, "s", &name)) {
> +		return NULL;
> +	}
> +
> +	path = lpcfg_cache_path(NULL, lp_ctx, name);
> +	if (!path) {

It would be good to have something like

		PyErr_Format(PyExc_RuntimeError,
                             "Unable to access cache %s",
			     name);	

in here before the return NULL, so that python has an explanation
(using RuntimeError is not great, but it is what we do). The
PyArg_ParseTuple() case is different because it sets an exception
itself.

> +		return NULL;
> +	}
> +	ret = PyStr_FromString(path);
> +	talloc_free(path);
> +
> +	return ret;
> +}
>  

>> Also, in Python 3, is a string what you want? It means the path has to
>> be utf-8 -- is that something we know? I don't know what we normally
>> do here.
> None of this code works in python3 yet, afaik. The libgpo python code
> was only just barely ported to python3.

OK.

cheers,
Douglas



More information about the samba-technical mailing list