From: "Daniel P. Berrange" <berrange@redhat.com> This introduces a few new APIs for dealing with strings. One to split a char * into a char **, another to join a char ** into a char *, and finally one to free a char ** There is a simple test suite to validate the edge cases too. No more need to use the horrible strtok_r() API, or hand-written code for splitting strings. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> (cherry picked from commit 76c1fd33c8093d6a7173a85486e1e6f51a832135) Signed-off-by: Eric Blake <eblake@redhat.com> Conflicts: tests/Makefile.am - several intermediate tests not backported --- .gitignore | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 6 ++ src/util/virstring.c | 168 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstring.h | 38 +++++++++++ tests/Makefile.am | 9 ++- tests/virstringtest.c | 161 +++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 383 insertions(+), 1 deletion(-) create mode 100644 src/util/virstring.c create mode 100644 src/util/virstring.h create mode 100644 tests/virstringtest.c diff --git a/.gitignore b/.gitignore index 3cc30e8..cc52789 100644 --- a/.gitignore +++ b/.gitignore @@ -147,6 +147,7 @@ /tests/virkeyfiletest /tests/virnet*test /tests/virshtest +/tests/virstringtest /tests/virtimetest /tests/viruritest /tests/vmx2xmltest diff --git a/src/Makefile.am b/src/Makefile.am index 0dadc29..b5da2fb 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -106,6 +106,7 @@ UTIL_SOURCES = \ util/virnetlink.c util/virnetlink.h \ util/virrandom.h util/virrandom.c \ util/virsocketaddr.h util/virsocketaddr.c \ + util/virstring.h util/virstring.c \ util/virtime.h util/virtime.c \ util/viruri.h util/viruri.c diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index afb308d..8328fcf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1490,6 +1490,12 @@ virSetErrorLogPriorityFunc; virStrerror; +# virstring.h +virStringSplit; +virStringJoin; +virStringFreeList; + + # virtime.h virTimeFieldsNow; virTimeFieldsNowRaw; diff --git a/src/util/virstring.c b/src/util/virstring.c new file mode 100644 index 0000000..1917e9a --- /dev/null +++ b/src/util/virstring.c @@ -0,0 +1,168 @@ +/* + * Copyright (C) 2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Daniel P. Berrange <berrange@redhat.com> + */ + +#include <config.h> + +#include "virstring.h" +#include "memory.h" +#include "buf.h" +#include "virterror_internal.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +/* + * The following virStringSplit & virStringJoin methods + * are derived from g_strsplit / g_strjoin in glib2, + * also available under the LGPLv2+ license terms + */ + +/** + * virStringSplit: + * @string: a string to split + * @delim: a string which specifies the places at which to split + * the string. The delimiter is not included in any of the resulting + * strings, unless @max_tokens is reached. + * @max_tokens: the maximum number of pieces to split @string into. + * If this is 0, the string is split completely. + * + * Splits a string into a maximum of @max_tokens pieces, using the given + * @delim. If @max_tokens is reached, the remainder of @string is + * appended to the last token. + * + * As a special case, the result of splitting the empty string "" is an empty + * vector, not a vector containing a single string. The reason for this + * special case is that being able to represent a empty vector is typically + * more useful than consistent handling of empty elements. If you do need + * to represent empty elements, you'll need to check for the empty string + * before calling virStringSplit(). + * + * Return value: a newly-allocated NULL-terminated array of strings. Use + * virStringFreeList() to free it. + */ +char **virStringSplit(const char *string, + const char *delim, + size_t max_tokens) +{ + char **tokens = NULL; + size_t ntokens = 0; + size_t maxtokens = 0; + const char *remainder = string; + char *tmp; + size_t i; + + if (max_tokens == 0) + max_tokens = INT_MAX; + + tmp = strstr(remainder, delim); + if (tmp) { + size_t delimlen = strlen(delim); + + while (--max_tokens && tmp) { + size_t len = tmp - remainder; + + if (VIR_RESIZE_N(tokens, maxtokens, ntokens, 1) < 0) + goto no_memory; + + if (!(tokens[ntokens] = strndup(remainder, len))) + goto no_memory; + ntokens++; + remainder = tmp + delimlen; + tmp = strstr(remainder, delim); + } + } + if (*string) { + if (VIR_RESIZE_N(tokens, maxtokens, ntokens, 1) < 0) + goto no_memory; + + if (!(tokens[ntokens] = strdup(remainder))) + goto no_memory; + ntokens++; + } + + if (VIR_RESIZE_N(tokens, maxtokens, ntokens, 1) < 0) + goto no_memory; + tokens[ntokens++] = NULL; + + return tokens; + +no_memory: + virReportOOMError(); + for (i = 0 ; i < ntokens ; i++) + VIR_FREE(tokens[i]); + VIR_FREE(tokens); + return NULL; +} + + +/** + * virStringJoin: + * @strings: a NULL-terminated array of strings to join + * @delim: a string to insert between each of the strings + * + * Joins a number of strings together to form one long string, with the + * @delim inserted between each of them. The returned string + * should be freed with VIR_FREE(). + * + * Returns: a newly-allocated string containing all of the strings joined + * together, with @delim between them + */ +char *virStringJoin(const char **strings, + const char *delim) +{ + char *ret; + virBuffer buf = VIR_BUFFER_INITIALIZER; + while (*strings) { + virBufferAdd(&buf, *strings, -1); + if (*(strings+1)) + virBufferAdd(&buf, delim, -1); + strings++; + } + if (virBufferError(&buf)) { + virReportOOMError(); + return NULL; + } + ret = virBufferContentAndReset(&buf); + if (!ret) { + if (!(ret = strdup(""))) { + virReportOOMError(); + return NULL; + } + } + return ret; +} + + +/** + * virStringFreeList: + * @str_array: a NULL-terminated array of strings to free + * + * Frees a NULL-terminated array of strings, and the array itself. + * If called on a NULL value, virStringFreeList() simply returns. + */ +void virStringFreeList(char **strings) +{ + char **tmp = strings; + while (tmp && *tmp) { + VIR_FREE(*tmp); + tmp++; + } + VIR_FREE(strings); +} diff --git a/src/util/virstring.h b/src/util/virstring.h new file mode 100644 index 0000000..a569fe0 --- /dev/null +++ b/src/util/virstring.h @@ -0,0 +1,38 @@ +/* + * Copyright (C) 2007-2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Authors: + * Daniel P. Berrange <berrange@redhat.com> + */ + +#ifndef __VIR_STRING_H__ +# define __VIR_STRING_H__ + +# include "internal.h" + +char **virStringSplit(const char *string, + const char *delim, + size_t max_tokens) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +char *virStringJoin(const char **strings, + const char *delim) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + +void virStringFreeList(char **strings); + +#endif /* __VIR_STRING_H__ */ diff --git a/tests/Makefile.am b/tests/Makefile.am index 705cbb4..1adaa4b 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -100,7 +100,9 @@ test_programs = virshtest sockettest \ virhashtest virnetmessagetest virnetsockettest \ utiltest virnettlscontexttest shunloadtest \ virtimetest viruritest virkeyfiletest \ - virauthconfigtest + virauthconfigtest \ + virstringtest \ + $(NULL) # This is a fake SSH we use from virnetsockettest ssh_SOURCES = ssh.c @@ -485,6 +487,11 @@ virtimetest_SOURCES = \ virtimetest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" $(AM_CFLAGS) virtimetest_LDADD = ../src/libvirt-net-rpc.la $(LDADDS) +virstringtest_SOURCES = \ + virstringtest.c testutils.h testutils.c +virstringtest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" $(AM_CFLAGS) +virstringtest_LDADD = $(LDADDS) + viruritest_SOURCES = \ viruritest.c testutils.h testutils.c viruritest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\"" $(AM_CFLAGS) diff --git a/tests/virstringtest.c b/tests/virstringtest.c new file mode 100644 index 0000000..7e726c6 --- /dev/null +++ b/tests/virstringtest.c @@ -0,0 +1,161 @@ +/* + * Copyright (C) 2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Daniel P. Berrange <berrange@redhat.com> + */ + +#include <config.h> + +#include <stdlib.h> + +#include "testutils.h" +#include "util.h" +#include "virterror_internal.h" +#include "memory.h" +#include "logging.h" + +#include "virstring.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +struct testSplitData { + const char *string; + const char *delim; + size_t max_tokens; + const char **tokens; +}; + + +struct testJoinData { + const char *string; + const char *delim; + const char **tokens; +}; + +static int testSplit(const void *args) +{ + const struct testSplitData *data = args; + char **got; + char **tmp1; + const char **tmp2; + int ret = -1; + + if (!(got = virStringSplit(data->string, data->delim, data->max_tokens))) { + VIR_DEBUG("Got no tokens at all"); + return -1; + } + + tmp1 = got; + tmp2 = data->tokens; + while (*tmp1 && *tmp2) { + if (STRNEQ(*tmp1, *tmp2)) { + fprintf(stderr, "Mismatch '%s' vs '%s'\n", *tmp1, *tmp2); + goto cleanup; + } + tmp1++; + tmp2++; + } + if (*tmp1) { + fprintf(stderr, "Too many pieces returned\n"); + goto cleanup; + } + if (*tmp2) { + fprintf(stderr, "Too few pieces returned\n"); + goto cleanup; + } + + ret = 0; +cleanup: + virStringFreeList(got); + + return ret; +} + + +static int testJoin(const void *args) +{ + const struct testJoinData *data = args; + char *got; + int ret = -1; + + if (!(got = virStringJoin(data->tokens, data->delim))) { + VIR_DEBUG("Got no result"); + return -1; + } + if (STRNEQ(got, data->string)) { + fprintf(stderr, "Mismatch '%s' vs '%s'\n", got, data->string); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(got); + + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + +#define TEST_SPLIT(str, del, max, toks) \ + do { \ + struct testSplitData splitData = { \ + .string = str, \ + .delim = del, \ + .max_tokens = max, \ + .tokens = toks, \ + }; \ + struct testJoinData joinData = { \ + .string = str, \ + .delim = del, \ + .tokens = toks, \ + }; \ + if (virtTestRun("Split " #str, 1, testSplit, &splitData) < 0) \ + ret = -1; \ + if (virtTestRun("Join " #str, 1, testJoin, &joinData) < 0) \ + ret = -1; \ + } while (0) + + const char *tokens1[] = { NULL }; + TEST_SPLIT("", " ", 0, tokens1); + + const char *tokens2[] = { "", "", NULL }; + TEST_SPLIT(" ", " ", 0, tokens2); + + const char *tokens3[] = { "", "", "", NULL }; + TEST_SPLIT(" ", " ", 0, tokens3); + + const char *tokens4[] = { "The", "quick", "brown", "fox", NULL }; + TEST_SPLIT("The quick brown fox", " ", 0, tokens4); + + const char *tokens5[] = { "The quick ", " fox", NULL }; + TEST_SPLIT("The quick brown fox", "brown", 0, tokens5); + + const char *tokens6[] = { "", "The", "quick", "brown", "fox", NULL }; + TEST_SPLIT(" The quick brown fox", " ", 0, tokens6); + + const char *tokens7[] = { "The", "quick", "brown", "fox", "", NULL }; + TEST_SPLIT("The quick brown fox ", " ", 0, tokens7); + + + return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIRT_TEST_MAIN(mymain) -- 1.8.3.1

From: "Daniel P. Berrange" <berrange@redhat.com> Change "Pid" to "Process" to align with the virProcessKill API naming prefix Signed-off-by: Daniel P. Berrange <berrange@redhat.com> (cherry picked from commit 0fb58ef5cd477cf9a0efdd966a22440ef087a2af) Signed-off-by: Eric Blake <eblake@redhat.com> Conflicts: src/util/util.c src/lxc/lxc_container.c src/lxc/lxc_controller.c --- daemon/libvirtd.c | 2 +- src/libvirt_private.syms | 4 ++-- src/lxc/lxc_container.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/rpc/virnetsocket.c | 2 +- src/util/command.c | 24 ++++++++++++------------ src/util/command.h | 6 +++--- src/util/util.c | 6 +++--- tests/testutils.c | 4 ++-- 9 files changed, 26 insertions(+), 26 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 5d57b50..e9db433 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -192,7 +192,7 @@ static int daemonForkIntoBackground(const char *argv0) VIR_FORCE_CLOSE(statuspipe[1]); /* We wait to make sure the first child forked successfully */ - if (virPidWait(pid, NULL) < 0) + if (virProcessWait(pid, NULL) < 0) goto error; /* If we get here, then the grandchild was spawned, so we diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8328fcf..5d8ff7b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -144,8 +144,8 @@ virCommandTranslateStatus; virCommandWait; virCommandWriteArgLog; virFork; -virPidAbort; -virPidWait; +virProcessAbort; +virProcessWait; virRun; diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 0636eab..442aa24 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1519,7 +1519,7 @@ int lxcContainerAvailable(int features) VIR_DEBUG("clone call returned %s, container support is not enabled", virStrerror(errno, ebuf, sizeof(ebuf))); return -1; - } else if (virPidWait(cpid, NULL) < 0) { + } else if (virProcessWait(cpid, NULL) < 0) { return -1; } diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 26b3115..b5cec3d 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -1626,7 +1626,7 @@ cleanup: VIR_FORCE_CLOSE(loopDevs[i]); VIR_FREE(loopDevs); - virPidAbort(container); + virProcessAbort(container); return rc; } diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index fa16d31..040090e 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -752,7 +752,7 @@ void virNetSocketFree(virNetSocketPtr sock) VIR_FORCE_CLOSE(sock->fd); VIR_FORCE_CLOSE(sock->errfd); - virPidAbort(sock->pid); + virProcessAbort(sock->pid); VIR_FREE(sock->localAddrStr); VIR_FREE(sock->remoteAddrStr); diff --git a/src/util/command.c b/src/util/command.c index eaa9f16..4340659 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -1652,7 +1652,7 @@ virCommandToString(virCommandPtr cmd) * @status: child exit status to translate * * Translate an exit status into a malloc'd string. Generic helper - * for virCommandRun(), virCommandWait(), virRun(), and virPidWait() + * for virCommandRun(), virCommandWait(), virRun(), and virProcessWait() * status argument, as well as raw waitpid(). */ char * @@ -2114,7 +2114,7 @@ virCommandHook(void *data) * for pid. While cmd is still in scope, you may reap the child via * virCommandWait or virCommandAbort. But after virCommandFree, if * you have not yet reaped the child, then it continues to run until - * you call virPidWait or virPidAbort. + * you call virProcessWait or virProcessAbort. */ int virCommandRunAsync(virCommandPtr cmd, pid_t *pid) @@ -2207,7 +2207,7 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) /** - * virPidWait: + * virProcessWait: * @pid: child to wait on * @exitstatus: optional status collection * @@ -2218,7 +2218,7 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) * child must exit with status 0 for this to succeed. */ int -virPidWait(pid_t pid, int *exitstatus) +virProcessWait(pid_t pid, int *exitstatus) { int ret; int status; @@ -2288,13 +2288,13 @@ virCommandWait(virCommandPtr cmd, int *exitstatus) return -1; } - /* If virPidWait reaps pid but then returns failure because + /* If virProcessWait reaps pid but then returns failure because * exitstatus was NULL, then a second virCommandWait would risk * calling waitpid on an unrelated process. Besides, that error * message is not as detailed as what we can provide. So, we - * guarantee that virPidWait only fails due to failure to wait, + * guarantee that virProcessWait only fails due to failure to wait, * and repeat the exitstatus check code ourselves. */ - ret = virPidWait(cmd->pid, exitstatus ? exitstatus : &status); + ret = virProcessWait(cmd->pid, exitstatus ? exitstatus : &status); if (ret == 0) { cmd->pid = -1; cmd->reap = false; @@ -2316,7 +2316,7 @@ virCommandWait(virCommandPtr cmd, int *exitstatus) #ifndef WIN32 /** - * virPidAbort: + * virProcessAbort: * @pid: child process to kill * * Abort a child process if PID is positive and that child is still @@ -2326,7 +2326,7 @@ virCommandWait(virCommandPtr cmd, int *exitstatus) * this does nothing. */ void -virPidAbort(pid_t pid) +virProcessAbort(pid_t pid) { int saved_errno; int ret; @@ -2390,13 +2390,13 @@ virCommandAbort(virCommandPtr cmd) { if (!cmd || cmd->pid == -1) return; - virPidAbort(cmd->pid); + virProcessAbort(cmd->pid); cmd->pid = -1; cmd->reap = false; } #else /* WIN32 */ void -virPidAbort(pid_t pid) +virProcessAbort(pid_t pid) { /* Not yet ported to mingw. Any volunteers? */ VIR_DEBUG("failed to reap child %lld, abandoning it", (long long)pid); @@ -2553,7 +2553,7 @@ int virCommandHandshakeNotify(virCommandPtr cmd) * * Release all resources. The only exception is that if you called * virCommandRunAsync with a non-null pid, then the asynchronous child - * is not reaped, and you must call virPidWait() or virPidAbort() yourself. + * is not reaped, and you must call virProcessWait() or virProcessAbort() yourself. */ void virCommandFree(virCommandPtr cmd) diff --git a/src/util/command.h b/src/util/command.h index 07aa0b3..d8a8c83 100644 --- a/src/util/command.h +++ b/src/util/command.h @@ -148,8 +148,8 @@ int virCommandRun(virCommandPtr cmd, int virCommandRunAsync(virCommandPtr cmd, pid_t *pid) ATTRIBUTE_RETURN_CHECK; -int virPidWait(pid_t pid, - int *exitstatus) ATTRIBUTE_RETURN_CHECK; +int virProcessWait(pid_t pid, + int *exitstatus) ATTRIBUTE_RETURN_CHECK; int virCommandWait(virCommandPtr cmd, int *exitstatus) ATTRIBUTE_RETURN_CHECK; @@ -162,7 +162,7 @@ int virCommandHandshakeWait(virCommandPtr cmd) int virCommandHandshakeNotify(virCommandPtr cmd) ATTRIBUTE_RETURN_CHECK; -void virPidAbort(pid_t pid); +void virProcessAbort(pid_t pid); void virCommandAbort(virCommandPtr cmd); diff --git a/src/util/util.c b/src/util/util.c index bb15518..e1f8d1e 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -718,10 +718,10 @@ virFileAccessibleAs(const char *path, int mode, } if (pid) { /* parent */ - if (virPidWait(pid, &status) < 0) { - /* virPidWait() already + if (virProcessWait(pid, &status) < 0) { + /* virProcessWait() already * reported error */ - return -1; + return -1; } if (!WIFEXITED(status)) { diff --git a/tests/testutils.c b/tests/testutils.c index 6eb40ed..63bec52 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -314,7 +314,7 @@ virtTestCaptureProgramOutput(const char *const argv[], char **buf, int maxlen) VIR_FORCE_CLOSE(pipefd[1]); len = virFileReadLimFD(pipefd[0], maxlen, buf); VIR_FORCE_CLOSE(pipefd[0]); - if (virPidWait(pid, NULL) < 0) + if (virProcessWait(pid, NULL) < 0) return -1; return len; @@ -684,7 +684,7 @@ int virtTestMain(int argc, } else { int i, status; for (i = 0 ; i < mp ; i++) { - if (virPidWait(workers[i], NULL) < 0) + if (virProcessWait(workers[i], NULL) < 0) ret = EXIT_FAILURE; } VIR_FREE(workers); -- 1.8.3.1

From: "Daniel P. Berrange" <berrange@redhat.com> There are a number of process related functions spread across multiple files. Start to consolidate them by creating a virprocess.{c,h} file Signed-off-by: Daniel P. Berrange <berrange@redhat.com> (cherry picked from commit e5e2b65cf86ea49eba76b3c274e3b9d2177485bc) Signed-off-by: Eric Blake <eblake@redhat.com> Conflicts: src/qemu/qemu_monitor.c src/util/util.h --- src/Makefile.am | 1 + src/libvirt_private.syms | 3 ++ src/qemu/qemu_agent.c | 1 + src/qemu/qemu_monitor.c | 1 + src/qemu/qemu_process.c | 1 + src/uml/uml_driver.c | 1 + src/util/util.c | 57 -------------------------------- src/util/util.h | 1 - src/util/virprocess.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virprocess.h | 31 ++++++++++++++++++ 10 files changed, 123 insertions(+), 58 deletions(-) create mode 100644 src/util/virprocess.c create mode 100644 src/util/virprocess.h diff --git a/src/Makefile.am b/src/Makefile.am index b5da2fb..6656fa0 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -85,6 +85,7 @@ UTIL_SOURCES = \ util/virfile.c util/virfile.h \ util/virnodesuspend.c util/virnodesuspend.h \ util/virpidfile.c util/virpidfile.h \ + util/virprocess.c util/virprocess.h \ util/virtypedparam.c util/virtypedparam.h \ util/xml.c util/xml.h \ util/virterror.c util/virterror_internal.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 13a732b..95f2543 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1453,6 +1453,9 @@ virPidFileDelete; virPidFileDeletePath; +# virprocess.h +virProcessKill; + # virrandom.h virRandomBits; virRandomGenerateWWN; diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 1e3e327..5618ddf 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -39,6 +39,7 @@ #include "virterror_internal.h" #include "json.h" #include "virfile.h" +#include "virprocess.h" #include "virtime.h" #define VIR_FROM_THIS VIR_FROM_QEMU diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index cfe1a25..265f03c 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -36,6 +36,7 @@ #include "memory.h" #include "logging.h" #include "virfile.h" +#include "virprocess.h" #define VIR_FROM_THIS VIR_FROM_QEMU diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 7fc87f4..a14fc57 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -61,6 +61,7 @@ #include "locking/domain_lock.h" #include "network/bridge_driver.h" #include "uuid.h" +#include "virprocess.h" #include "virtime.h" #include "virnetdevtap.h" diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 8db78b8..83a31a9 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -63,6 +63,7 @@ #include "configmake.h" #include "virnetdevtap.h" #include "virnodesuspend.h" +#include "virprocess.h" #include "viruri.h" #define VIR_FROM_THIS VIR_FROM_UML diff --git a/src/util/util.c b/src/util/util.c index e1f8d1e..b807c5a 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2132,63 +2132,6 @@ check_and_return: return result; } -/* send signal to a single process */ -int virProcessKill(pid_t pid, int sig) -{ - if (pid <= 1) { - errno = ESRCH; - return -1; - } - -#ifdef WIN32 - /* Mingw / Windows don't have many signals (AFAIK) */ - switch (sig) { - case SIGINT: - /* This does a Ctrl+C equiv */ - if (!GenerateConsoleCtrlEvent(CTRL_C_EVENT, pid)) { - errno = ESRCH; - return -1; - } - break; - - case SIGTERM: - /* Since TerminateProcess is closer to SIG_KILL, we do - * a Ctrl+Break equiv which is more pleasant like the - * good old unix SIGTERM/HUP - */ - if (!GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT, pid)) { - errno = ESRCH; - return -1; - } - break; - - default: - { - HANDLE proc; - proc = OpenProcess(PROCESS_TERMINATE, FALSE, pid); - if (!proc) { - errno = ESRCH; /* Not entirely accurate, but close enough */ - return -1; - } - - /* - * TerminateProcess is more or less equiv to SIG_KILL, in that - * a process can't trap / block it - */ - if (sig != 0 && !TerminateProcess(proc, sig)) { - errno = ESRCH; - return -1; - } - CloseHandle(proc); - } - } - return 0; -#else - return kill(pid, sig); -#endif -} - - #ifdef HAVE_GETPWUID_R enum { VIR_USER_ENT_DIRECTORY, diff --git a/src/util/util.h b/src/util/util.h index 838bff7..c63f5fb 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -225,7 +225,6 @@ static inline int getgid (void) { return 0; } char *virGetHostname(virConnectPtr conn); -int virProcessKill(pid_t pid, int sig); char *virGetUserDirectory(uid_t uid); char *virGetUserName(uid_t uid); diff --git a/src/util/virprocess.c b/src/util/virprocess.c new file mode 100644 index 0000000..e7db68f --- /dev/null +++ b/src/util/virprocess.c @@ -0,0 +1,84 @@ +/* + * virprocess.c: interaction with processes + * + * Copyright (C) 2010-2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + + +#include <config.h> + +#include <signal.h> +#include <errno.h> + +#include "virprocess.h" + +/* send signal to a single process */ +int virProcessKill(pid_t pid, int sig) +{ + if (pid <= 1) { + errno = ESRCH; + return -1; + } + +#ifdef WIN32 + /* Mingw / Windows don't have many signals (AFAIK) */ + switch (sig) { + case SIGINT: + /* This does a Ctrl+C equiv */ + if (!GenerateConsoleCtrlEvent(CTRL_C_EVENT, pid)) { + errno = ESRCH; + return -1; + } + break; + + case SIGTERM: + /* Since TerminateProcess is closer to SIG_KILL, we do + * a Ctrl+Break equiv which is more pleasant like the + * good old unix SIGTERM/HUP + */ + if (!GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT, pid)) { + errno = ESRCH; + return -1; + } + break; + + default: + { + HANDLE proc; + proc = OpenProcess(PROCESS_TERMINATE, FALSE, pid); + if (!proc) { + errno = ESRCH; /* Not entirely accurate, but close enough */ + return -1; + } + + /* + * TerminateProcess is more or less equiv to SIG_KILL, in that + * a process can't trap / block it + */ + if (sig != 0 && !TerminateProcess(proc, sig)) { + errno = ESRCH; + return -1; + } + CloseHandle(proc); + } + } + return 0; +#else + return kill(pid, sig); +#endif +} diff --git a/src/util/virprocess.h b/src/util/virprocess.h new file mode 100644 index 0000000..b1000c6 --- /dev/null +++ b/src/util/virprocess.h @@ -0,0 +1,31 @@ +/* + * virprocess.h: interaction with processes + * + * Copyright (C) 2010-2012 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#ifndef __VIR_PROCESS_H__ +# define __VIR_PROCESS_H__ + +# include <sys/types.h> + +# include "internal.h" + +int virProcessKill(pid_t pid, int sig); + +#endif /* __VIR_PROCESS_H__ */ -- 1.8.3.1

From: "Daniel P. Berrange" <berrange@redhat.com> Since PIDs can be reused, polkit prefers to be given a (PID,start time) pair. If given a PID on its own, it will attempt to lookup the start time in /proc/pid/stat, though this is subject to races. It is safer if the client app resolves the PID start time itself, because as long as the app has the client socket open, the client PID won't be reused. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> (cherry picked from commit 979e9c56a7aadf2dcfbddd1abfbad594b78b4468) Signed-off-by: Eric Blake <eblake@redhat.com> Conflicts: src/libvirt_private.syms - not backported src/locking/lock_daemon.c - not backported src/rpc/virnetserverclient.c src/rpc/virnetsocket.c src/rpc/virnetsocket.h src/util/viridentity.h - not backported src/util/virprocess.c src/util/virprocess.h src/util/virstring.c src/util/virstring.h Most conflicts were contextual (this patch adds new functions, but upstream intermediate patches not backported here also added new features, and the resolution was picking out just the portions needed by this commit). virnetsocket.c also had slightly different locking semantics. --- daemon/remote.c | 12 +++-- src/rpc/virnetserverclient.c | 8 ++- src/rpc/virnetserverclient.h | 3 +- src/rpc/virnetsocket.c | 19 +++++-- src/rpc/virnetsocket.h | 3 +- src/util/virprocess.c | 118 +++++++++++++++++++++++++++++++++++++++++++ src/util/virprocess.h | 3 ++ src/util/virstring.c | 11 ++++ src/util/virstring.h | 2 + 9 files changed, 167 insertions(+), 12 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 435f663..c65e4e4 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -2119,6 +2119,7 @@ remoteDispatchAuthList(virNetServerPtr server ATTRIBUTE_UNUSED, uid_t callerUid; gid_t callerGid; pid_t callerPid; + unsigned long long timestamp; /* If the client is root then we want to bypass the * policykit auth to avoid root being denied if @@ -2126,7 +2127,7 @@ remoteDispatchAuthList(virNetServerPtr server ATTRIBUTE_UNUSED, */ if (auth == VIR_NET_SERVER_SERVICE_AUTH_POLKIT) { if (virNetServerClientGetUNIXIdentity(client, &callerUid, &callerGid, - &callerPid) < 0) { + &callerPid, ×tamp) < 0) { /* Don't do anything on error - it'll be validated at next * phase of auth anyway */ virResetLastError(); @@ -2554,6 +2555,7 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, pid_t callerPid = -1; gid_t callerGid = -1; uid_t callerUid = -1; + unsigned long long timestamp; const char *action; int status = -1; char *ident = NULL; @@ -2579,7 +2581,7 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, } if (virNetServerClientGetUNIXIdentity(client, &callerUid, &callerGid, - &callerPid) < 0) { + &callerPid, ×tamp) < 0) { goto authfail; } @@ -2587,7 +2589,11 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, (long long) callerPid, callerUid); virCommandAddArg(cmd, "--process"); - virCommandAddArgFormat(cmd, "%lld", (long long) callerPid); + if (timestamp != 0) { + virCommandAddArgFormat(cmd, "%lld,%llu", (long long) callerPid, timestamp); + } else { + virCommandAddArgFormat(cmd, "%lld", (long long) callerPid); + } virCommandAddArg(cmd, "--allow-user-interaction"); if (virAsprintf(&ident, "pid:%lld,uid:%d", diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 3838136..73aa28d 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -448,16 +448,20 @@ int virNetServerClientGetFD(virNetServerClientPtr client) } int virNetServerClientGetUNIXIdentity(virNetServerClientPtr client, - uid_t *uid, gid_t *gid, pid_t *pid) + uid_t *uid, gid_t *gid, pid_t *pid, + unsigned long long *timestamp) { int ret = -1; virNetServerClientLock(client); if (client->sock) - ret = virNetSocketGetUNIXIdentity(client->sock, uid, gid, pid); + ret = virNetSocketGetUNIXIdentity(client->sock, + uid, gid, pid, + timestamp); virNetServerClientUnlock(client); return ret; } + bool virNetServerClientIsSecure(virNetServerClientPtr client) { bool secure = false; diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h index 154a160..9fc05c2 100644 --- a/src/rpc/virnetserverclient.h +++ b/src/rpc/virnetserverclient.h @@ -71,7 +71,8 @@ int virNetServerClientSetIdentity(virNetServerClientPtr client, const char *virNetServerClientGetIdentity(virNetServerClientPtr client); int virNetServerClientGetUNIXIdentity(virNetServerClientPtr client, - uid_t *uid, gid_t *gid, pid_t *pid); + uid_t *uid, gid_t *gid, pid_t *pid, + unsigned long long *timestamp); void virNetServerClientRef(virNetServerClientPtr client); diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index e82f375..1b4f894 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -828,31 +828,40 @@ int virNetSocketGetPort(virNetSocketPtr sock) int virNetSocketGetUNIXIdentity(virNetSocketPtr sock, uid_t *uid, gid_t *gid, - pid_t *pid) + pid_t *pid, + unsigned long long *timestamp) { struct ucred cr; socklen_t cr_len = sizeof(cr); + int ret = -1; + virMutexLock(&sock->lock); if (getsockopt(sock->fd, SOL_SOCKET, SO_PEERCRED, &cr, &cr_len) < 0) { virReportSystemError(errno, "%s", _("Failed to get client socket identity")); - virMutexUnlock(&sock->lock); - return -1; + goto cleanup; } + if (virProcessGetStartTime(cr.pid, timestamp) < 0) + goto cleanup; + *pid = cr.pid; *uid = cr.uid; *gid = cr.gid; + ret = 0; + +cleanup: virMutexUnlock(&sock->lock); - return 0; + return ret; } #else int virNetSocketGetUNIXIdentity(virNetSocketPtr sock ATTRIBUTE_UNUSED, uid_t *uid ATTRIBUTE_UNUSED, gid_t *gid ATTRIBUTE_UNUSED, - pid_t *pid ATTRIBUTE_UNUSED) + pid_t *pid ATTRIBUTE_UNUSED, + unsigned long long *timestamp ATTRIBUTE_UNUSED) { /* XXX Many more OS support UNIX socket credentials we could port to. See dbus ....*/ virReportSystemError(ENOSYS, "%s", diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h index 5ba7c8f..4eaf951 100644 --- a/src/rpc/virnetsocket.h +++ b/src/rpc/virnetsocket.h @@ -89,7 +89,8 @@ int virNetSocketGetPort(virNetSocketPtr sock); int virNetSocketGetUNIXIdentity(virNetSocketPtr sock, uid_t *uid, gid_t *gid, - pid_t *pid); + pid_t *pid, + unsigned long long *timestamp); int virNetSocketSetBlocking(virNetSocketPtr sock, bool blocking); diff --git a/src/util/virprocess.c b/src/util/virprocess.c index ac78f1d..f05f8f3 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -26,11 +26,19 @@ #include <errno.h> #include <sys/wait.h> +#ifdef __FreeBSD__ +# include <sys/param.h> +# include <sys/sysctl.h> +# include <sys/user.h> +#endif + +#include "viratomic.h" #include "virprocess.h" #include "virterror_internal.h" #include "memory.h" #include "logging.h" #include "util.h" +#include "virstring.h" #include "ignore-value.h" #define virReportError(code, ...) \ virReportErrorHelper(VIR_FROM_NONE, code, __FILE__, \ @@ -239,3 +247,113 @@ int virProcessKill(pid_t pid, int sig) return kill(pid, sig); #endif } + + +#ifdef __linux__ +/* + * Port of code from polkitunixprocess.c under terms + * of the LGPLv2+ + */ +int virProcessGetStartTime(pid_t pid, + unsigned long long *timestamp) +{ + char *filename = NULL; + char *buf = NULL; + char *tmp; + int ret = -1; + int len; + char **tokens = NULL; + + if (virAsprintf(&filename, "/proc/%llu/stat", + (unsigned long long)pid) < 0) { + virReportOOMError(); + return -1; + } + + if ((len = virFileReadAll(filename, 1024, &buf)) < 0) + goto cleanup; + + /* start time is the token at index 19 after the '(process name)' entry - since only this + * field can contain the ')' character, search backwards for this to avoid malicious + * processes trying to fool us + */ + + if (!(tmp = strrchr(buf, ')'))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot find start time in %s"), + filename); + goto cleanup; + } + tmp += 2; /* skip ') ' */ + if ((tmp - buf) >= len) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot find start time in %s"), + filename); + goto cleanup; + } + + tokens = virStringSplit(tmp, " ", 0); + + if (virStringListLength(tokens) < 20) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot find start time in %s"), + filename); + goto cleanup; + } + + if (virStrToLong_ull(tokens[19], + NULL, + 10, + timestamp) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse start time %s in %s"), + tokens[19], filename); + goto cleanup; + } + + ret = 0; + +cleanup: + virStringFreeList(tokens); + VIR_FREE(filename); + VIR_FREE(buf); + return ret; +} +#elif defined(__FreeBSD__) +int virProcessGetStartTime(pid_t pid, + unsigned long long *timestamp) +{ + struct kinfo_proc p; + int mib[4]; + size_t len = 4; + + sysctlnametomib("kern.proc.pid", mib, &len); + + len = sizeof(struct kinfo_proc); + mib[3] = pid; + + if (sysctl(mib, 4, &p, &len, NULL, 0) < 0) { + virReportSystemError(errno, "%s", + _("Unable to query process ID start time")); + return -1; + } + + *timestamp = (unsigned long long)p.ki_start.tv_sec; + + return 0; + +} +#else +int virProcessGetStartTime(pid_t pid, + unsigned long long *timestamp) +{ + static int warned = 0; + if (virAtomicIntInc(&warned) == 1) { + VIR_WARN("Process start time of pid %llu not available on this platform", + (unsigned long long)pid); + warned = true; + } + *timestamp = 0; + return 0; +} +#endif diff --git a/src/util/virprocess.h b/src/util/virprocess.h index 048a73c..30ca21f 100644 --- a/src/util/virprocess.h +++ b/src/util/virprocess.h @@ -39,4 +39,7 @@ virProcessWait(pid_t pid, int *exitstatus) int virProcessKill(pid_t pid, int sig); +int virProcessGetStartTime(pid_t pid, + unsigned long long *timestamp); + #endif /* __VIR_PROCESS_H__ */ diff --git a/src/util/virstring.c b/src/util/virstring.c index 1917e9a..92289eb 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -166,3 +166,14 @@ void virStringFreeList(char **strings) } VIR_FREE(strings); } + + +size_t virStringListLength(char **strings) +{ + size_t i = 0; + + while (strings && strings[i]) + i++; + + return i; +} diff --git a/src/util/virstring.h b/src/util/virstring.h index a569fe0..d68ed2f 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -35,4 +35,6 @@ char *virStringJoin(const char **strings, void virStringFreeList(char **strings); +size_t virStringListLength(char **strings); + #endif /* __VIR_STRING_H__ */ -- 1.8.3.1

From: "Daniel P. Berrange" <berrange@redhat.com> The 'stats' variable was not initialized to NULL, so if some early validation of the RPC call fails, it is possible to jump to the 'cleanup' label and VIR_FREE an uninitialized pointer. This is a security flaw, since the API can be called from a readonly connection which can trigger the validation checks. This was introduced in release v0.9.1 onwards by commit 158ba8730e44b7dd07a21ab90499996c5dec080a Author: Daniel P. Berrange <berrange@redhat.com> Date: Wed Apr 13 16:21:35 2011 +0100 Merge all returns paths from dispatcher into single path Signed-off-by: Daniel P. Berrange <berrange@redhat.com> (cherry picked from commit e7f400a110e2e3673b96518170bfea0855dd82c0) Conflicts: daemon/remote.c - context --- daemon/remote.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/daemon/remote.c b/daemon/remote.c index 2cdb079..f127eaa 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1061,7 +1061,7 @@ remoteDispatchDomainMemoryStats(virNetServerPtr server ATTRIBUTE_UNUSED, remote_domain_memory_stats_ret *ret) { virDomainPtr dom = NULL; - struct _virDomainMemoryStat *stats; + struct _virDomainMemoryStat *stats = NULL; int nr_stats, i; int rv = -1; struct daemonClientPrivate *priv = -- 1.8.3.1