build improvements, code audit

README.md

@@ -240,7 +240,18 @@ $ make check
 $ ./tools/coverage.sh
 ```
 
-The coverage report can then be viewed at `./src/lcov/src/index.html`. Patches
+For coverage with `clang`, you need to install `llvm-cov`, typically via the
+`llvm-<version>` package that corresponds to your `clang` version. Once
+installed, set the `GCOV` environment variable to the versioned `llvm-cov`
+binary name before running `./tools/coverage.sh`, e.g:
+
+```
+$ GCOV=llvm-cov-11 ./tools/coverage.sh clean
+$ make check
+$ GCOV=llvm-cov-11 ./tools/coverage.sh
+```
+
+The coverage report can be viewed at `./src/lcov/src/index.html`. Patches
 to increase the test coverage are welcome.
 
 ## Users of libwally-core

configure.ac

@@ -107,6 +107,7 @@ if test "x$debug" == "xyes"; then
     if test "x$coverage" == "xyes"; then
         AX_CHECK_COMPILE_FLAG([-fprofile-arcs -ftest-coverage], [AM_CFLAGS="$AM_CFLAGS -fprofile-arcs -ftest-coverage"])
         AX_CHECK_LINK_FLAG([-lgcov], [LDFLAGS="$LDFLAGS -lgcov"])
+        AX_CHECK_LINK_FLAG([--coverage], [LDFLAGS="$LDFLAGS --coverage"])
     fi
 else
     # Optimise and harden if we can

src/bip32.c

@@ -87,6 +87,7 @@ static int path_from_string_n(const char *str, size_t str_len,
                               size_t *written)
 {
     const bool allow_upper = flags & BIP32_FLAG_ALLOW_UPPER;
+    bool found_wildcard = false;
     size_t start, i = 0;
     uint64_t v;
 
@@ -130,6 +131,7 @@ static int path_from_string_n(const char *str, size_t str_len,
                 goto fail; /* Unknown character */
 
             /* Wildcard */
+            found_wildcard = true;
             if (!(flags & BIP32_FLAG_STR_WILDCARD))
                 goto fail; /* Wildcard not allowed, or previously seen */
             flags &= ~BIP32_FLAG_STR_WILDCARD;
@@ -154,6 +156,9 @@ static int path_from_string_n(const char *str, size_t str_len,
         ++*written;
     }
 
+    if (!found_wildcard && child_num != 0)
+        return WALLY_EINVAL; /* Child number given for a non-wildcard path */
+
     return *written ? WALLY_OK : WALLY_EINVAL;
 fail:
     if (written)

src/ccan/ccan/base64/base64.c

@@ -3,7 +3,9 @@
 
 #include <errno.h>
 #include <string.h>
+#if 0
 #include <assert.h>
+#endif
 #include <stdint.h>
 
 /**
@@ -14,8 +16,9 @@
  */
 static char sixbit_to_b64(const base64_maps_t *maps, const uint8_t sixbit)
 {
+#if 0
 	assert(sixbit <= 63);
-
+#endif
 	return maps->encode_map[(unsigned char)sixbit];
 }
 
@@ -78,8 +81,9 @@ void base64_encode_tail_using_maps(const base64_maps_t *maps, char dest[4],
 {
 	char longsrc[3] = { 0 };
 
+#if 0
 	assert(srclen <= 3);
-
+#endif
 	memcpy(longsrc, src, srclen);
 	base64_encode_triplet_using_maps(maps, dest, longsrc);
 	memset(dest+1+srclen, '=', 3-srclen);

src/ccan/ccan/crypto/ripemd160/ripemd160.c

@@ -10,7 +10,9 @@
 #include <ccan/endian/endian.h>
 #include <ccan/compiler/compiler.h>
 #include <stdbool.h>
+#if 0
 #include <assert.h>
+#endif
 #include <string.h>
 
 static void invalidate_ripemd160(struct ripemd160_ctx *ctx)
@@ -24,11 +26,13 @@ static void invalidate_ripemd160(struct ripemd160_ctx *ctx)
 
 static void check_ripemd160(struct ripemd160_ctx *ctx UNUSED)
 {
+#if 0
 #ifdef CCAN_CRYPTO_RIPEMD160_USE_OPENSSL
 	assert(ctx->c.num != -1U);
 #else
 	assert(ctx->bytes != -1ULL);
 #endif
+#endif
 }
 
 #ifdef CCAN_CRYPTO_RIPEMD160_USE_OPENSSL

src/ccan/ccan/crypto/sha256/sha256.c

@@ -10,7 +10,9 @@
 #include <ccan/endian/endian.h>
 #include <ccan/compiler/compiler.h>
 #include <stdbool.h>
+#if 0
 #include <assert.h>
+#endif
 #include <string.h>
 
 #ifdef CCAN_CRYPTO_SHA256_USE_OPENSSL
@@ -21,7 +23,9 @@ static void invalidate_sha256(struct sha256_ctx *ctx)
 
 static void check_sha256(struct sha256_ctx *ctx UNUSED)
 {
+#if 0
 	assert(ctx->c.md_len != 0);
+#endif
 }
 
 void sha256_init(struct sha256_ctx *ctx)
@@ -48,7 +52,9 @@ static void invalidate_sha256(struct sha256_ctx *ctx)
 
 static void check_sha256(struct sha256_ctx *ctx UNUSED)
 {
+#if 0
 	assert(ctx->bytes != (size_t)-1);
+#endif
 }
 
 static uint32_t Ch(uint32_t x, uint32_t y, uint32_t z)

src/ccan/ccan/crypto/sha512/sha512.c

@@ -10,7 +10,9 @@
 #include <ccan/endian/endian.h>
 #include <ccan/compiler/compiler.h>
 #include <stdbool.h>
+#if 0
 #include <assert.h>
+#endif
 #include <string.h>
 
 static void invalidate_sha512(struct sha512_ctx *ctx)
@@ -24,11 +26,13 @@ static void invalidate_sha512(struct sha512_ctx *ctx)
 
 static void check_sha512(struct sha512_ctx *ctx UNUSED)
 {
+#if 0
 #ifdef CCAN_CRYPTO_SHA512_USE_OPENSSL
 	assert(ctx->c.md_len != 0);
 #else
 	assert(ctx->bytes != (size_t)-1);
 #endif
+#endif
 }
 
 #ifdef CCAN_CRYPTO_SHA512_USE_OPENSSL

src/ccan/ccan/str/hex/hex.c

@@ -1,6 +1,8 @@
 /* CC0 license (public domain) - see LICENSE file for details */
 #include <ccan/str/hex/hex.h>
+#if 0
 #include <assert.h>
+#endif
 #include <stdio.h>
 #include <stdlib.h>
 

src/ctest/test_psbt.c

@@ -11,7 +11,9 @@
 
 /* Ignore test logging compiler warnings */
 #pragma GCC diagnostic ignored "-Wformat-nonliteral"
+#if defined(__clang__)
 #pragma clang diagnostic ignored "-Wformat-nonliteral"
+#endif
 
 static void fail(const char *fmt, ...)
 {

src/ctest/test_psbt_limits.c

@@ -11,7 +11,9 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <unistd.h>
+#if 0
 #include <assert.h>
+#endif
 #include <errno.h>
 
 #ifdef _WIN32
@@ -27,7 +29,9 @@
 
 /* Ignore test logging compiler warnings */
 #pragma GCC diagnostic ignored "-Wformat-nonliteral"
+#if defined(__clang__)
 #pragma clang diagnostic ignored "-Wformat-nonliteral"
+#endif
 
 static void fail(const char *fmt, ...)
 {

src/pullpush.c

@@ -1,7 +1,9 @@
 #include "internal.h"
 #include "script_int.h"
 
+#if 0
 #include <assert.h>
+#endif
 #include <limits.h>
 #include "pullpush.h"
 
@@ -176,9 +178,11 @@ void pull_subfield_end(const unsigned char **cursor, size_t *max,
         pull_failed(cursor, max);
     } else if (*cursor != NULL) {
         const unsigned char *subend = subcursor + submax;
-        assert(subcursor >= *cursor);
-        assert(subend <= *cursor + *max);
-        *max -= (subend - *cursor);
-        *cursor = subend;
+        if (subcursor < *cursor || subend > *cursor + *max) {
+            pull_failed(cursor, max);
+        } else {
+            *max -= (subend - *cursor);
+            *cursor = subend;
+        }
     }
 }

src/script.c

@@ -430,10 +430,14 @@ int wally_scriptpubkey_p2pkh_from_bytes(
         *written = 0;
 
     if (!bytes || !bytes_len || !script_flags_ok(flags, 0) ||
-        (flags & WALLY_SCRIPT_SHA256) || !bytes_out ||
-        len < WALLY_SCRIPTPUBKEY_P2PKH_LEN || !written)
+        (flags & WALLY_SCRIPT_SHA256) || !bytes_out || !len  || !written)
         return WALLY_EINVAL;
 
+    if (len < WALLY_SCRIPTPUBKEY_P2PKH_LEN) {
+        *written = WALLY_SCRIPTPUBKEY_P2PKH_LEN;
+        return WALLY_OK; /* Tell caller whats needed */
+    }
+
     if (flags & WALLY_SCRIPT_HASH160) {
         if (bytes_len != EC_PUBLIC_KEY_LEN && bytes_len != EC_PUBLIC_KEY_UNCOMPRESSED_LEN)
             return WALLY_EINVAL;

src/test/test_address.py

@@ -21,6 +21,8 @@
 NETWORK_LIQUID_REGTEST  = 0x04
 NETWORK_LIQUID_TESTNET  = 0x05
 
+SCRIPTPUBKEY_P2PKH_LEN = 25
+SCRIPTPUBKEY_P2SH_LEN = 23
 
 # Vector from test_bip32.py. We only need an xpub to derive addresses.
 vec = {
@@ -125,23 +127,24 @@ def do_test_vector(self, vec, path, network):
         self.assertEqual(new_addr, vec[path]['address_segwit'])
 
         # Parse legacy address (P2PKH):
-        out, out_len = make_cbuffer('00' * (25))
+        out, out_len = make_cbuffer('00' * SCRIPTPUBKEY_P2PKH_LEN)
         ret, written = wally_address_to_scriptpubkey(utf8(vec[path]['address_legacy']), network, out, out_len)
 
         self.assertEqual(ret, WALLY_OK)
-        self.assertEqual(h(out[0:written]), utf8(vec[path]['scriptpubkey_legacy']))
+        self.assertEqual(written, SCRIPTPUBKEY_P2PKH_LEN)
+        self.assertEqual(h(out[:written]), utf8(vec[path]['scriptpubkey_legacy']))
 
         # Get address for P2PKH scriptPubKey
         ret, new_addr = wally_scriptpubkey_to_address(out, written, network)
         self.assertEqual(ret, WALLY_OK)
         self.assertEqual(new_addr, vec[path]['address_legacy'])
 
         # Parse wrapped SegWit address (P2SH_P2WPKH):
-        out, out_len = make_cbuffer('00' * (25))
         ret, written = wally_address_to_scriptpubkey(utf8(vec[path]['address_p2sh_segwit']), network, out, out_len)
 
         self.assertEqual(ret, WALLY_OK)
-        self.assertEqual(h(out[0:written]), utf8(vec[path]['scriptpubkey_p2sh_segwit']))
+        self.assertEqual(written, SCRIPTPUBKEY_P2SH_LEN)
+        self.assertEqual(h(out[:written]), utf8(vec[path]['scriptpubkey_p2sh_segwit']))
 
         # Get address for P2SH scriptPubKey
         ret, new_addr = wally_scriptpubkey_to_address(out, written, network)
@@ -152,7 +155,7 @@ def do_test_vector(self, vec, path, network):
         out, out_len = make_cbuffer('00' * (100))
         ret, written = wally_addr_segwit_to_bytes(utf8(vec[path]['address_segwit']), utf8(bech32_prefix), 0, out, out_len)
         self.assertEqual(ret, WALLY_OK)
-        self.assertEqual(h(out[0:written]), utf8(vec[path]['scriptpubkey_segwit']))
+        self.assertEqual(h(out[:written]), utf8(vec[path]['scriptpubkey_segwit']))
 
     def test_address_scriptpubkey_liquid(self):
         """Check that addresses can be converted to and from scriptpubkeys for Liquid"""
@@ -164,7 +167,7 @@ def test_address_scriptpubkey_liquid(self):
             out, out_len = make_cbuffer('00' * (100))
             ret, written = wally_address_to_scriptpubkey(utf8(addr), network, out, out_len)
             self.assertEqual(ret, WALLY_OK)
-            self.assertEqual(h(out[0:written]), utf8(scriptpubkey))
+            self.assertEqual(h(out[:written]), utf8(scriptpubkey))
 
             ret, new_addr = wally_scriptpubkey_to_address(out, written, network)
             self.assertEqual(ret, WALLY_OK)

src/test/test_bip32.py

@@ -541,6 +541,7 @@ def get_paths(path):
                  ('/*/*',         W, 0),          # More than one wildcard
                  ('/1*',          W, 0),          # Invalid wildcard position (1)
                  ('/*1',          W, 0),          # Invalid wildcard position (2)
+                 ('m/1',          W, 1),          # Non-zero wildcard given for non-wildcard path
                  ('/*',           W, 2147483648)] # Hardened wildcard
 
         for path, flags, wildcard in cases:

src/test/test_script.py

@@ -98,13 +98,13 @@ def test_scriptpubkey_p2pkh_from_bytes(self):
         # Invalid args
         out, out_len = make_cbuffer('00' * SCRIPTPUBKEY_P2PKH_LEN)
         invalid_args = [
-            (None, PK_LEN, SCRIPT_HASH160, out, out_len), # Null bytes
-            (PK, 0, SCRIPT_HASH160, out, out_len), # Empty bytes
-            (PK, PK_LEN, SCRIPT_SHA256, out, out_len), # Unsupported flags
-            (PK, PK_LEN, SCRIPT_HASH160, None, out_len), # Null output
-            (PK, PK_LEN, SCRIPT_HASH160, out, SCRIPTPUBKEY_P2PKH_LEN-1), # Short output len
-            (PK, PK_LEN, 0, out, out_len), # Pubkey w/o SCRIPT_HASH160
-            (PKU, PKU_LEN, 0, out, out_len), # Uncompressed pubkey w/o SCRIPT_HASH160
+            (None, PK_LEN, SCRIPT_HASH160, out,  out_len), # Null bytes
+            (PK,  0,       SCRIPT_HASH160, out,  out_len), # Empty bytes
+            (PK,  PK_LEN,  SCRIPT_SHA256,  out,  out_len), # Unsupported flags
+            (PK,  PK_LEN,  SCRIPT_HASH160, None, out_len), # Null output
+            (PK,  PK_LEN,  SCRIPT_HASH160, out,  0),       # Empty output
+            (PK,  PK_LEN,  0,              out,  out_len), # Pubkey w/o SCRIPT_HASH160
+            (PKU, PKU_LEN, 0,              out,  out_len), # Uncompressed pubkey w/o SCRIPT_HASH160
         ]
         for args in invalid_args:
             ret = wally_scriptpubkey_p2pkh_from_bytes(*args)
@@ -124,6 +124,11 @@ def test_scriptpubkey_p2pkh_from_bytes(self):
             ret = wally_scriptpubkey_get_type(out, SCRIPTPUBKEY_P2PKH_LEN)
             self.assertEqual(ret, (WALLY_OK, SCRIPT_TYPE_P2PKH))
 
+        # Calling with a too-short output buffer returns the required length
+        args = (PK, PK_LEN, SCRIPT_HASH160, out, SCRIPTPUBKEY_P2PKH_LEN-1) # Short output len
+        ret = wally_scriptpubkey_p2pkh_from_bytes(*args)
+        self.assertEqual(ret, (WALLY_OK, SCRIPTPUBKEY_P2PKH_LEN))
+
     def test_scriptpubkey_p2sh_from_bytes(self):
         """Tests for creating p2sh scriptPubKeys"""
         # Invalid args

tools/coverage.sh

@@ -5,7 +5,7 @@
 # ./tools/coverage.sh       : Calculates coverage stats, produces
 #                             src/lcov/index.html as output.
 
-lcov="lcov --directory=src/ --base-directory src/"
+lcov="lcov --directory=src/ --base-directory src/ --gcov-tool $PWD/tools/run-gcov.sh"
 
 if [ "$1" = "clean" ]; then
     $lcov --zerocounters

tools/run-gcov.sh

@@ -0,0 +1,4 @@
+#!/bin/bash
+# Set GCOV to e.g. llvm-gov, llvm-gov-11 etc for clang,
+# leave it unset for gcc
+exec $GCOV gcov "$@"