cborparser_dup_string: don't modify *buffer until success

We were returning from the function with the memory we had allocated and
freed, if the second iteration over the string produced a failure that
didn't happen on the first one. This can't happen with pure memory
buffers, but can happen with an external data source that fails to
produce the same contents twice.

I'm documenting that the values in all error conditions except for OOM
are undefined, so one mustn't attempt to use them, even to free. This
does not change behaviour of the library, just documents.

But this commit does make it clear the OOM condition will return a valid
`*buflen` and `next`, the latter of which is new behaviour with this
commit.

Fixes #258.

Signed-off-by: Thiago Macieira <[email protected]>

Author: thiagomacieira

src/cborparser_dup_string.c

@@ -49,16 +49,19 @@
  * undefined, so checking with \ref cbor_value_get_type or \ref
  * cbor_value_is_text_string is recommended.
  *
- * If \c malloc returns a NULL pointer, this function will return error
- * condition \ref CborErrorOutOfMemory.
- *
  * On success, \c *buffer will contain a valid pointer that must be freed by
- * calling \c free(). This is the case even for zero-length strings.
- *
- * The \a next pointer, if not null, will be updated to point to the next item
- * after this string. If \a value points to the last item, then \a next will be
+ * calling \c free(). This is the case even for zero-length strings. The \a
+ * next pointer, if not null, will be updated to point to the next item after
+ * this string. If \a value points to the last item, then \a next will be
  * invalid.
  *
+ * If \c malloc returns a NULL pointer, this function will return error
+ * condition \ref CborErrorOutOfMemory. In this case, \c *buflen should contain
+ * the number of bytes necessary to copy this string and \a value will be
+ * updated to point to the next element. On all other failure cases, the values
+ * contained in \c *buffer, \c *buflen and \a next are undefined and mustn't be
+ * used (for example, calling \c{free()}).
+ *
  * This function may not run in constant time (it will run in O(n) time on the
  * number of chunks). It requires constant memory (O(1)) in addition to the
  * malloc'ed block.
@@ -80,16 +83,19 @@
  * undefined, so checking with \ref cbor_value_get_type or \ref
  * cbor_value_is_byte_string is recommended.
  *
- * If \c malloc returns a NULL pointer, this function will return error
- * condition \ref CborErrorOutOfMemory.
- *
  * On success, \c *buffer will contain a valid pointer that must be freed by
- * calling \c free(). This is the case even for zero-length strings.
- *
- * The \a next pointer, if not null, will be updated to point to the next item
- * after this string. If \a value points to the last item, then \a next will be
+ * calling \c free(). This is the case even for zero-length strings. The \a
+ * next pointer, if not null, will be updated to point to the next item after
+ * this string. If \a value points to the last item, then \a next will be
  * invalid.
  *
+ * If \c malloc returns a NULL pointer, this function will return error
+ * condition \ref CborErrorOutOfMemory. In this case, \c *buflen should contain
+ * the number of bytes necessary to copy this string and \a value will be
+ * updated to point to the next element. On all other failure cases, the values
+ * contained in \c *buffer, \c *buflen and \a next are undefined and mustn't be
+ * used (for example, calling \c{free()}).
+ *
  * This function may not run in constant time (it will run in O(n) time on the
  * number of chunks). It requires constant memory (O(1)) in addition to the
  * malloc'ed block.
@@ -98,24 +104,28 @@
  */
 CborError _cbor_value_dup_string(const CborValue *value, void **buffer, size_t *buflen, CborValue *next)
 {
+    const CborValue it = *value;    // often value == next
     CborError err;
+    void *tmpbuf;
     cbor_assert(buffer);
     cbor_assert(buflen);
     *buflen = SIZE_MAX;
-    err = _cbor_value_copy_string(value, NULL, buflen, NULL);
+    err = _cbor_value_copy_string(&it, NULL, buflen, next);
     if (err)
         return err;
 
     ++*buflen;
-    *buffer = cbor_malloc(*buflen);
-    if (!*buffer) {
+    tmpbuf = cbor_malloc(*buflen);
+    if (!tmpbuf) {
         /* out of memory */
         return CborErrorOutOfMemory;
     }
-    err = _cbor_value_copy_string(value, *buffer, buflen, next);
+    err = _cbor_value_copy_string(&it, tmpbuf, buflen, next);
     if (err) {
-        cbor_free(*buffer);
+        /* This shouldn't have happened! We've iterated once. */
+        cbor_free(tmpbuf);
         return err;
     }
+    *buffer = tmpbuf;
     return CborNoError;
 }