Fwd: [PATCHES] simple gpo patches fixes

[David Mulder]

On 17.04.2018 17:20, Douglas Bagnall wrote:
> On 14/04/18 02:19, David Mulder via samba-technical wrote:
>> I've added that type check, as well as couple other checks. I also
>> wasn't checking the ads type input or verifying the list and it's length
>> before popping from it. Latest sources are on the github pull request.
> Thanks. The ADS check is definitely necessary.
>
> I thought the list was OK without the checks (in that you'd get a Python
> exception from PyList_GetItem(), not a segfault), but I see that you're
> accepting an empty list, so you do need more checks.
>
> Two questions:
>
> 1. Is there a test for the empty list?
Sure, if the list is len == 0, it returns success (since there is
nothing to refresh).
        gp_list_len = PyList_Size(gpo_list);
        if (gp_list_len == 0) {
                ret = Py_True;
                goto out;
        }

>
> 2. If you're only ever looking at the first item, why is it a list?
Because under the hood GROUP_POLICY_OBJECT is a linked list, but
py_ads_get_gpo_list() iterates over that linked list and turns it into a
python list (each element in the python list still holds it's *next
pointer, despite now being contained within a python list). In the C
code we need the linked list back to operate on, so we grab the first
item from the python list, which is the head of our C linked list. The
order of a python list is persistent, so we don't need to worry about
loosing the head of our linked list. The only time we'd end up operating
on something other than the head of our linked list, is if someone, say,
passed in a splice of the gpo_list, such as
gpo.check_refresh_gpo_list(gpo_list[1:]), in which case we'd take the
second item in the linked list as the head of our list. This will still
work as intended (since any member of the linked list can be the head),
and if someone splices the list this way, then that's probably what they
intended to happen anyway (this explicitly says to refresh all the gpos
expect the first, which makes sense).
>
> The first question is a "there should be" hint, but the second one is a
> genuine question from a GPO ignoramus.
>
> Douglas
>

-- 
David Mulder
SUSE Labs Software Engineer - Samba
dmulder at suse.com
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)