libkvs: do not store strings with \0 term

Problem: the reasons no longer exist for storing
string/json values in the KVS with their terminating
NULL byte, and this complicates the implementation
of atomic append.

Skip storing the NULL byte, and change lookup_get()
et al to rely on the fact that treeobj_decode_val()
pads the space after the returned data with a NULL byte.

Use json_loadb() instead of _loads() when decoding
a JSON value.

Update unit and sharness tests.

Fix #1261

src/common/libkvs/kvs_lookup.c

@@ -165,14 +165,6 @@ flux_future_t *flux_kvs_lookupat (flux_t *h, int flags, const char *key,
     return f;
 }
 
-static bool value_is_string (const char *s, int len)
-{
-    if (len < 1 || s[len - 1] != '\0')
-        return false;
-    else
-        return true;
-}
-
 static int decode_treeobj (flux_future_t *f, json_t **treeobj)
 {
     json_t *obj;
@@ -204,10 +196,7 @@ int flux_kvs_lookup_get (flux_future_t *f, const char **value)
                                               &ctx->val_len) < 0)
             return -1;
         ctx->val_valid = true;
-    }
-    if (ctx->val_data && !value_is_string (ctx->val_data, ctx->val_len)) {
-        errno = EINVAL;
-        return -1;
+        // N.B. val_data includes xtra 0 byte term not reflected in val_len
     }
     if (value)
         *value = ctx->val_data;
@@ -256,11 +245,7 @@ int flux_kvs_lookup_get_unpack (flux_future_t *f, const char *fmt, ...)
         ctx->val_valid = true;
     }
     if (!ctx->val_obj) {
-        if (!value_is_string (ctx->val_data, ctx->val_len)) {
-            errno = EINVAL;
-            return -1;
-        }
-        if (!(ctx->val_obj = json_loads (ctx->val_data,
+        if (!(ctx->val_obj = json_loadb (ctx->val_data, ctx->val_len,
                                          JSON_DECODE_ANY, NULL))) {
             errno = EINVAL;
             return -1;

src/common/libkvs/kvs_txn.c

@@ -176,7 +176,7 @@ int flux_kvs_txn_put (flux_kvs_txn_t *txn, int flags,
     }
     if (validate_flags (flags, 0) < 0)
         goto error;
-    if (!(dirent = treeobj_create_val (value, value ? strlen (value) + 1 : 0)))
+    if (!(dirent = treeobj_create_val (value, value ? strlen (value) : 0)))
         goto error;
     if (append_op_to_txn (txn, flags, key, dirent) < 0)
         goto error;
@@ -214,7 +214,7 @@ int flux_kvs_txn_vpack (flux_kvs_txn_t *txn, int flags,
         goto error;
     }
     json_decref (val);
-    if (!(dirent = treeobj_create_val (s, strlen (s) + 1))) {
+    if (!(dirent = treeobj_create_val (s, strlen (s)))) {
         free (s);
         goto error;
     }

src/common/libkvs/kvs_watch.c

@@ -259,7 +259,7 @@ static flux_future_t *kvs_watch_rpc (flux_t *h, const char *key,
 /* val will be one of three things:
  * 1) JSON_NULL, set json_str to string-encoded object (NULL)
  * 2) RFC 11 dir object, set json_str to string-encoded object
- * 3) RFC 11 val object, unbase64, verify NULL termination, set json_str
+ * 3) RFC 11 val object, unbase64, set json_str
  * The caller must free returned json_str (if any)
  */
 static int decode_val_object (json_t *val, char **json_str)
@@ -285,11 +285,6 @@ static int decode_val_object (json_t *val, char **json_str)
             goto error;
         if (treeobj_decode_val (val, (void **)&s, &len) < 0)
             goto error;
-        if (s[len - 1] != '\0') {
-            free (s);
-            errno = EPROTO;
-            goto error;
-        }
     }
     else {
         errno = EPROTO;
@@ -348,7 +343,7 @@ int flux_kvs_watch_once (flux_t *h, const char *key, char **valp)
     }
     val_in = *valp;
     if (val_in) {
-        if (!(xval_obj = treeobj_create_val (val_in, strlen (val_in) + 1)))
+        if (!(xval_obj = treeobj_create_val (val_in, strlen (val_in))))
             goto done;
         if (!(xval_str = treeobj_encode (xval_obj)))
             goto done;

src/common/libkvs/test/kvs_txn.c

@@ -51,12 +51,7 @@ int check_int_value (json_t *dirent, int expected)
         diag ("%s: initial base64 decode failed", __FUNCTION__);
         return -1;
     }
-    if (len == 0 || data[len - 1] != '\0') {
-        diag ("%s: data not null terminated", __FUNCTION__);
-        free (data);
-        return -1;
-    }
-    if (!(val = json_loads (data, JSON_DECODE_ANY, NULL))) {
+    if (!(val = json_loadb (data, len, JSON_DECODE_ANY, NULL))) {
         diag ("%s: couldn't decode JSON", __FUNCTION__);
         free (data);
         return -1;
@@ -90,12 +85,7 @@ int check_string_value (json_t *dirent, const char *expected)
         diag ("%s: initial base64 decode failed", __FUNCTION__);
         return -1;
     }
-    if (len == 0 || data[len - 1] != '\0') {
-        diag ("%s: data not null terminated", __FUNCTION__);
-        free (data);
-        return -1;
-    }
-    if (!(val = json_loads (data, JSON_DECODE_ANY, NULL))) {
+    if (!(val = json_loadb (data, len, JSON_DECODE_ANY, NULL))) {
         diag ("%s: couldn't decode JSON", __FUNCTION__);
         free (data);
         return -1;

t/t1000-kvs.t

@@ -420,11 +420,11 @@ test_expect_success 'kvs: zero-length value NOT handled by get --json' '
 	flux kvs put --raw $DIR.a= &&
 	test_must_fail flux kvs get --json $DIR.a
 '
-test_expect_success 'kvs: zero-length value NOT made by put with no options' '
+test_expect_success 'kvs: zero-length value is made by put with no options' '
 	flux kvs unlink -Rf $DIR &&
 	flux kvs put $DIR.a= &&
-	flux kvs get --raw $DIR.a >onenull.output &&
-	test_must_fail diff -q /dev/null onenull.output
+	flux kvs get --raw $DIR.a >empty3.output &&
+	test_cmp /dev/null empty3.output
 '
 test_expect_success 'kvs: zero-length value does not cause dir to fail' '
 	flux kvs unlink -Rf $DIR &&
@@ -437,39 +437,6 @@ test_expect_success 'kvs: zero-length value does not cause ls -FR to fail' '
 	flux kvs ls -FR $DIR
 '
 
-#
-# empty string values
-#
-test_expect_success 'kvs: empty string value made by put with no options' '
-	flux kvs unlink -Rf $DIR &&
-	flux kvs put $DIR.a= &&
-	dd if=/dev/zero count=1 bs=1 of=empty.expected &&
-	flux kvs get --raw $DIR.a >empty.actual &&
-	test_cmp empty.expected empty.actual
-'
-test_expect_success 'kvs: empty string value can be retrieved by get' '
-	flux kvs unlink -Rf $DIR &&
-	flux kvs put $DIR.a= &&
-	echo >empty2.expected &&
-	flux kvs get $DIR.a >empty2.actual &&
-	test_cmp empty2.expected empty2.actual
-'
-test_expect_success 'kvs: empty string value NOT handled by get --json' '
-	flux kvs unlink -Rf $DIR &&
-	flux kvs put $DIR.a= &&
-	test_must_fail flux kvs get --json $DIR.a
-'
-test_expect_success 'kvs: empty string value does not cause dir to fail' '
-	flux kvs unlink -Rf $DIR &&
-	flux kvs put $DIR.a= &&
-	flux kvs dir $DIR | grep -q "a ="
-'
-test_expect_success 'kvs: empty string value does not cause ls -FR to fail' '
-	flux kvs unlink -Rf $DIR &&
-	flux kvs put $DIR.a= &&
-	flux kvs ls -FR $DIR | grep -q "a"
-'
-
 #
 # raw value tests
 #

t/t1002-kvs-extra.t

@@ -272,9 +272,8 @@ test_expect_success 'kvs: kvsdir_get_size works' '
 
 # kvs reads/writes of raw data to/from content store work
 
-# largevalhash includes string quotes and NUL char at end
 largeval="abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz"
-largevalhash="sha1-79da8e5c9dbe65c6460377d3f09b8f535ceb7d9d"
+largevalhash="sha1-0b22e9fecf9c832032fe976e67058df0322dcc5c"
 
 test_expect_success 'kvs: large put stores raw data into content store' '
 	flux kvs unlink -Rf $TEST &&