xenstore: deprecating but \-quoting binary data.

Presently it's not clear what the allowable character set is for
values in xenstore.  The current command-line tools just pass values
to printf("%s",...) so implicitly assume that it's 7-bit printable
ASCII (since the interpretation of 8-bit characters would be unclear).
However there are rumours of programs which dump binary data into
xenstore and/or bugs involving nul bytes being added to the ends of
xenstore values (and even of some drivers insisting on a spurious
nul).

There isn't all that much useful documentation about xenstore.  There
is a doc detailing which xenstore keys may be used and what their
meanings are (interface.tex) but it is very out of date, amongst other
reasons because it's in format which is not very easy to update when
adding functionality to the code and because there is no way to check
programs' behaviour in xenstore against the spec.  I think the
xenstore part of interface.tex should be replaced with a new document
in a simpler format, which should amonst other things be sufficiently
machine-readable that automatic testing could reveal at least basic
out-of-spec behaviours like setting or using undocumented keys.

This new document ought to specify the allowable character set of both
keys and values, and ought to specify the xenstored protocol as well.

It seems to me that the appropriate character set for xenstore values
is 7-bit printing ASCII (0x20..0x7e).  Values should not have a
trailing nul byte `on the wire' but of course the xs library interface
should continue to add an additional nul beyond the quoted length for
the convenience of callers.

That is consistent with nearly all of the existing uses and makes the
whole system much more tractable compared to an explicit expectation
that binary data will be stored.  (For example, if we like binary data
in xenstore, why are uuids represented in their printable hex
encoding?)  xenstore data is supposedly non-performance-critical
metadata for use by control plane machinery so the overhead of
printing and parsing text strings is hardly a problem.

Applications which set binary values should be deprecated but to avoid
breaking those applications xenstored should continue indefinitely to
be binary-transparent.

Under these circumstances it can only be regarded as a bug that the
current command-line tools are lossy in the presence of binary data.
Not only does this make them break for those now-deprecated uses, but
it also prevents them from being used to detect and debug problems
relating to the exact byte strings being recorded in xenstore.

As a first step towards the utopia I describe above, this patch
causes xenstore-read and -ls to \-escape the values of xenstore
keys, and xenstore-write to un-\-escape them.  The escaping is a
subset of that permitted by C89; only \t \r \n \\ and hex and octal
are used and recognised.  (So no \f, \a etc.)

This change will not change the representation by these tools of
values which contain only 7-bit printing ASCII characters unless they
contain \'s.

Values which contain \'s will need to be quoted on entry and dequoted
on exit if being manipulated by xenstore-*.  The only values likely to
be affected are paths in Windows guest filesystems and in practice we
believe that any such filename which is actually relevant to anything
will be set other than via xenstore-write.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

diff --git a/tools/xenstore/xenstore_client.c b/tools/xenstore/xenstore_client.c
index c34dcbfc70f15944a2f3b704a9dbbb27552961b5..0f36356233820fb944eef784df9bd8b6c0ed6eee 100644 (file)
--- a/tools/xenstore/xenstore_client.c
+++ b/tools/xenstore/xenstore_client.c
@@ -138,19 +138,25 @@ perform(int optind, int argc, char **argv, struct xs_handle *xsh,
 {
     while (optind < argc) {
 #if defined(CLIENT_read)
-       char *val = xs_read(xsh, xth, argv[optind], NULL);
+       struct expanding_buffer ebuf;
+       unsigned len;
+       char *val = xs_read(xsh, xth, argv[optind], &len);
        if (val == NULL) {
            warnx("couldn't read path %s", argv[optind]);
            return 1;
        }
        if (prefix)
            output("%s: ", argv[optind]);
-       output("%s\n", val);
+       output("%s\n", sanitise_value(&ebuf, val, len));
        free(val);
        optind++;
 #elif defined(CLIENT_write)
-       if (!xs_write(xsh, xth, argv[optind], argv[optind + 1],
-                      strlen(argv[optind + 1]))) {
+       struct expanding_buffer ebuf;
+       char *val_spec = argv[optind + 1];
+       unsigned len;
+       expanding_buffer_ensure(&ebuf, strlen(val_spec)+1);
+       unsanitise_value(ebuf.buf, &len, val_spec);
+       if (!xs_write(xsh, xth, argv[optind], ebuf.buf, len)) {
            warnx("could not write path %s", argv[optind]);
            return 1;
        }
@@ -179,9 +185,10 @@ perform(int optind, int argc, char **argv, struct xs_handle *xsh,
             slash = strrchr(p, '/');
             if (slash) {
                 char *val;
+                unsigned len;
                 *slash = '\0';
-                val = xs_read(xsh, xth, p, NULL);
-                if (val && strlen(val) == 0) {
+                val = xs_read(xsh, xth, p, &len);
+                if (val && len == 0) {
                     unsigned int num;
                     char ** list = xs_directory(xsh, xth, p, &num);
 

diff --git a/tools/xenstore/xs_lib.c b/tools/xenstore/xs_lib.c
index 77cd64f3cad8f011bae18d12b7802102cd47beb1..45ca5df4025220610a65ad8851e8e2cae6bfd76f 100644 (file)
--- a/tools/xenstore/xs_lib.c
+++ b/tools/xenstore/xs_lib.c
@@ -22,6 +22,7 @@
 #include <string.h>
 #include <stdlib.h>
 #include <errno.h>
+#include <assert.h>
 #include "xs_lib.h"
 
 /* Common routines for the Xen store daemon and client library. */
@@ -181,3 +182,114 @@ unsigned int xs_count_strings(const char *strings, unsigned int len)
 
        return num;
 }
+
+char *expanding_buffer_ensure(struct expanding_buffer *ebuf, int min_avail)
+{
+       int want;
+       char *got;
+
+       if (ebuf->avail >= min_avail)
+               return ebuf->buf;
+
+       if (min_avail >= INT_MAX/3)
+               return 0;
+
+       want = ebuf->avail + min_avail + 10;
+       got = realloc(ebuf->buf, want);
+       if (!got)
+               return 0;
+
+       ebuf->buf = got;
+       ebuf->avail = want;
+       return ebuf->buf;
+}
+
+char *sanitise_value(struct expanding_buffer *ebuf,
+                    const char *val, unsigned len)
+{
+       int used, remain, c;
+       unsigned char *ip;
+
+#define ADD(c) (ebuf->buf[used++] = (c))
+#define ADDF(f,c) (used += sprintf(ebuf->buf+used, (f), (c)))
+
+       assert(len < INT_MAX/5);
+
+       ip = (unsigned char *)val;
+       used = 0;
+       remain = len;
+
+       if (!expanding_buffer_ensure(ebuf, remain + 1))
+               return NULL;
+
+       while (remain-- > 0) {
+               c= *ip++;
+
+               if (c >= ' ' && c <= '~' && c != '\\') {
+                       ADD(c);
+                       continue;
+               }
+
+               if (!expanding_buffer_ensure(ebuf, used + remain + 5))
+                       /* for "<used>\\nnn<remain>\0" */
+                       return 0;
+
+               ADD('\\');
+               switch (c) {
+               case '\t':  ADD('t');   break;
+               case '\n':  ADD('n');   break;
+               case '\r':  ADD('r');   break;
+               case '\\':  ADD('\\');  break;
+               default:
+                       if (c < 010) ADDF("%03o", c);
+                       else         ADDF("x%02x", c);
+               }
+       }
+
+       ADD(0);
+       assert(used <= ebuf->avail);
+       return ebuf->buf;
+
+#undef ADD
+#undef ADDF
+}
+
+void unsanitise_value(char *out, unsigned *out_len_r, const char *in)
+{
+       const char *ip;
+       char *op;
+       unsigned c;
+       int n;
+
+       for (ip = in, op = out; (c = *ip++); *op++ = c) {
+               if (c == '\\') {
+                       c = *ip++;
+
+#define GETF(f) do {                                   \
+                       n = 0;                          \
+                        sscanf(ip, f "%n", &c, &n);    \
+                       ip += n;                        \
+               } while (0)
+
+                       switch (c) {
+                       case 't':              c= '\t';            break;
+                       case 'n':              c= '\n';            break;
+                       case 'r':              c= '\r';            break;
+                       case '\\':             c= '\\';            break;
+                       case 'x':                    GETF("%2x");  break;
+                       case '0': case '4':
+                       case '1': case '5':
+                       case '2': case '6':
+                       case '3': case '7':    --ip; GETF("%3o");  break;
+                       case 0:                --ip;               break;
+                       default:;
+                       }
+#undef GETF
+               }
+       }
+
+       *op = 0;
+
+       if (out_len_r)
+               *out_len_r = op - out;
+}

diff --git a/tools/xenstore/xs_lib.h b/tools/xenstore/xs_lib.h
index f56b7c3e2958a4e4a010967ccf52fb4320899286..bea010d78f125dc2a9e186fb84c81941d03a9882 100644 (file)
--- a/tools/xenstore/xs_lib.h
+++ b/tools/xenstore/xs_lib.h
@@ -67,4 +67,19 @@ bool xs_perm_to_string(const struct xs_permissions *perm,
 /* Given a string and a length, count how many strings (nul terms). */
 unsigned int xs_count_strings(const char *strings, unsigned int len);
 
+/* Sanitising (quoting) possibly-binary strings. */
+struct expanding_buffer {
+       char *buf;
+       int avail;
+};
+
+/* Ensure that given expanding buffer has at least min_avail characters. */
+char *expanding_buffer_ensure(struct expanding_buffer *, int min_avail);
+
+/* sanitise_value() may return NULL if malloc fails. */
+char *sanitise_value(struct expanding_buffer *, const char *val, unsigned len);
+
+/* *out_len_r on entry is ignored; out must be at least strlen(in)+1 bytes. */
+void unsanitise_value(char *out, unsigned *out_len_r, const char *in);
+
 #endif /* _XS_LIB_H */

diff --git a/tools/xenstore/xsls.c b/tools/xenstore/xsls.c
index 71b76975bf3afe1655b96ebc2560ab9fe69d40df..061feebd57fd3013421300407ea019d4a12ebc25 100644 (file)
--- a/tools/xenstore/xsls.c
+++ b/tools/xenstore/xsls.c
@@ -19,6 +19,7 @@ static int desired_width = 60;
 
 void print_dir(struct xs_handle *h, char *path, int cur_depth, int show_perms)
 {
+    static struct expanding_buffer ebuf;
     char **e;
     char newpath[STRING_MAX], *val;
     int newpath_len;
@@ -60,11 +61,13 @@ void print_dir(struct xs_handle *h, char *path, int cur_depth, int show_perms)
         }
         else {
             if (max_width < (linewid + len + TAG_LEN)) {
-                printf(" = \"%.*s...\"",
-                       (int)(max_width - TAG_LEN - linewid), val);
+                printf(" = \"%.*s\\...\"",
+                       (int)(max_width - TAG_LEN - linewid),
+                      sanitise_value(&ebuf, val, len));
             }
             else {
-                linewid += printf(" = \"%s\"", val);
+                linewid += printf(" = \"%s\"",
+                                 sanitise_value(&ebuf, val, len));
                 if (show_perms) {
                     putchar(' ');
                     for (linewid++;