utils, random: fix slice(), and make the TESTING RNG unbuildable in release

include/dogecoin/utils.h

@@ -83,6 +83,9 @@ LIBDOGECOIN_API void print_bits(size_t const size, void const* ptr);
 LIBDOGECOIN_API void prepend(char* s, const char* t);
 LIBDOGECOIN_API void append(char* s, char* t);
 LIBDOGECOIN_API char* concat(char* prefix, char* suffix);
+/* Copy str[start, end) into result, NUL-terminated. result must have room for
+   (end - start) + 1 bytes -- the function takes no size for it. Yields an empty
+   string if end <= start or start is past the end of str. */
 LIBDOGECOIN_API void slice(const char *str, char *result, size_t start, size_t end);
 LIBDOGECOIN_API void replace_last_after_delim(const char *str, char* delim, char* replacement);
 LIBDOGECOIN_API void text_to_hex(char* in, char* out);

src/random.c

@@ -149,6 +149,19 @@ dogecoin_bool dogecoin_random_bytes(uint8_t* buf, uint32_t len, const uint8_t up
     }
 
 #ifdef TESTING
+/*
+ * This branch replaces the CSPRNG with srand(time(NULL)) and rand(), which
+ * would make every key this library generates predictable from the wall clock.
+ * Nothing in the build system defines TESTING -- not CMakeLists.txt, not
+ * configure.ac -- so it is unreachable today. The guard is here so that it
+ * stays unreachable in anything shipped: a release build sets NDEBUG, and
+ * combining the two must fail loudly at compile time rather than quietly
+ * produce guessable keys.
+ */
+#ifdef NDEBUG
+#error "TESTING replaces the RNG with srand()/rand(); it must never be enabled in a release build"
+#endif
+
 void dogecoin_random_init_internal(void)
     {
     srand(time(NULL));

src/utils.c

@@ -680,7 +680,32 @@ char* concat(char* prefix, char* suffix) {
 
 void slice(const char *str, char *result, size_t start, size_t end)
 {
-    strncpy(result, str + start, end - start);
+    size_t slen, len;
+
+    /*
+     * Two problems with the strncpy this replaces. It never terminated:
+     * strncpy writes no NUL when it copies its full count, and the count here
+     * was always exactly the number of bytes copied, so result was left
+     * unterminated for every input. And `end - start` is size_t arithmetic, so
+     * end < start wrapped to a length near SIZE_MAX.
+     *
+     * result must have room for (end - start) + 1 bytes. The function takes no
+     * size for it, so that is the contract; see the header.
+     */
+    if (!str || !result) {
+        return;
+    }
+    slen = strlen(str);
+    if (end <= start || start >= slen) {
+        result[0] = '\0';
+        return;
+    }
+    if (end > slen) {
+        end = slen;
+    }
+    len = end - start;
+    memcpy(result, str + start, len);
+    result[len] = '\0';
 }
 
 void remove_substr(char *string, char *sub) {

test/unittester.c

@@ -97,6 +97,7 @@ extern void test_invalid_tx_deser();
 extern void test_tx_sign();
 extern void test_scripts();
 extern void test_utils();
+extern void test_utils_slice();
 extern void test_vector();
 extern void test_qr();
 
@@ -232,6 +233,7 @@ int main()
     u_run_test(test_script_parse);
     u_run_test(test_script_op_codeseperator);
     u_run_test(test_utils);
+    u_run_test(test_utils_slice);
     u_run_test(test_vector);
     u_run_test(test_qr);
 

test/utils_tests.c

@@ -132,3 +132,56 @@ void test_dit()
         debug_print("%s", "DIT test: disabled (DIT not supported)\n");
     }
 }
+
+
+/*
+ * slice() is LIBDOGECOIN_API and had no in-tree callers, which is why neither
+ * defect was noticed:
+ *
+ *   strncpy(result, str + start, end - start);
+ *
+ * strncpy writes no terminator when it copies its full count, and the count was
+ * always exactly the number of bytes copied -- so result came back unterminated
+ * for every input, and a caller printing it read past the buffer. Separately,
+ * `end - start` is size_t arithmetic, so end < start wrapped to a length near
+ * SIZE_MAX and strncpy ran until it hit the source NUL or a fault.
+ */
+void test_utils_slice()
+{
+    char buf[64];
+
+    /* Ordinary slice, and it must be terminated. */
+    memset(buf, 'X', sizeof(buf));
+    slice("abcdefghij", buf, 2, 5);
+    u_assert_str_eq(buf, "cde");
+    u_assert_int_eq((int)strlen(buf), 3);
+
+    /* Whole string. */
+    memset(buf, 'X', sizeof(buf));
+    slice("abcdef", buf, 0, 6);
+    u_assert_str_eq(buf, "abcdef");
+
+    /* end < start used to wrap to a huge count. Must yield an empty string. */
+    memset(buf, 'X', sizeof(buf));
+    slice("abcdefghij", buf, 5, 2);
+    u_assert_str_eq(buf, "");
+
+    /* end == start is an empty slice, not a one-byte read. */
+    memset(buf, 'X', sizeof(buf));
+    slice("abcdefghij", buf, 3, 3);
+    u_assert_str_eq(buf, "");
+
+    /* start past the end of the source must not read past it. */
+    memset(buf, 'X', sizeof(buf));
+    slice("abc", buf, 10, 20);
+    u_assert_str_eq(buf, "");
+
+    /* end past the end of the source clamps rather than over-reading. */
+    memset(buf, 'X', sizeof(buf));
+    slice("abc", buf, 1, 99);
+    u_assert_str_eq(buf, "bc");
+
+    /* NULL arguments are refused rather than dereferenced. */
+    slice(NULL, buf, 0, 1);
+    slice("abc", NULL, 0, 1);
+}