Bug 13046 - libgpo doesn't sort the GPOs in the correct order.
Summary: libgpo doesn't sort the GPOs in the correct order.
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Other (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-09-21 17:43 UTC by Jeremy Allison
Modified: 2017-11-01 08:50 UTC (History)
2 users (show)

See Also:


Attachments
Proposed patch for master (needs bugID adding). (13.77 KB, patch)
2017-09-21 17:43 UTC, Jeremy Allison
no flags Details
Updated git-am patch for master with loop termination fix. (14.07 KB, patch)
2017-09-22 17:37 UTC, Jeremy Allison
gd: review+
Details
git-am fix for 4.7.next, 4.6.next. (14.59 KB, patch)
2017-10-03 23:05 UTC, Jeremy Allison
gd: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2017-09-21 17:43:41 UTC
Created attachment 13621 [details]
Proposed patch for master (needs bugID adding).

From "Lutz Justen" <ljusten@google.com>

The patches make sure that the order of GPOs in
net ads gpo list matches the expected application order:

- Local first, then site GPOs, then domain GPOs, then OU GPOs.
- Domains and OUs: parent-to-child. - GPOs within same domain/OU: Link
order.
- Enforced GPOs are applied last, so they override all non-enforced GPOs.

I don't know if net ads gpo list was supposed to list GPOs in the proper
order, but our code depends on it. I didn't see any other place outside of
ads_get_gpo_list where Samba is making sure that GPOs are applied in the
right order.
Comment 1 Jeremy Allison 2017-09-22 16:19:35 UTC
Comment on attachment 13621 [details]
Proposed patch for master (needs bugID adding).

Patch is bad. As Lutz pointed out:

Concerning your patch, I've found an issue:

This loop will iterate forever since i is unsigned, so i-- will wrap to (uint32_t)-1 once it reaches 0:

uint32_t i;
for (i = gp_link->num_links - 1; i >= 0; i--) {

Thus, i needs to be int and num_links has to be casted to int, anyway, and the check for num_links==0 is unnecessary. Other than that, it looks OK.
Comment 2 Jeremy Allison 2017-09-22 17:37:57 UTC
Created attachment 13625 [details]
Updated git-am patch for master with loop termination fix.
Comment 3 Guenther Deschner 2017-09-22 20:37:57 UTC
Comment on attachment 13625 [details]
Updated git-am patch for master with loop termination fix.

Looks good, pushed to autobuild (with one cosmetic correction)
Comment 4 Jeremy Allison 2017-10-03 23:05:34 UTC
Created attachment 13647 [details]
git-am fix for 4.7.next, 4.6.next.

Cherry-pick from what went into master.
Comment 5 Guenther Deschner 2017-10-05 14:25:48 UTC
Comment on attachment 13647 [details]
git-am fix for 4.7.next, 4.6.next.

LGTM
Comment 6 Guenther Deschner 2017-10-05 14:26:21 UTC
Karolin, please add to 4.7 and 4.6. Thanks!
Comment 7 Karolin Seeger 2017-10-24 08:21:29 UTC
(In reply to Guenther Deschner from comment #6)
Pushed to autobuild-v4-{7,6}-test.
Comment 8 Karolin Seeger 2017-11-01 08:50:42 UTC
(In reply to Karolin Seeger from comment #7)
Pushed to both branches.
Closing out bug report.

Thanks!