Firmware unit test fixing

.github/workflows/run-tests.yml

@@ -23,15 +23,14 @@ jobs:
       - name: Firmware tests using TCPSigner
         run: firmware/test/test-all
 
-      - name: Firmware TCPSigner's unit tests
-        working-directory: 
-        run: firmware/src/tcpsigner/test/run-all.sh
+      - name: Firmware HAL's unit tests
+        run: firmware/src/hal/test/run-all.sh
 
       - name: Firmware common lib unit tests
         run: firmware/src/common/test/run-all.sh
 
-      - name: Ledger Signer's unit tests
-        run: firmware/src/ledger/signer/test/run-all.sh
+      - name: Firmware PowHSM's unit tests
+        run: firmware/src/powhsm/test/run-all.sh
 
       - name: Ledger UI's unit tests
         run: firmware/src/ledger/ui/test/run-all.sh

firmware/coverage/gen-coverage

@@ -19,14 +19,23 @@ if [[ $1 == "exec" ]]; then
     pushd $REPOROOT/firmware/src/tcpsigner > /dev/null
     COVERAGE=y make clean all
     ./tcpsigner --checkpoint 0xbdcb3c17c7aee714cec8ad900341bfd987b452280220dcbd6e7191f67ea4209b --difficulty 0x32 --network regtest > /dev/null &
+    TCPSIGNER_PID=$!
     popd > /dev/null
 
     pushd $REPOROOT/firmware/test > /dev/null
     python run.py
     err_code=$?
     popd > /dev/null
 
+    kill $TCPSIGNER_PID
+
+    # Capture coverage data
     lcov --capture --directory $SRCDIR --list-full-path --output-file $BASEDIR/coverage.info
+    # Remove unwanted coverage info (test files, tcpsigner, x86 HAL implementation)
+    lcov --remove $BASEDIR/coverage.info "*/test_*.c" --output-file $BASEDIR/coverage.info
+    lcov --remove $BASEDIR/coverage.info "*/tcpsigner/src/*" --output-file $BASEDIR/coverage.info
+    lcov --remove $BASEDIR/coverage.info "*/hal/src/x86/*" --output-file $BASEDIR/coverage.info
+    # Generate report and summary
     genhtml $BASEDIR/coverage.info --output $BASEDIR/output -p $SRCDIR -t "powHSM firmware"
     lcov --summary $BASEDIR/coverage.info | grep lines | sed -e "s/.\+lines.\+: \([[:digit:].]\+\).\+/\1/g" > $BASEDIR/output/total
     mv $BASEDIR/coverage.info $BASEDIR/output

firmware/src/.gitignore

@@ -1,19 +1,25 @@
-bin/
-debug/
-dep/
-obj/
+# Build artifacts
+**/bin
+**/debug
+**/dep
+**/obj
+**/objs
+**/static-analysis
+**/test/**/*.o
+**/test/**/*.out
+
+# UI glyphs
+ledger/ui/src/glyphs.c
+ledger/ui/src/glyphs.h
+
+# Icons hex
+ledger/signer/icon.hex
+ledger/ui/icon.hex
 
 # TCPSigner
 tcpsigner/tcpsigner
-tcpsigner/objs
-tcpsigner/test/**/*.o
-tcpsigner/test/**/test.out
 tcpsigner/**/*.json
 
-# Icons hex
-signer/icon.hex
-ui/icon.hex
-
 # Code coverage artifacts
-*.gcda
-*.gcno
+**/*.gcda
+**/*.gcno

firmware/src/common/test/common.mk

@@ -0,0 +1,28 @@
+# The MIT License (MIT)
+#
+# Copyright (c) 2021 RSK Labs Ltd
+#
+# Permission is hereby granted, free of charge, to any person obtaining a copy of
+# this software and associated documentation files (the "Software"), to deal in
+# the Software without restriction, including without limitation the rights to
+# use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies
+# of the Software, and to permit persons to whom the Software is furnished to do
+# so, subject to the following conditions:
+#
+# The above copyright notice and this permission notice shall be included in all
+# copies or substantial portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+# SOFTWARE.
+
+SRCDIR = ../../src
+CFLAGS = -iquote $(SRCDIR) -iquote .
+
+include ../../../../coverage/coverage.mk
+
+CFLAGS += $(COVFLAGS)

firmware/src/common/test/ints/Makefile

@@ -20,23 +20,22 @@
 # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 # SOFTWARE.
 
-SRCDIR = ../../src
-CFLAGS = -I $(SRCDIR) -I ./
+include ../common.mk
 
 PROG = test.out
 OBJS = test_ints.o
 
 all: $(PROG)
 
 $(PROG): $(OBJS)
-	$(CC) -o $@ $^
+	$(CC) $(COVFLAGS) -o $@ $^
 
 test_ints.o: test_ints.c $(SRCDIR)/ints.h
 
 .PHONY: clean test
 
 clean:
-	rm -f $(PROG) ./*.o
+	rm -f $(PROG) ./*.o $(COVFILES)
 
 test: all
 	./$(PROG)

firmware/src/common/test/memutil/Makefile

@@ -20,23 +20,22 @@
 # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 # SOFTWARE.
 
-SRCDIR = ../../src
-CFLAGS = -I $(SRCDIR) -I ./
+include ../common.mk
 
 PROG = test.out
 OBJS = test_memutil.o
 
 all: $(PROG)
 
 $(PROG): $(OBJS)
-	$(CC) -o $@ $^
+	$(CC) $(COVFLAGS) -o $@ $^
 
-test_memutil.o: test_memutil.c $(SRCDIR)/memutil.h os.h
+test_memutil.o: test_memutil.c $(SRCDIR)/memutil.h
 
 .PHONY: clean test
 
 clean:
-	rm -f $(PROG) ./*.o
+	rm -f $(PROG) ./*.o $(COVFILES)
 
 test: all
 	./$(PROG)

firmware/src/common/test/memutil/os.h → firmware/src/common/test/memutil/common_requirements.h

@@ -22,4 +22,9 @@
  * IN THE SOFTWARE.
  */
 
-void os_memmove(void *dst, const void *src, unsigned int length);
+#ifndef __COMMON_REQUIREMENTS_H
+#define __COMMON_REQUIREMENTS_H
+
+void platform_memmove(void *dst, const void *src, unsigned int length);
+
+#endif // __COMMON_REQUIREMENTS_H

firmware/src/common/test/memutil/test_memutil.c

@@ -46,11 +46,11 @@ unsigned char* expected_src;
 unsigned int expected_length;
 int copied;
 
-void os_memmove_reset_mock() {
+void platform_memmove_reset_mock() {
     copied = 0;
 }
 
-void os_memmove(void* dst, const void* src, unsigned int length) {
+void platform_memmove(void* dst, const void* src, unsigned int length) {
     assert(expected_dst == dst);
     assert(expected_src == src);
     assert(expected_length == length);
@@ -62,7 +62,7 @@ void test_ok() {
     char src[15];
     char dst[10];
 
-    os_memmove_reset_mock();
+    platform_memmove_reset_mock();
     TEST_MEMMOVE(
         dst, sizeof(dst), 0, src, sizeof(src), 0, 10, { assert(false); });
     TEST_MEMMOVE(
@@ -82,7 +82,7 @@ void test_negatives() {
     char dst[10];
     int failed = 0;
 
-    os_memmove_reset_mock();
+    platform_memmove_reset_mock();
     TEST_MEMMOVE(dst, sizeof(dst), 0, src, sizeof(src), 0, -1, { failed++; });
     TEST_MEMMOVE(dst, sizeof(dst), 0, src, sizeof(src), 0, -5, { failed++; });
     TEST_MEMMOVE(dst, sizeof(dst), -1, src, sizeof(src), 0, 2, { failed++; });
@@ -97,7 +97,7 @@ void test_src_outofbounds() {
     char dst[10];
     int failed = 0;
 
-    os_memmove_reset_mock();
+    platform_memmove_reset_mock();
     TEST_MEMMOVE(dst, sizeof(dst), 0, src, sizeof(src), 0, 6, { failed++; });
     TEST_MEMMOVE(dst, sizeof(dst), 0, src, sizeof(src), 0, 7, { failed++; });
     assert(failed == 2);
@@ -110,7 +110,7 @@ void test_src_outofbounds_offset() {
     char dst[10];
     int failed = 0;
 
-    os_memmove_reset_mock();
+    platform_memmove_reset_mock();
     TEST_MEMMOVE(dst, sizeof(dst), 0, src, sizeof(src), 6, 10, { failed++; });
     TEST_MEMMOVE(dst, sizeof(dst), 0, src, sizeof(src), 15, 4, { failed++; });
     assert(failed == 2);
@@ -123,7 +123,7 @@ void test_dst_outofbounds() {
     char dst[10];
     int failed = 0;
 
-    os_memmove_reset_mock();
+    platform_memmove_reset_mock();
     TEST_MEMMOVE(dst, sizeof(dst), 0, src, sizeof(src), 0, 11, { failed++; });
     TEST_MEMMOVE(dst, sizeof(dst), 0, src, sizeof(src), 0, 13, { failed++; });
     assert(failed == 2);
@@ -136,7 +136,7 @@ void test_dst_outofbounds_offset() {
     char dst[10];
     int failed = 0;
 
-    os_memmove_reset_mock();
+    platform_memmove_reset_mock();
     TEST_MEMMOVE(dst, sizeof(dst), 8, src, sizeof(src), 0, 3, { failed++; });
     TEST_MEMMOVE(dst, sizeof(dst), 10, src, sizeof(src), 0, 1, { failed++; });
     assert(failed == 2);
@@ -149,7 +149,7 @@ void test_overflow() {
     char dst[10];
     int failed = 0;
 
-    os_memmove_reset_mock();
+    platform_memmove_reset_mock();
     TEST_MEMMOVE(
         dst, sizeof(dst), 10, src, sizeof(src), 0, UINT_MAX - 5, { failed++; });
     TEST_MEMMOVE(

firmware/src/hal/src/x86/bip32.c

@@ -83,6 +83,10 @@ size_t bip32_parse_path(const char* path, uint8_t* out) {
             number = true;
             start = ++pos;
             if (parts == EXPECTED_PARTS) {
+                if (pos < pathlen) {
+                    DEBUG_LOG("Path has too many parts: %s\n", path);
+                    return 0;
+                }
                 out[0] = (char)parts;
                 return 1 + parts * sizeof(uint32_t);
             }
@@ -92,20 +96,3 @@ size_t bip32_parse_path(const char* path, uint8_t* out) {
     DEBUG_LOG("Unexpected code path reached for path: %s\n", path);
     return 0;
 }
-
-// Testing: move somewhere else
-// #include <stdio.h>
-// int main() {
-//     char* bip32path = "m/44'/137'/1'/0/0";
-//     uint8_t path[100];
-//     size_t pathlen = bip32_parse_path(bip32path, path);
-//     printf("BIP32: %s\n", bip32path);
-//     printf("Len: %lu\n", pathlen);
-//     printf("Path: ");
-//     for (int i = 0; i < pathlen; i++) {
-//         printf("%02X", path[i]);
-//         if (i==0 || i%4 == 0) printf(" ");
-//     }
-//     printf("\n");
-//     return 0;
-// }

firmware/src/hal/src/x86/bip32.h

@@ -40,7 +40,7 @@
  * @param path the bip32 path as string
  * @param out the destination buffer for the parsed path
  *
- * @returns the size of the parsed path in bytes
+ * @returns the size of the parsed path in bytes, or zero in case of error
  */
 size_t bip32_parse_path(const char* path, uint8_t* out);
 

firmware/src/hal/test/bip32/Makefile

@@ -0,0 +1,42 @@
+# The MIT License (MIT)
+#
+# Copyright (c) 2021 RSK Labs Ltd
+#
+# Permission is hereby granted, free of charge, to any person obtaining a copy of
+# this software and associated documentation files (the "Software"), to deal in
+# the Software without restriction, including without limitation the rights to
+# use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies
+# of the Software, and to permit persons to whom the Software is furnished to do
+# so, subject to the following conditions:
+#
+# The above copyright notice and this permission notice shall be included in all
+# copies or substantial portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+# SOFTWARE.
+
+include ../common.mk
+
+ifneq ($(DEBUG),)
+	CFLAGS += -DDEBUG_BIP32
+endif
+
+PROG = test.out
+OBJS = bip32.o test_bip32.o log.o
+
+all: $(PROG)
+
+$(PROG): $(OBJS)
+	$(CC) $(COVFLAGS) -o $@ $^
+
+.PHONY: clean test
+clean:
+	rm -f $(PROG) ./*.o $(COVFILES)
+
+test: all
+	./$(PROG)