Merge pull request #108 from trilitech/105-fix-minor-issues-with-code

Fix minor issues like NULL pointer check

.doxygen/Doxyfile

@@ -734,7 +734,7 @@ STRICT_PROTO_MATCHING  = NO
 # list. This list is created by putting \todo commands in the documentation.
 # The default value is: YES.
 
-GENERATE_TODOLIST      = YES
+GENERATE_TODOLIST      = NO
 
 # The GENERATE_TESTLIST tag can be used to enable (YES) or disable (NO) the test
 # list. This list is created by putting \test commands in the documentation.

README.md

@@ -142,26 +142,23 @@ BOLOS_SDK=$NANOS_SDK make DEBUG=1
 You can replace `NANOS` with `NANOSP`, `NANOX`, `STAX` for the other devices in BOLOS_SDK environmental variable.
 
 ### Testing
-To test the application you need to enable python virtual environment and some dependencies. On any operating system, create a python virtual environment and activate it.
+The application tests are run using same docker container used for building. Inside the docker container run following script,
 ```
-$ sudo apt-get update && sudo apt-get install -y qemu-user-static tesseract-ocr libtesseract-dev python3-pip libgmp-dev libsodium-dev git
-$ python3 -m venv env
-$ source env/bin/activate
-```
-Inside the virtualenv, load the requirements.txt file.
-```
-(env)$ pip install -r test/requirements.txt
+$ apk add gmp-dev libsodium-dev;
+$ python3 -m venv tezos_test_env --system-site-package;
+$ source ./tezos_test_env/bin/activate;
+(tezos_test_env)$ python3 -m pip install -r test/requirements.txt -q ;
 ```
 Now you can run ragger tests for any perticular ledger device. Please make sure you have built the app.elf files for that perticular device first. Then run following command:
 ```
-(env)$ pytest test --device nanosp
+(tezos_test_env)$ python3 -m pytest test --device nanosp
 ```
 Replace nanosp with any of the following for respective device: nanos, nanosp, nanox , stax.
 
 These tests are run on Ledger emulator called speculos which emulates the actual ledger device. To run theese test on actual device you have to choose a backend. Run following commands to run these test on device:
 ```
-(env)$ pip install ragger[all_backends]
-(env)$ pytest test --device nanosp --backend ledgercomm -s
+(tezos_test_env)$ python3 -m pip install ragger[all_backends]
+(tezos_test_env)$ python3 -m pytest test --device nanosp --backend ledgercomm -s
 ```
 Note the `-s` flag which is required when running interactive tests with pytest. You can also choose `ledgerwallet` backend to run tests on device.
 

src/apdu_query.c

@@ -69,10 +69,12 @@ int handle_query_main_hwm(void) {
 }
 
 int handle_query_auth_key(void) {
+    tz_exc exc = SW_OK;
     uint8_t resp[1u + (MAX_BIP32_PATH * sizeof(uint32_t))] = {0};
     size_t offset = 0;
 
     uint8_t const length = g_hwm.baking_key.bip32_path.length;
+    TZ_ASSERT(length <= NUM_ELEMENTS(g_hwm.baking_key.bip32_path.components), EXC_WRONG_LENGTH);
 
     resp[offset] = length;
     offset++;
@@ -83,21 +85,22 @@ int handle_query_auth_key(void) {
     }
 
     return io_send_response_pointer(resp, offset, SW_OK);
+end:
+    return io_send_apdu_err(exc);
 }
 
 int handle_query_auth_key_with_curve(void) {
+    tz_exc exc = SW_OK;
     uint8_t resp[2u + (MAX_BIP32_PATH * sizeof(uint32_t))] = {0};
     size_t offset = 0;
 
     uint8_t const length = g_hwm.baking_key.bip32_path.length;
+    TZ_ASSERT(length <= NUM_ELEMENTS(g_hwm.baking_key.bip32_path.components), EXC_WRONG_LENGTH);
 
     int derivation_type = unparse_derivation_type(g_hwm.baking_key.derivation_type);
+    TZ_ASSERT(derivation_type >= 0, EXC_REFERENCED_DATA_NOT_FOUND);
 
-    if (derivation_type < 0) {
-        return io_send_apdu_err(EXC_REFERENCED_DATA_NOT_FOUND);
-    }
-
-    resp[offset] = unparse_derivation_type(g_hwm.baking_key.derivation_type);
+    resp[offset] = derivation_type;
     offset++;
 
     resp[offset] = length;
@@ -109,4 +112,6 @@ int handle_query_auth_key_with_curve(void) {
     }
 
     return io_send_response_pointer(resp, offset, SW_OK);
+end:
+    return io_send_apdu_err(exc);
 }

src/apdu_sign.c

@@ -97,7 +97,6 @@ static tz_exc blake2b_incremental_hash(uint8_t *const buff,
         *buff_length -= B2B_BLOCKBYTES;
         current += B2B_BLOCKBYTES;
     }
-    // TODO use circular buffer at some point
     memmove(buff, current, *buff_length);
 
 end:
@@ -289,7 +288,7 @@ int select_signing_key(buffer_t *cdata, derivation_type_t derivation_type) {
  * Cdata:
  *   + (max-size) uint8 *: message
  */
-int handle_sign(buffer_t *cdata, bool last, bool with_hash) {
+int handle_sign(buffer_t *cdata, const bool last, const bool with_hash) {
     tz_exc exc = SW_OK;
 
     TZ_ASSERT_NOT_NULL(cdata);
@@ -375,7 +374,7 @@ static int perform_signature(bool const send_hash) {
 
     TZ_ASSERT(os_global_pin_is_validated() == BOLOS_UX_OK, EXC_SECURITY);
 
-    TZ_ASSERT(write_high_water_mark(&G.parsed_baking_data), EXC_MEMORY_ERROR);
+    TZ_CHECK(write_high_water_mark(&G.parsed_baking_data));
 
     uint8_t resp[SIGN_HASH_SIZE + MAX_SIGNATURE_SIZE] = {0};
     size_t offset = 0;

src/globals.h

@@ -58,8 +58,10 @@ void toggle_hwm(void);
 /// Our buffer must accommodate any remainder from hashing and the next message at once.
 #define TEZOS_BUFSIZE (BLAKE2B_BLOCKBYTES + MAX_APDU_SIZE)
 
-#define PRIVATE_KEY_DATA_SIZE 64u
-#define MAX_SIGNATURE_SIZE    100u
+#define PRIVATE_KEY_DATA_SIZE         64u
+#define MAX_SIGNATURE_SIZE            100u
+#define ELLIPTIC_CURVE_PUB_KEY_LENGTH 65u
+#define PUB_KEY_COMPPRESSED_LENGTH    33u
 
 /**
  * @brief This structure represents the state needed to handle HMAC

src/keys.c

@@ -87,7 +87,7 @@ cx_err_t generate_public_key(cx_ecfp_public_key_t *public_key,
     signature_type_t signature_type = derivation_type_to_signature_type(derivation_type);
     cx_curve_t cx_curve = signature_type_to_cx_curve(signature_type);
 
-    public_key->W_len = 65;
+    public_key->W_len = ELLIPTIC_CURVE_PUB_KEY_LENGTH;
     public_key->curve = cx_curve;
 
     // generate corresponding public key
@@ -105,7 +105,7 @@ cx_err_t generate_public_key(cx_ecfp_public_key_t *public_key,
     if (cx_curve == CX_CURVE_Ed25519) {
         CX_CHECK(
             cx_edwards_compress_point_no_throw(CX_CURVE_Ed25519, public_key->W, public_key->W_len));
-        public_key->W_len = 33;
+        public_key->W_len = PUB_KEY_COMPPRESSED_LENGTH;
     }
 
 end:
@@ -134,7 +134,7 @@ static cx_err_t public_key_hash(uint8_t *const hash_out,
         return CX_INVALID_PARAMETER;
     }
 
-    if (hash_out_size < HASH_SIZE) {
+    if (hash_out_size < KEY_HASH_SIZE) {
         return CX_INVALID_PARAMETER_SIZE;
     }
 
@@ -151,7 +151,7 @@ static cx_err_t public_key_hash(uint8_t *const hash_out,
         case SIGNATURE_TYPE_SECP256R1: {
             memcpy(compressed.W, public_key->W, public_key->W_len);
             compressed.W[0] = 0x02 + (public_key->W[64] & 0x01);
-            compressed.W_len = 33;
+            compressed.W_len = PUB_KEY_COMPPRESSED_LENGTH;
             break;
         }
         default:
@@ -161,14 +161,14 @@ static cx_err_t public_key_hash(uint8_t *const hash_out,
     cx_err_t error = CX_OK;
     cx_blake2b_t hash_state;
     // cx_blake2b_init takes size in bits.
-    CX_CHECK(cx_blake2b_init_no_throw(&hash_state, HASH_SIZE * 8u));
+    CX_CHECK(cx_blake2b_init_no_throw(&hash_state, KEY_HASH_SIZE * 8u));
 
     CX_CHECK(cx_hash_no_throw((cx_hash_t *) &hash_state,
                               CX_LAST,
                               compressed.W,
                               compressed.W_len,
                               hash_out,
-                              HASH_SIZE));
+                              KEY_HASH_SIZE));
 
     if (compressed_out != NULL) {
         memmove(compressed_out, &compressed, sizeof(*compressed_out));

src/operations.c

@@ -147,7 +147,7 @@ static inline tz_exc compute_pkh(cx_ecfp_public_key_t *const compressed_pubkey_o
 static tz_parser_result parse_implicit(
     parsed_contract_t *const out,
     raw_tezos_header_signature_type_t const *const raw_signature_type,
-    uint8_t const hash[HASH_SIZE]) {
+    uint8_t const hash[KEY_HASH_SIZE]) {
     tz_parser_result res = PARSER_CONTINUE;
 
     out->originated = 0;
@@ -172,7 +172,6 @@ static tz_parser_result parse_implicit(
 
 #define NEXT_BYTE (byte)
 
-// TODO: this function cannot parse z values than would not fit in a uint64
 /**
  * @brief Parses a Z number
  *
@@ -285,7 +284,6 @@ static tz_exc parse_operations_init(struct parsed_operation_group *const out,
     TZ_CHECK(compute_pkh(&out->public_key, &out->signing, path_with_curve));
 
     // Start out with source = signing, for reveals
-    // TODO: This is slightly hackish
     memcpy(&out->operation.source, &out->signing, sizeof(out->signing));
 
     state->op_step = 0;
@@ -304,6 +302,9 @@ static tz_exc parse_operations_init(struct parsed_operation_group *const out,
 
 bool parse_operations_final(struct parse_state *const state,
                             struct parsed_operation_group *const out) {
+    if ((state == NULL) || (out == NULL)) {
+        return false;
+    }
     if ((out->operation.tag == OPERATION_TAG_NONE) && !out->has_reveal) {
         return false;
     }

src/operations.h

@@ -53,7 +53,7 @@ struct operation_group_header {
  */
 struct implicit_contract {
     raw_tezos_header_signature_type_t signature_type;  ///< type of the contract signature
-    uint8_t pkh[HASH_SIZE];                            ///< raw public key hash
+    uint8_t pkh[KEY_HASH_SIZE];                        ///< raw public key hash
 } __attribute__((packed));
 
 /**
@@ -72,7 +72,7 @@ union public_key {
  */
 struct delegation_contents {
     raw_tezos_header_signature_type_t signature_type;  ///< type of the delegate signature
-    uint8_t hash[HASH_SIZE];                           ///< raw delegate
+    uint8_t hash[KEY_HASH_SIZE];                       ///< raw delegate
 } __attribute__((packed));
 
 /**

src/to_string.c

@@ -34,14 +34,14 @@
 static int pkh_to_string(char *const dest,
                          size_t const dest_size,
                          signature_type_t const signature_type,
-                         uint8_t const hash[HASH_SIZE]);
+                         uint8_t const hash[KEY_HASH_SIZE]);
 
 tz_exc bip32_path_with_curve_to_pkh_string(char *const out,
                                            size_t const out_size,
                                            bip32_path_with_curve_t const *const key) {
     tz_exc exc = SW_OK;
     cx_err_t error = CX_OK;
-    uint8_t hash[HASH_SIZE];
+    uint8_t hash[KEY_HASH_SIZE];
 
     TZ_ASSERT_NOT_NULL(out);
     TZ_ASSERT_NOT_NULL(key);
@@ -87,15 +87,15 @@ static void compute_hash_checksum(uint8_t out[TEZOS_HASH_CHECKSUM_SIZE],
 static int pkh_to_string(char *const dest,
                          size_t const dest_size,
                          signature_type_t const signature_type,
-                         uint8_t const hash[HASH_SIZE]) {
+                         uint8_t const hash[KEY_HASH_SIZE]) {
     if ((dest == NULL) || (hash == NULL)) {
         return -1;
     }
 
     // Data to encode
     struct __attribute__((packed)) {
         uint8_t prefix[3];
-        uint8_t hash[HASH_SIZE];
+        uint8_t hash[KEY_HASH_SIZE];
         uint8_t checksum[TEZOS_HASH_CHECKSUM_SIZE];
     } data;
 
@@ -161,13 +161,14 @@ static int chain_id_to_string(char *const dest,
     return base58_encode((const uint8_t *) &data, sizeof(data), dest, dest_size);
 }
 
-#define SAFE_STRCPY(dest, size, x) \
-    ({                             \
-        if (size < sizeof(x)) {    \
-            return -1;             \
-        }                          \
-        strlcpy(dest, x, size);    \
-        return sizeof(x);          \
+#define SAFE_STRCPY(dest, dest_size, in) \
+    ({                                   \
+        size_t in_size = strlen(in);     \
+        if (dest_size < in_size) {       \
+            return -1;                   \
+        }                                \
+        strlcpy(dest, in, dest_size);    \
+        return in_size;                  \
     })
 
 int chain_id_to_string_with_aliases(char *const dest,
@@ -302,7 +303,7 @@ int microtez_to_string(char *const dest, size_t dest_size, uint64_t number) {
 }
 
 int hwm_to_string(char *dest, size_t dest_size, high_watermark_t const *const hwm) {
-    if (dest == NULL) {
+    if ((dest == NULL) || (hwm == NULL)) {
         return -1;
     }
     int result = number_to_string(dest, dest_size, hwm->highest_level);
@@ -323,7 +324,7 @@ int hwm_to_string(char *dest, size_t dest_size, high_watermark_t const *const hw
 }
 
 int hwm_status_to_string(char *dest, size_t dest_size, volatile bool const *hwm_disabled) {
-    if ((dest == NULL) || (dest_size < 9u)) {
+    if ((dest == NULL) || (dest_size < 9u) || (hwm_disabled == NULL)) {
         return -1;
     }
     memcpy(dest, *hwm_disabled ? "Disabled" : "Enabled", dest_size);
@@ -332,15 +333,8 @@ int hwm_status_to_string(char *dest, size_t dest_size, volatile bool const *hwm_
 
 int copy_string(char *const dest, size_t const dest_size, char const *const src) {
     if ((dest == NULL) || (src == NULL)) {
-        return false;
-    }
-
-    char const *const src_in = (char const *) PIC(src);
-    // I don't care that we will loop through the string twice, latency is not an issue
-    size_t src_size = strlen(src_in);
-    if (src_size >= dest_size) {
         return -1;
     }
-    strlcpy(dest, src_in, dest_size);
-    return src_size;
+    char const *const src_in = (char const *) PIC(src);
+    SAFE_STRCPY(dest, dest_size, src_in);
 }

src/types.h

@@ -204,15 +204,14 @@ typedef struct {
                                               no need to track HWM using Ledger.*/
 } baking_data;
 
-#define SIGN_HASH_SIZE 32u  // TODO: Rename or use a different constant.
+#define SIGN_HASH_SIZE 32u
 
-#define PKH_STRING_SIZE 40u  // includes null byte // TODO: use sizeof for this.
+#define PKH_STRING_SIZE 40u  // includes null byte
 #define HWM_STATUS_SIZE 9u   // HWM status takes values Enabled and Disabled.
 #define PROTOCOL_HASH_BASE58_STRING_SIZE \
     sizeof("ProtoBetaBetaBetaBetaBetaBetaBetaBetaBet11111a5ug96")
 
-// TODO: Rename to KEY_HASH_SIZE
-#define HASH_SIZE 20u
+#define KEY_HASH_SIZE 20u
 
 /**
  * @brief This structure represents the content of a parsed baking data
@@ -235,7 +234,7 @@ typedef struct parsed_contract {
     signature_type_t signature_type; /**< 0 in originated case
                                      An implicit contract with signature_type of 0
                                      means not present*/
-    uint8_t hash[HASH_SIZE];         ///< hash of the contract
+    uint8_t hash[KEY_HASH_SIZE];     ///< hash of the contract
 } parsed_contract_t;
 
 /**

test/requirements.txt

@@ -1,5 +1,5 @@
 base58
 bip32
 GitPython
-pytezos
-ragger[tests,speculos]
+pytezos>=3.11.3
+ragger>=1.18.1

test/setup_script.sh

@@ -16,3 +16,4 @@
 # limitations under the License.
 
 echo "No setup"
+pip install ragger[tests,speculos]