4 a.m.
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
...
-static int lxcContainerSetStdio(int control, const char *ttyPath)
+static int lxcContainerSetStdio(int control, int ttyfd)
{
Hi Dan,
Not a big deal, but since ttyfd is now an input to this function,
it'd be less surprising if the caller were to close it.
Probably not worth changing...
int rc = -1;
- int ttyfd;
int open_max, i;
if (setsid() < 0) {
lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
_("setsid failed: %s"), strerror(errno));
- goto error_out;
- }
-
- ttyfd = open(ttyPath, O_RDWR|O_NOCTTY);
- if (ttyfd < 0) {
- lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
- _("open(%s) failed: %s"), ttyPath, strerror(errno));
- goto error_out;
+ goto cleanup;
}
if (ioctl(ttyfd, TIOCSCTTY, NULL) < 0) {
@@ -159,8 +161,6 @@
cleanup:
close(ttyfd);
-
-error_out:
return rc;
}
@@ -223,6 +223,7 @@
return 0;
}
+
/**
* lxcEnableInterfaces:
* @vm: Pointer to vm structure
@@ -252,6 +253,20 @@
return rc;
}
...
+ if (root) {
+ char *oldroot;
+ struct mntent *mntent;
+ char **mounts = NULL;
+ int nmounts = 0;
+ FILE *procmnt;
This can be "const":
+ struct {
+ int maj;
+ int min;
+ const char *path;
+ } devs[] = {
+ { 1, 3, "/dev/null" },
+ { 1, 5, "/dev/zero" },
+ { 1, 7, "/dev/full" },
+ { 5, 1, "/dev/console" },
Might be good to add /dev/random and /dev/urandom.
I recently had trouble on a system where udev malfunctioned,
and some /dev/{u,}random-requiring libs/services failed in unusual ways.
Also, how about adding permission bits to the table, so that
console doesn't end up being mode 0777:
struct {
int maj;
int min;
mode_t mode,
const char *path;
} const devs[] = {
{ 1, 3, 0666, "/dev/null" },
{ 1, 5, 0666, "/dev/zero" },
{ 1, 7, 0666, "/dev/full" },
{ 5, 1, 0600, "/dev/console" },
{ 1, 8, 0666, "/dev/random" },
{ 1, 9, 0666, "/dev/urandom" },
};
+ if (virFileMakePath("/dev/pts") < 0 ||
+ mount("/.oldroot/dev/pts", "/dev/pts", NULL,
+ MS_MOVE, NULL) < 0) {
+ lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("failed to move /dev/pts into container: %s"),
+ strerror(errno));
+ return -1;
+ }
+
+ /* Populate /dev/ with a few important bits */
+ umask(0);
In principle, it's better never to change umask. Otherwise, when
multi-threaded, that temporarily-cleared umask could hose a file-creation
operation in another thread -- and it'd be a race, so probably hard to
reproduce.
+ for (i = 0 ; i < ARRAY_CARDINALITY(devs) ; i++) {
+ dev_t dev = makedev(devs[i].maj, devs[i].min);
+ if (mknod(devs[i].path,
+ 0777 | S_IFCHR,
Dropping the umask, you might do s/0777/0/ here, and then
call chmod to set each mode to devs[i].mode
+ dev) < 0) {
+ lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("failed to make device %s: %s"),
+ devs[i].path, strerror(errno));
Returning here would leave umask set to 0.
Another reason not to change it in the first place.
+ return -1;
+ }
+ }
+ umask(0700);
When you do change umask, be sure to restore to the previous value.
+ /* Pull in rest of container's mounts */
+ for (tmp = vmDef->fss; tmp; tmp = tmp->next) {
+ char *src;
+ if (STREQ(tmp->dst, "/"))
+ continue;
+ // XXX fix
+ if (tmp->type != VIR_DOMAIN_FS_TYPE_MOUNT)
+ continue;
+
+ if (asprintf(&src, "/.oldroot/%s", tmp->src) < 0)
+ return -1;
+
+ if (virFileMakePath(tmp->dst) < 0 ||
+ mount(src, tmp->dst, NULL, MS_BIND, NULL) < 0) {
+ lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("failed to mount %s at %s for container: %s"),
+ tmp->src, tmp->dst, strerror(errno));
Call VIR_FREE(src) here to avoid a leak.
+ return -1;
+ }
+ VIR_FREE(src);
+ }
+
+ if (!(procmnt = setmntent("/proc/mounts", "r"))) {
+ lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+ _("failed to read /proc/mounts: %s"),
+ strerror(errno));
+ return -1;
+ }
+ while ((mntent = getmntent(procmnt)) != NULL) {
+ if (!STRPREFIX(mntent->mnt_dir, "/.oldroot"))
+ continue;
+ if (VIR_REALLOC_N(mounts, nmounts+1) < 0)
Call endmntent(procmnt) here, to
avoid a potential file descriptor leak.
+ return -1;
+ mounts[nmounts++] = strdup(mntent->mnt_dir);
A failed strdup here would lead to segfault via qsort's comparator.
> + }
> + endmntent(procmnt);
> +
> + qsort(mounts, nmounts, sizeof(mounts[0]),
> + lxcContainerChildMountSort);
> +
> + for (i = 0 ; i < nmounts ; i++) {
> + if (umount(mounts[i]) < 0) {
> + lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> + _("failed to unmount %s: %s"),
> + mounts[i], strerror(errno));
+ return -1;
Maybe try to unmount all of them before returning, even if an
earlier one fails?
Also, be sure to free "mounts" before returning.
+ }
+ }
+ } else {
...