Review: Needs Fixing > > The first call to initgroups for user jobs looks fine, but the 2nd call for > > system jobs (that specify setuid/setgid stanzas) has a few problems: > > > > (1) Shouldn't we only be calling this if we're root _and_ 'if (class->setuid > > || class->setgid)' and then performing the password and group lookup for > > job_setuid and job_setgid? > > No, in the case where the root user is in additional groups, we need the > initgroups call to populate the group list.
Ah - I did wonder about that. We've survived for a long time without calling initgroups for system jobs, but that appears to have been an oversight from way back :) > > > (2) The checks on pwd and grp for near the 2nd call to initgroups are not > > currently legitimate - pwd and grp should be initialised to NULL to avoid > > undefined behaviour. > > Oops, indeed, fixed. > > > (3) You don't need those two errno resets just before the 2nd call since the > > code never explicitly checks errno there. > > Fixed. The final change required is to update job_process_error_read() for the new JobProcessErrorTypes you've added. Once you've done this, please can you force such an error in a manual test, to ensure expected behaviour (currently, you should find that init will assert). I've raised bug 1085836 to document the requirement to create a new test program which will embody all tests that need to be run as the root user. -- https://code.launchpad.net/~stgraber/upstart/upstart-initgroups/+merge/136794 Your team Upstart Reviewers is subscribed to branch lp:upstart. -- upstart-devel mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/upstart-devel
