New version of mock working (I think)

[Clark Williams]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Enrico Scholz wrote:
> williams at redhat.com (Clark Williams) writes:
>
>>>> if os.path.exists(self.basedir): cmd = '%s -rf %s' %
>>>> (self.config['rm'], self.basedir)
>>> wouldn't it be better to do such things with native syscalls
>>> instead of invoking a command? You will never get this
>>> implemented race-free with shell commands.
>> Well, as an old-timey UNIX programmer, I was always taught (by
>> even older UNIX hackers) that you shouldn't use mount(2), that
>> mount(8) is preferred because all the corner-case logic for
>> dealing with mount points is encapsulated there; just use it.
>
> NFS & similar filesystem types which require e.g. RPC are the only
> "corner-case" I am aware of. A native mount(2) would require
>
> | chdir(new_chroot); | chrootChdir("/dev/pts"); | mount("none",
> ".", "devpts", "gid=5,mode=620");
>
> or
>
> | chroot(new_chroot); | mount("none", "/dev/pts", "devpts",
> "gid=5,mode=620");
>
> while mount(8) requires significantly more written and executed
> code (think of cmdline escaping, loading the 9 libraries /bin/mount
> is linked against, ...).
>

It took me a little while to track down the mount interface (I've
never used diskutils), so I feel better about it now.  I'm not against
using the mount(2)/umount(2) calls to manage stuff in our chroot.
Also, I haven't heard anyone jump in here screaming that we shouldn't
do that (could be you, me, Michael and Dan are the only ones on this
channel :).  I'll look at modifying things to use the diskutils mount.

I have some reservations about chroot though...

<snip>

>
> do it in a safe manner...
>
> | chdir(rootdir); | chrootChdir("/dev/pts"); | mount("none", ".",
> "devpts", "gid=5,mode=620");
>
> with
>
> | chrootChdir(char const *dir) | { |   int root_fd = open("/",
> O_RDONLY); |   chroot("."); |   chdir(dir); |   int tgt_fd  =
> open(".", O_RDONLY); |   fchdir(root_fd); |   chroot("."); |
> fchdir(tgt_fd); |   close(tgt_fd); |   close(root_fd); | }
>
> [obviously, error-checking must be added at every line]
>
>
I'd like to hold off a bit on attempting to use os.chroot() rather
than using the chroot program.  As I'm sure you are aware, moving to
os.chroot() entails a fairly big philosophical shift from a design
perspective. Currently we only format commands to be run within the
chroot and feed them to the chroot program. Shifting over to
os.chroot() means that we'll need to os.fork() and wait for child
completion.  I'd rather take this in small steps, that is release a
version of mock that uses the new launcher mechanism, let that settle,
then consider getting more fine grained by actually crossing the
chroot barrier in python code.

I'm not saying I'm against doing it, rather I'd like to get this first
step out of the way before we try. In the mean time we can argue about
it to our hearts content :).

>
>>>> +    [ -d $(DESTDIR)/$(BINDIR) ] || \ +        $(MKDIR) -p
>>>> $(DESTDIR)/$(BINDIR) +    [ -d $(DESTDIR)/$(LIBDIR) ] || \ +
>>>> $(MKDIR) -p $(DESTDIR)/$(LIBDIR) +    $(INSTALL) -o root -g
>>>> $(MOCKGROUP) -m 4750 $(EXECUTABLE) $(DESTDIR)/$(BINDIR)
>>> ~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> please do not do such things. rpm packagers will hate you for
>>> that.
>> Please do not do *what* things? Create directories?
>
> Sorry; my comment was not about the mkdir stuff with the (IMO
> unneeded) test-if-exists. It just stepped in more or less
> inadvertently.
>
>
>> Use the install utility?
>
> no; only the underlined part. When 'install' was compiled with (or
> without ... I can not remember the exact conditions) AFS support,
> the 'make install' will fail as non-root user.
>
>

You know, I really didn't mean for those spec file modifications to
see the light of day. That's what I get for being lazy and doing a
'cvs diff'.


>>>> newargc = argc + 1; newargvsz = sizeof(char *) * (newargc +
>>>> 1); newargv = malloc(newargvsz);
>>> why are you doing dynamic memory allocations here? Operating on
>>> stack is much cheaper.
>>>
>> Well, I'd say that this is a holdover from kernel programming
>> where you don't put things other than trivial variables on the
>> stack.
>
> We are in userspace where we have 8MB stack or so by default... And
> I do not think that somebody wants to implement mock-helper as a
> kernel module with only 4KB stack... ;)
>
>

Bah! Child's play! Let's implement mock as a kernel thread!

>> Probably not an issue in user-space, but I'm not going to change
>> it for a one-time exec. Who cares if it's a couple of
>> micro-seconds faster to assign to the stack as opposed to
>> allocating a page off the heap and duping a few strings?
>
> We are speaking about SUID programs. Every additional and unneeded
> line of code increases complexity and complicates an audit. Your
> original code showed that the unneeded memory-allocating is
> error-prone too.
>
>
>> It's done once then and then we exec python.
>
> Why write complicated code, when same thing can be done shorter,
> faster and less error-prone?
>
>
Hoist on my own petard. When it comes down to it, I'm all about less
code. I'll go rework mock.c (the mock-helper replacement) and post a
new one for further review.

Clark

P.S.  I was really kidding about the kernel thread thing...
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iD8DBQFEnEekHyuj/+TTEp0RArdXAKCOrjnKxMDlS+GBiaV987fbQCjZBACg3zZ6
lrT/VY4GFXGkj6glnBhcSzs=
=AES+
-----END PGP SIGNATURE-----