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