"Richard W.M. Jones" <rjones(a)redhat.com&gt; wrote: Thanks for the quick feedback. Yeah, that's probably cleaner. And if someone does e.g. virsh define DIR_NAME on a system that lets you read directories, they won't be too surprised that the contents of the directory is invalid XML. Here's the updated patch: Don't fail to read a file because it's non-seekable (e.g., a pipe). * src/util.c (fread_file_lim): New function. (__virFileReadAll): Use fread_file_lim, rather than requiring that stat.st_size provide a usable file size. * tests/read-non-seekable: New test, for the above. * tests/Makefile.am (test_scripts): Add read-non-seekable. * tests/test-lib.sh (mkfifo_or_skip_): New helper function. Signed-off-by: Jim Meyering <meyering(a)redhat.com&gt; --- src/util.c | 99 +++++++++++++++++++++++++++++++--------------- tests/Makefile.am | 1 + tests/read-non-seekable | 47 ++++++++++++++++++++++ tests/test-lib.sh | 12 ++++++ 4 files changed, 127 insertions(+), 32 deletions(-) create mode 100755 tests/read-non-seekable diff --git a/src/util.c b/src/util.c index 0667780..801f615 100644 --- a/src/util.c +++ b/src/util.c @@ -49,6 +49,10 @@ #include "util-lib.c" +#ifndef MIN +# define MIN(a, b) ((a) < (b) ? (a) : (b)) +#endif + #define MAX_ERROR_LEN 1024 #define TOLOWER(Ch) (isupper (Ch) ? tolower (Ch) : (Ch)) @@ -283,14 +287,64 @@ virExecNonBlock(virConnectPtr conn, #endif /* __MINGW32__ */ +/* Like gnulib's fread_file, but read no more than the specified maximum + number of bytes. If the length of the input is <= max_len, and + upon error while reading that data, it works just like fread_file. */ +static char * +fread_file_lim (FILE *stream, size_t max_len, size_t *length) +{ + char *buf = NULL; + size_t alloc = 0; + size_t size = 0; + int save_errno; + + for (;;) { + size_t count; + size_t requested; + + if (size + BUFSIZ + 1 > alloc) { + char *new_buf; + + alloc += alloc / 2; + if (alloc < size + BUFSIZ + 1) + alloc = size + BUFSIZ + 1; + + new_buf = realloc (buf, alloc); + if (!new_buf) { + save_errno = errno; + break; + } + + buf = new_buf; + } + + /* Ensure that (size + requested <= max_len); */ + requested = MIN (size < max_len ? max_len - size : 0, + alloc - size - 1); + count = fread (buf + size, 1, requested, stream); + size += count; + + if (count != requested || requested == 0) { + save_errno = errno; + if (ferror (stream)) + break; + buf[size] = '\0'; + *length = size; + return buf; + } + } + + free (buf); + errno = save_errno; + return NULL; +} -int __virFileReadAll(const char *path, - int maxlen, - char **buf) +int __virFileReadAll(const char *path, int maxlen, char **buf) { FILE *fh; - struct stat st; int ret = -1; + size_t len; + char *s; if (!(fh = fopen(path, "r"))) { virLog("Failed to open file '%s': %s", @@ -298,39 +352,21 @@ int __virFileReadAll(const char *path, goto error; } - if (fstat(fileno(fh), &st) < 0) { - virLog("Failed to stat file '%s': %s", - path, strerror(errno)); + s = fread_file_lim(fh, maxlen+1, &len); + if (s == NULL) { + virLog("Failed to read '%s': %s", path, strerror (errno)); goto error; } - if (S_ISDIR(st.st_mode)) { - virLog("Ignoring directory '%s'", path); + if (len > maxlen || (int)len != len) { + free(s); + virLog("File '%s' is too large %d, max %d", + path, (int)len, maxlen); goto error; } - if (st.st_size > maxlen) { - virLog("File '%s' is too large %d, max %d", path, (int)st.st_size, maxlen); - goto error; - } - - *buf = malloc(st.st_size + 1); - if (*buf == NULL) { - virLog("Failed to allocate data"); - goto error; - } - - if ((ret = fread(*buf, st.st_size, 1, fh)) != 1) { - free(*buf); - *buf = NULL; - virLog("Failed to read config file '%s': %s", - path, strerror(errno)); - goto error; - } - - (*buf)[st.st_size] = '\0'; - - ret = st.st_size; + *buf = s; + ret = len; error: if (fh) @@ -739,6 +775,5 @@ virParseMacAddr(const char* str, unsigned char *addr) * indent-tabs-mode: nil * c-indent-level: 4 * c-basic-offset: 4 - * tab-width: 4 * End: */ diff --git a/tests/Makefile.am b/tests/Makefile.am index 901e88a..ca12b84 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -46,6 +46,7 @@ noinst_PROGRAMS = xmlrpctest xml2sexprtest sexpr2xmltest virshtest conftest \ test_scripts = \ daemon-conf \ int-overflow \ + read-non-seekable \ vcpupin EXTRA_DIST += $(test_scripts) diff --git a/tests/read-non-seekable b/tests/read-non-seekable new file mode 100755 index 0000000..9bb4a21 --- /dev/null +++ b/tests/read-non-seekable @@ -0,0 +1,47 @@ +#!/bin/sh +# ensure that certain file-reading commands can handle non-seekable files + +# Copyright (C) 2008 Free Software Foundation, Inc. + +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. + +# This program 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 General Public License for more details. + +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>;. + +if test "$VERBOSE" = yes; then + set -x + virsh --version +fi + +. $srcdir/test-lib.sh + +fail=0 + +cat <<\EOF > dom +<domain type='test' id='2'> + <name>t2</name> + <uuid>004b96e1-2d78-c30f-5aa5-000000000000</uuid> + <memory>8388608</memory> + <vcpu>2</vcpu> + <on_reboot>restart</on_reboot> + <on_poweroff>destroy</on_poweroff> + <on_crash>restart</on_crash> +</domain> +EOF + +virsh -c test:///default define dom > /dev/null || fail=1 + +mkfifo_or_skip_ fifo +cat dom > fifo & + +virsh -c test:///default define fifo > /dev/null || fail=1 + +(exit $fail); exit $fail diff --git a/tests/test-lib.sh b/tests/test-lib.sh index cdbea5d..a007109 100644 --- a/tests/test-lib.sh +++ b/tests/test-lib.sh @@ -120,6 +120,18 @@ skip_if_root_() { uid_is_privileged_ && skip_test_ "must be run as non-root"; } error_() { echo "$0: $@" 1>&2; (exit 1); exit 1; } framework_failure() { error_ 'failure in testing framework'; } +mkfifo_or_skip_() +{ + test $# = 1 || framework_failure + if ! mkfifo "$1"; then + # Make an exception of this case -- usually we interpret framework-creation + # failure as a test failure. However, in this case, when running on a SunOS + # system using a disk NFS mounted from OpenBSD, the above fails like this: + # mkfifo: cannot make fifo `fifo-10558': Not owner + skip_test_ 'NOTICE: unable to create test prerequisites' + fi +} + test_dir_=$(pwd) this_test_() { echo "./$0" | sed 's,.*/,,'; } -- 1.5.5.rc2.7.g144a