[1/3] qtest: Enable creation of multiple qemu instances

From: Jason Baron <jbaron@redhat.com>

Currently, the qtest harness can only spawn 1 qemu instance at a time because
the parent pid is used to create the socket files. Use the child pid instead,
so we can remove that limitation.

Signed-off-by: Jason Baron <jbaron@redhat.com>
---
 tests/libqtest.c |   31 +++++++++++++++++--------------
 1 files changed, 17 insertions(+), 14 deletions(-)

On Thu, Dec 13, 2012 at 10:02 PM, Jason Baron <jbaron@redhat.com> wrote:
> From: Jason Baron <jbaron@redhat.com>
>
> Currently, the qtest harness can only spawn 1 qemu instance at a time because
> the parent pid is used to create the socket files. Use the child pid instead,
> so we can remove that limitation.
>
> Signed-off-by: Jason Baron <jbaron@redhat.com>
> ---
>  tests/libqtest.c |   31 +++++++++++++++++--------------
>  1 files changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 71b84c1..f3dd4e4 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -101,6 +101,10 @@ static pid_t qtest_qemu_pid(QTestState *s)
>      return pid;
>  }
>
> +#define QTEST_FILE_TEMP "/tmp/qtest-%d.sock"
> +#define QTEST_QMP_FILE_TEMP "/tmp/qtest-%d.qmp"
> +#define QTEST_PID_FILE_TEMP "/tmp/qtest-%d.pid"

These filenames are too predictable from security point of view,
please change the code to use mkstemp() instead. That would also solve
the original file name conflict problem.

> +
>  QTestState *qtest_init(const char *extra_args)
>  {
>      QTestState *s;
> @@ -113,25 +117,16 @@ QTestState *qtest_init(const char *extra_args)
>      qemu_binary = getenv("QTEST_QEMU_BINARY");
>      g_assert(qemu_binary != NULL);
>
> -    s = g_malloc(sizeof(*s));
> -
> -    s->socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
> -    s->qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
> -    pid_file = g_strdup_printf("/tmp/qtest-%d.pid", getpid());
> -
> -    sock = init_socket(s->socket_path);
> -    qmpsock = init_socket(s->qmp_socket_path);
> -
>      pid = fork();
>      if (pid == 0) {
>          command = g_strdup_printf("%s "
> -                                  "-qtest unix:%s,nowait "
> +                                  "-qtest unix:" QTEST_FILE_TEMP ",nowait "
>                                    "-qtest-log /dev/null "
> -                                  "-qmp unix:%s,nowait "
> -                                  "-pidfile %s "
> +                                  "-qmp unix:" QTEST_QMP_FILE_TEMP ",nowait "
> +                                  "-pidfile " QTEST_PID_FILE_TEMP " "
>                                    "-machine accel=qtest "
> -                                  "%s", qemu_binary, s->socket_path,
> -                                  s->qmp_socket_path, pid_file,
> +                                  "%s", qemu_binary, getpid(),
> +                                  getpid(), getpid(),
>                                    extra_args ?: "");
>
>          ret = system(command);
> @@ -139,6 +134,14 @@ QTestState *qtest_init(const char *extra_args)
>          g_free(command);
>      }
>
> +    s = g_malloc(sizeof(*s));
> +    s->socket_path = g_strdup_printf(QTEST_FILE_TEMP, pid);
> +    s->qmp_socket_path = g_strdup_printf(QTEST_QMP_FILE_TEMP, pid);
> +    pid_file = g_strdup_printf(QTEST_PID_FILE_TEMP, pid);
> +
> +    sock = init_socket(s->socket_path);
> +    qmpsock = init_socket(s->qmp_socket_path);
> +
>      s->fd = socket_accept(sock);
>      s->qmp_fd = socket_accept(qmpsock);
>
> --
> 1.7.1
>
>

> > +#define QTEST_FILE_TEMP "/tmp/qtest-%d.sock"
> > +#define QTEST_QMP_FILE_TEMP "/tmp/qtest-%d.qmp"
> > +#define QTEST_PID_FILE_TEMP "/tmp/qtest-%d.pid"
> 
> These filenames are too predictable from security point of view,

This need not be secure as long as the file is created with 0600
permissions.  In fact, inspecting the pid file from the shell can
be useful.

However, using mkstemp() on a prefix that includes the parent pid
can indeed be the best of both worlds.

Paolo

On Sat, Dec 15, 2012 at 9:14 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> > +#define QTEST_FILE_TEMP "/tmp/qtest-%d.sock"
>> > +#define QTEST_QMP_FILE_TEMP "/tmp/qtest-%d.qmp"
>> > +#define QTEST_PID_FILE_TEMP "/tmp/qtest-%d.pid"
>>
>> These filenames are too predictable from security point of view,
>
> This need not be secure as long as the file is created with 0600
> permissions.  In fact, inspecting the pid file from the shell can
> be useful.

Permissions do not help at all because the attacker could for example
target overwriting of a critical file.

>
> However, using mkstemp() on a prefix that includes the parent pid
> can indeed be the best of both worlds.

Yes.

>
> Paolo

On Sat, Dec 15, 2012 at 09:20:13AM +0000, Blue Swirl wrote:
> On Sat, Dec 15, 2012 at 9:14 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> > +#define QTEST_FILE_TEMP "/tmp/qtest-%d.sock"
> >> > +#define QTEST_QMP_FILE_TEMP "/tmp/qtest-%d.qmp"
> >> > +#define QTEST_PID_FILE_TEMP "/tmp/qtest-%d.pid"
> >>
> >> These filenames are too predictable from security point of view,
> >
> > This need not be secure as long as the file is created with 0600
> > permissions.  In fact, inspecting the pid file from the shell can
> > be useful.
> 
> Permissions do not help at all because the attacker could for example
> target overwriting of a critical file.
> 
> >
> > However, using mkstemp() on a prefix that includes the parent pid
> > can indeed be the best of both worlds.
> 
> Yes.
> 
> >
> > Paolo
> 

Yes, but mkstemp() creates the file, and bind() returns EADDRINUSE, if the file
already exists.

Using mktemp() in this case, with bind() should be ok, since bind() checks if
the file exists and then creates it, if not, all within the bind() system call
(so its atomic).

Thanks,

-Jason

On Mon, Dec 17, 2012 at 5:13 PM, Jason Baron <jbaron@redhat.com> wrote:
> On Sat, Dec 15, 2012 at 09:20:13AM +0000, Blue Swirl wrote:
>> On Sat, Dec 15, 2012 at 9:14 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> >> > +#define QTEST_FILE_TEMP "/tmp/qtest-%d.sock"
>> >> > +#define QTEST_QMP_FILE_TEMP "/tmp/qtest-%d.qmp"
>> >> > +#define QTEST_PID_FILE_TEMP "/tmp/qtest-%d.pid"
>> >>
>> >> These filenames are too predictable from security point of view,
>> >
>> > This need not be secure as long as the file is created with 0600
>> > permissions.  In fact, inspecting the pid file from the shell can
>> > be useful.
>>
>> Permissions do not help at all because the attacker could for example
>> target overwriting of a critical file.
>>
>> >
>> > However, using mkstemp() on a prefix that includes the parent pid
>> > can indeed be the best of both worlds.
>>
>> Yes.
>>
>> >
>> > Paolo
>>
>
> Yes, but mkstemp() creates the file, and bind() returns EADDRINUSE, if the file
> already exists.
>
> Using mktemp() in this case, with bind() should be ok, since bind() checks if
> the file exists and then creates it, if not, all within the bind() system call
> (so its atomic).

mktemp() manual page warns against using it, tempnam() looks like a
better choice.

>
> Thanks,
>
> -Jason