On Fri, Mar 01, 2013 at 11:40:27AM -0600, Michael Roth wrote:
> Hosts hold on to handles provided by guest-file-open for periods that can
> span beyond the life of the qemu-ga process that issued them. Since these
> are issued starting from 0 on every restart, we run the risk of issuing
> duplicate handles after restarts/reboots.
>
> As a result, users with a stale copy of these handles may end up
> reading/writing corrupted data due to their existing handles effectively
> being re-assigned to an unexpected file or offset.
>
> We unfortunately do not issue handles as strings, but as integers, so a
> solution such as using UUIDs can't be implemented without introducing a
> new interface.
>
> As a workaround, we fix this by implementing a persistent key-value store
> that will be used to track the value of the last handle that was issued
> across restarts/reboots to avoid issuing duplicates.
>
> The store is automatically written to the same directory we currently
> set via --statedir to track fsfreeze state, and so should be applicable
> for stable releases where this flag is supported.
>
> A follow-up can use this same store for handling fsfreeze state, but
> that change is cosmetic and left out for now.
>
> Signed-off-by: Michael Roth <mdr...@linux.vnet.ibm.com>
> Cc: qemu-sta...@nongnu.org
Thanks, applied to qga branch with a s/uint64/int64/ fix-up for
guest_file_handle_add()'s return value.
> ---
> qga/commands-posix.c | 25 +++++--
> qga/guest-agent-core.h | 1 +
> qga/main.c | 184
> ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 204 insertions(+), 6 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 7a0202e..5d12716 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -129,14 +129,22 @@ static struct {
> QTAILQ_HEAD(, GuestFileHandle) filehandles;
> } guest_file_state;
>
> -static void guest_file_handle_add(FILE *fh)
> +static uint64_t guest_file_handle_add(FILE *fh, Error **errp)
> {
> GuestFileHandle *gfh;
> + uint64_t handle;
> +
> + handle = ga_get_fd_handle(ga_state, errp);
> + if (error_is_set(errp)) {
> + return 0;
> + }
>
> gfh = g_malloc0(sizeof(GuestFileHandle));
> - gfh->id = fileno(fh);
> + gfh->id = handle;
> gfh->fh = fh;
> QTAILQ_INSERT_TAIL(&guest_file_state.filehandles, gfh, next);
> +
> + return handle;
> }
>
> static GuestFileHandle *guest_file_handle_find(int64_t id, Error **err)
> @@ -158,7 +166,7 @@ int64_t qmp_guest_file_open(const char *path, bool
> has_mode, const char *mode, E
> {
> FILE *fh;
> int fd;
> - int64_t ret = -1;
> + int64_t ret = -1, handle;
>
> if (!has_mode) {
> mode = "r";
> @@ -184,9 +192,14 @@ int64_t qmp_guest_file_open(const char *path, bool
> has_mode, const char *mode, E
> return -1;
> }
>
> - guest_file_handle_add(fh);
> - slog("guest-file-open, handle: %d", fd);
> - return fd;
> + handle = guest_file_handle_add(fh, err);
> + if (error_is_set(err)) {
> + fclose(fh);
> + return -1;
> + }
> +
> + slog("guest-file-open, handle: %d", handle);
> + return handle;
> }
>
> void qmp_guest_file_close(int64_t handle, Error **err)
> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> index 3354598..624a559 100644
> --- a/qga/guest-agent-core.h
> +++ b/qga/guest-agent-core.h
> @@ -35,6 +35,7 @@ bool ga_is_frozen(GAState *s);
> void ga_set_frozen(GAState *s);
> void ga_unset_frozen(GAState *s);
> const char *ga_fsfreeze_hook(GAState *s);
> +int64_t ga_get_fd_handle(GAState *s, Error **errp);
>
> #ifndef _WIN32
> void reopen_fd_to_null(int fd);
> diff --git a/qga/main.c b/qga/main.c
> index db281a5..3635430 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -15,6 +15,7 @@
> #include <stdbool.h>
> #include <glib.h>
> #include <getopt.h>
> +#include <glib/gstdio.h>
> #ifndef _WIN32
> #include <syslog.h>
> #include <sys/wait.h>
> @@ -30,6 +31,7 @@
> #include "qapi/qmp/qerror.h"
> #include "qapi/qmp/dispatch.h"
> #include "qga/channel.h"
> +#include "qemu/bswap.h"
> #ifdef _WIN32
> #include "qga/service-win32.h"
> #include <windows.h>
> @@ -53,6 +55,11 @@
> #endif
> #define QGA_SENTINEL_BYTE 0xFF
>
> +typedef struct GAPersistentState {
> +#define QGA_PSTATE_DEFAULT_FD_COUNTER 1000
> + int64_t fd_counter;
> +} GAPersistentState;
> +
> struct GAState {
> JSONMessageParser parser;
> GMainLoop *main_loop;
> @@ -76,6 +83,8 @@ struct GAState {
> #ifdef CONFIG_FSFREEZE
> const char *fsfreeze_hook;
> #endif
> + const gchar *pstate_filepath;
> + GAPersistentState pstate;
> };
>
> struct GAState *ga_state;
> @@ -724,6 +733,171 @@ VOID WINAPI service_main(DWORD argc, TCHAR *argv[])
> }
> #endif
>
> +static void set_persistent_state_defaults(GAPersistentState *pstate)
> +{
> + g_assert(pstate);
> + pstate->fd_counter = QGA_PSTATE_DEFAULT_FD_COUNTER;
> +}
> +
> +static void persistent_state_from_keyfile(GAPersistentState *pstate,
> + GKeyFile *keyfile)
> +{
> + g_assert(pstate);
> + g_assert(keyfile);
> + /* if any fields are missing, either because the file was tampered with
> + * by agents of chaos, or because the field wasn't present at the time
> the
> + * file was created, the best we can ever do is start over with the
> default
> + * values. so load them now, and ignore any errors in accessing key-value
> + * pairs
> + */
> + set_persistent_state_defaults(pstate);
> +
> + if (g_key_file_has_key(keyfile, "global", "fd_counter", NULL)) {
> + pstate->fd_counter =
> + g_key_file_get_int64(keyfile, "global", "fd_counter", NULL);
> + }
> +}
> +
> +static void persistent_state_to_keyfile(const GAPersistentState *pstate,
> + GKeyFile *keyfile)
> +{
> + g_assert(pstate);
> + g_assert(keyfile);
> +
> + g_key_file_set_int64(keyfile, "global", "fd_counter",
> pstate->fd_counter);
> +}
> +
> +static gboolean write_persistent_state(const GAPersistentState *pstate,
> + const gchar *path)
> +{
> + GKeyFile *keyfile = g_key_file_new();
> + GError *gerr = NULL;
> + gboolean ret = true;
> + gchar *data = NULL;
> + gsize data_len;
> +
> + g_assert(pstate);
> +
> + persistent_state_to_keyfile(pstate, keyfile);
> + data = g_key_file_to_data(keyfile, &data_len, &gerr);
> + if (gerr) {
> + g_critical("failed to convert persistent state to string: %s",
> + gerr->message);
> + ret = false;
> + goto out;
> + }
> +
> + g_file_set_contents(path, data, data_len, &gerr);
> + if (gerr) {
> + g_critical("failed to write persistent state to %s: %s",
> + path, gerr->message);
> + ret = false;
> + goto out;
> + }
> +
> +out:
> + if (gerr) {
> + g_error_free(gerr);
> + }
> + if (keyfile) {
> + g_key_file_free(keyfile);
> + }
> + g_free(data);
> + return ret;
> +}
> +
> +static gboolean read_persistent_state(GAPersistentState *pstate,
> + const gchar *path, gboolean frozen)
> +{
> + GKeyFile *keyfile = NULL;
> + GError *gerr = NULL;
> + struct stat st;
> + gboolean ret = true;
> +
> + g_assert(pstate);
> +
> + if (stat(path, &st) == -1) {
> + /* it's okay if state file doesn't exist, but any other error
> + * indicates a permissions issue or some other misconfiguration
> + * that we likely won't be able to recover from.
> + */
> + if (errno != ENOENT) {
> + g_critical("unable to access state file at path %s: %s",
> + path, strerror(errno));
> + ret = false;
> + goto out;
> + }
> +
> + /* file doesn't exist. initialize state to default values and
> + * attempt to save now. (we could wait till later when we have
> + * modified state we need to commit, but if there's a problem,
> + * such as a missing parent directory, we want to catch it now)
> + *
> + * there is a potential scenario where someone either managed to
> + * update the agent from a version that didn't use a key store
> + * while qemu-ga thought the filesystem was frozen, or
> + * deleted the key store prior to issuing a fsfreeze, prior
> + * to restarting the agent. in this case we go ahead and defer
> + * initial creation till we actually have modified state to
> + * write, otherwise fail to recover from freeze.
> + */
> + set_persistent_state_defaults(pstate);
> + if (!frozen) {
> + ret = write_persistent_state(pstate, path);
> + if (!ret) {
> + g_critical("unable to create state file at path %s", path);
> + ret = false;
> + goto out;
> + }
> + }
> + ret = true;
> + goto out;
> + }
> +
> + keyfile = g_key_file_new();
> + g_key_file_load_from_file(keyfile, path, 0, &gerr);
> + if (gerr) {
> + g_critical("error loading persistent state from path: %s, %s",
> + path, gerr->message);
> + ret = false;
> + goto out;
> + }
> +
> + persistent_state_from_keyfile(pstate, keyfile);
> +
> +out:
> + if (keyfile) {
> + g_key_file_free(keyfile);
> + }
> + if (gerr) {
> + g_error_free(gerr);
> + }
> +
> + return ret;
> +}
> +
> +int64_t ga_get_fd_handle(GAState *s, Error **errp)
> +{
> + int64_t handle;
> +
> + g_assert(s->pstate_filepath);
> + /* we blacklist commands and avoid operations that potentially require
> + * writing to disk when we're in a frozen state. this includes opening
> + * new files, so we should never get here in that situation
> + */
> + g_assert(!ga_is_frozen(s));
> +
> + handle = s->pstate.fd_counter++;
> + if (s->pstate.fd_counter < 0) {
> + s->pstate.fd_counter = 0;
> + }
> + if (!write_persistent_state(&s->pstate, s->pstate_filepath)) {
> + error_setg(errp, "failed to commit persistent state to disk");
> + }
> +
> + return handle;
> +}
> +
> int main(int argc, char **argv)
> {
> const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
> @@ -853,7 +1027,9 @@ int main(int argc, char **argv)
> ga_enable_logging(s);
> s->state_filepath_isfrozen = g_strdup_printf("%s/qga.state.isfrozen",
> state_dir);
> + s->pstate_filepath = g_strdup_printf("%s/qga.state", state_dir);
> s->frozen = false;
> +
> #ifndef _WIN32
> /* check if a previous instance of qemu-ga exited with filesystems' state
> * marked as frozen. this could be a stale value (a non-qemu-ga process
> @@ -910,6 +1086,14 @@ int main(int argc, char **argv)
> }
> }
>
> + /* load persistent state from disk */
> + if (!read_persistent_state(&s->pstate,
> + s->pstate_filepath,
> + ga_is_frozen(s))) {
> + g_critical("failed to load persistent state");
> + goto out_bad;
> + }
> +
> if (blacklist) {
> s->blacklist = blacklist;
> do {
> --
> 1.7.9.5
>
