Bug 8213 - Fixes in idmap_autorid
Summary: Fixes in idmap_autorid
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: Winbind (show other bugs)
Version: 3.6.0rc2
Hardware: All All
: P5 normal
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-08 20:35 UTC by Michael Adam
Modified: 2011-06-24 19:09 UTC (History)
2 users (show)

See Also:


Attachments
patchset for v3-6-test (5.03 KB, patch)
2011-06-08 20:35 UTC, Michael Adam
ambi: review-
Details
patch for master (7.81 KB, patch)
2011-06-10 10:09 UTC, Michael Adam
no flags Details
updated patchset for v3-6-test (3.48 KB, patch)
2011-06-10 10:09 UTC, Michael Adam
ambi: review+
Details
updated patchset for master (7.89 KB, patch)
2011-06-10 11:38 UTC, Michael Adam
ambi: review+
Details
patch for the 3.6.0 idmap_autorid manpage (850 bytes, patch)
2011-06-24 11:10 UTC, Michael Adam
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Adam 2011-06-08 20:35:18 UTC
Created attachment 6545 [details]
patchset for v3-6-test

idmap_autorid is going to be released first with 3.6.0.
This is why I collect two little improvements to
autorid in this single bug report. If we need to split,
then I can still do it.

(1) autorid initialization should fail if configured for a non-default domain
    (i.e. for domain != "*"): It only makes sense for the default config.

(2) The configuration should be made more systematic in the sense that
    I would like to use "idmap config * : rangesize = autorid" instead
    of "autorid : rangesize".

Attached find a patchset (already applied to master) that fixes the two issues for v3-6-test. It contains the doc fix for (2) and the actual code fix for (2) consists of two patches, one preparatory and the actual config change.
Comment 1 Michael Adam 2011-06-08 20:35:59 UTC
Comment on attachment 6545 [details]
patchset for v3-6-test

Ambi, please review.
Comment 2 Christian Ambach 2011-06-10 08:42:33 UTC
Comment on attachment 6545 [details]
patchset for v3-6-test

adding the stackframe and freeing it leads to access to freed memory as the config that is allocated in the stackframe is stored as private data

additionally, the dynamic calculation if the parameter with config_option = talloc_asprintf(frame, "idmap config %s", dom->name);

becomes unnecessary with the next patch that limits the module to the default config, so this can be done with a fixed string
Comment 3 Michael Adam 2011-06-10 10:03:41 UTC
(In reply to comment #2)
> Comment on attachment 6545 [details]
> patchset for v3-6-test
> 
> adding the stackframe and freeing it leads to access to freed memory as the
> config that is allocated in the stackframe is stored as private data
> 
> additionally, the dynamic calculation if the parameter with config_option =
> talloc_asprintf(frame, "idmap config %s", dom->name);
> 
> becomes unnecessary with the next patch that limits the module to the default
> config, so this can be done with a fixed string

As discussed already: Thanks for catching the talloc error!
It was a bad oversight in my patch. Update patch following...
Comment 4 Michael Adam 2011-06-10 10:09:10 UTC
Created attachment 6556 [details]
patch for master

This is a patchset for master that first reverts
the old patches (apart from the doc fix) and then
re-does the patches the right way.

(So that the proposed v3-6-test patch that I am going
to attach next can be identical to the top master commits).
Comment 5 Michael Adam 2011-06-10 10:09:56 UTC
Created attachment 6557 [details]
updated patchset for v3-6-test

Updated patchset for v3-6-test
this consists of the top commits of the master patch
(less the revert commits)
Comment 6 Michael Adam 2011-06-10 11:38:18 UTC
Created attachment 6559 [details]
updated patchset for master

The master patchset did not apply any more after the change of TALLOC_ZERO_P to talloc_zero.

==> updated patchset attached.
Comment 7 Michael Adam 2011-06-16 22:07:47 UTC
Ambi,
did you have a chance to review/test the new patches?
Thanks - Michael
Comment 8 Christian Ambach 2011-06-20 10:44:38 UTC
Comment on attachment 6557 [details]
updated patchset for v3-6-test

reviewed the patches, but did not have time to test them out. But from pure looking, they seem to be good
Comment 9 Michael Adam 2011-06-20 12:08:17 UTC
I just pushed the (master) patches to autobuild.

Assigning to Karolin for inclusion into 3.6.0.

Cheers
Comment 10 Karolin Seeger 2011-06-21 18:03:07 UTC
Pushed to v3-6-test.
Closing out bug report.

Thanks!
Comment 11 Michael Adam 2011-06-24 11:08:43 UTC
There was a patch missing to the idmap_autorid manpage
to reflect the changes of this bug.

I did not want to open an new bug for this...
Comment 12 Michael Adam 2011-06-24 11:10:48 UTC
Created attachment 6617 [details]
patch for the 3.6.0 idmap_autorid manpage

patch attached. this patch is already in master.
Comment 13 Michael Adam 2011-06-24 11:11:11 UTC
Karo, please add the manpage patch to 3.6.0. Thanks!
Comment 14 Karolin Seeger 2011-06-24 19:09:38 UTC
Pushed.
Closing out bug report.

Thanks!