Fwd: [PATCHES] simple gpo patches fixes

[Douglas Bagnall]

On 19/04/18 00:25, David Mulder wrote:
> 
> 
> 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;
>         }

Yeah, I saw that. I meant a python test that calls this with an empty list.
If there was it would have failed until now, right?

Now I'm noticing there are no tests for gpo.check_refresh_gpo_list() at all,
which there should be.

>>
>> 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).

I see problems with that. While this works as intended:

gpo.check_refresh_gpo_list(gpo_list[1:])

if I am following, none of the following will work:

gpo.check_refresh_gpo_list(gpo_list[:2])  # truncate the list
gpo.check_refresh_gpo_list(gpo_list[:1] + gpo_list[3:])  # remove one
del gpo_list[2]; gpo.check_refresh_gpo_list(gpo_list)  # remove one
gpo_list.sort(); gpo.check_refresh_gpo_list(gpo_list)  # reorder
gpo.check_refresh_gpo_list(gpo_list1 + gpo_list 2)  # join

It would be *slightly* better to use an immutable tuple rather than a list, 
and better still to have an opaque object from python's point of view. The
real solution would be to iterate through and reconstruct the linked list
from the Python list.

cheers,
Douglas