refactor: make s2n_stuffer_read_hex match s2n_stuffer_read

stuffer/s2n_stuffer.h

@@ -164,7 +164,7 @@ int s2n_stuffer_copy(struct s2n_stuffer *from, struct s2n_stuffer *to, uint32_t
  * "FF" == 255 or [ 0xff ]
  * "0001" == 1 or [ 0x00, 0x01 ]
  */
-S2N_RESULT s2n_stuffer_read_hex(struct s2n_stuffer *bytes_out, const struct s2n_blob *hex_in);
+S2N_RESULT s2n_stuffer_read_hex(struct s2n_stuffer *hex_in, const struct s2n_blob *bytes_out);
 S2N_RESULT s2n_stuffer_write_hex(struct s2n_stuffer *hex_out, const struct s2n_blob *bytes_in);
 S2N_RESULT s2n_stuffer_read_uint8_hex(struct s2n_stuffer *stuffer, uint8_t *u);
 S2N_RESULT s2n_stuffer_write_uint8_hex(struct s2n_stuffer *stuffer, uint8_t u);

stuffer/s2n_stuffer_hex.c

@@ -41,30 +41,28 @@ static S2N_RESULT s2n_stuffer_hex_digit_from_char(uint8_t c, uint8_t *i)
     return S2N_RESULT_OK;
 }
 
-S2N_RESULT s2n_stuffer_read_hex(struct s2n_stuffer *bytes_out, const struct s2n_blob *hex_in)
+S2N_RESULT s2n_stuffer_read_hex(struct s2n_stuffer *hex_in, const struct s2n_blob *bytes_out)
 {
-    RESULT_PRECONDITION(s2n_stuffer_validate(bytes_out));
-    RESULT_PRECONDITION(s2n_blob_validate(hex_in));
-
-    size_t hex_size = hex_in->size;
-    size_t bytes_size = hex_in->size / 2;
-    RESULT_ENSURE(hex_size % 2 == 0, S2N_ERR_BAD_HEX);
-    if (hex_size == 0) {
+    RESULT_PRECONDITION(s2n_stuffer_validate(hex_in));
+    RESULT_PRECONDITION(s2n_blob_validate(bytes_out));
+    if (bytes_out->size == 0) {
         return S2N_RESULT_OK;
     }
 
-    RESULT_GUARD_POSIX(s2n_stuffer_reserve_space(bytes_out, bytes_size));
-    uint8_t *out = bytes_out->blob.data + bytes_out->write_cursor;
-    uint8_t *in = hex_in->data;
+    size_t hex_size = bytes_out->size * 2;
+    RESULT_ENSURE(s2n_stuffer_data_available(hex_in) >= hex_size, S2N_ERR_BAD_HEX);
 
-    for (size_t i = 0; i < bytes_size; i++) {
+    uint8_t *out = bytes_out->data;
+    uint8_t *in = hex_in->blob.data + hex_in->read_cursor;
+
+    for (size_t i = 0; i < bytes_out->size; i++) {
         uint8_t hex_high = 0, hex_low = 0;
         RESULT_GUARD(s2n_stuffer_hex_digit_from_char(in[(i * 2)], &hex_high));
         RESULT_GUARD(s2n_stuffer_hex_digit_from_char(in[(i * 2) + 1], &hex_low));
         out[i] = (hex_high * 16) + hex_low;
     }
 
-    RESULT_GUARD_POSIX(s2n_stuffer_skip_write(bytes_out, bytes_size));
+    RESULT_GUARD_POSIX(s2n_stuffer_skip_read(hex_in, hex_size));
     return S2N_RESULT_OK;
 }
 

tests/cbmc/proofs/s2n_stuffer_read_hex/Makefile

@@ -38,6 +38,6 @@ PROJECT_SOURCES += $(SRCDIR)/utils/s2n_mem.c
 PROJECT_SOURCES += $(SRCDIR)/utils/s2n_result.c
 PROJECT_SOURCES += $(SRCDIR)/utils/s2n_safety.c
 
-UNWINDSET += s2n_stuffer_read_hex.8:$(MAX_BLOB_SIZE)
+UNWINDSET += s2n_stuffer_read_hex.7:$(MAX_BLOB_SIZE)
 
 include ../Makefile.common

tests/cbmc/proofs/s2n_stuffer_read_hex/s2n_stuffer_read_hex_harness.c

@@ -24,42 +24,29 @@ void s2n_stuffer_read_hex_harness()
 {
     nondet_s2n_mem_init();
 
-    struct s2n_stuffer *output = cbmc_allocate_s2n_stuffer();
-    __CPROVER_assume(s2n_result_is_ok(s2n_stuffer_validate(output)));
+    struct s2n_stuffer *input = cbmc_allocate_s2n_stuffer();
+    __CPROVER_assume(s2n_result_is_ok(s2n_stuffer_validate(input)));
 
-    struct s2n_blob *hex_in = cbmc_allocate_s2n_blob();
-    __CPROVER_assume(s2n_result_is_ok(s2n_blob_validate(hex_in)));
-    __CPROVER_assume(s2n_blob_is_bounded(hex_in, MAX_BLOB_SIZE));
+    struct s2n_blob *output = cbmc_allocate_s2n_blob();
+    __CPROVER_assume(s2n_result_is_ok(s2n_blob_validate(output)));
+    __CPROVER_assume(s2n_blob_is_bounded(output, MAX_BLOB_SIZE - 1));
 
-    struct s2n_stuffer old_output = *output;
-    struct store_byte_from_buffer output_saved_byte = { 0 };
-    save_byte_from_blob(&output->blob, &output_saved_byte);
-    __CPROVER_assume(output_saved_byte.idx < output->write_cursor);
+    struct s2n_stuffer old_input = *input;
+    struct s2n_blob old_output = *output;
+    struct store_byte_from_buffer input_saved_byte = { 0 };
+    save_byte_from_blob(&input->blob, &input_saved_byte);
 
-    struct s2n_blob old_hex_in = *hex_in;
-    struct store_byte_from_buffer old_hex_in_byte = { 0 };
-    save_byte_from_blob(hex_in, &old_hex_in_byte);
+    s2n_result result = s2n_stuffer_read_hex(input, output);
 
-    s2n_result result = s2n_stuffer_read_hex(output, hex_in);
-
-    struct s2n_stuffer expected_bytes_out = old_output;
-    struct s2n_blob expected_hex_in = old_hex_in;
-
-    /* On success, the byte equivalent of the hex was written to the stuffer */
+    /* On success, enough hex to fill the blob was read from the stuffer */
+    struct s2n_stuffer expected_input = old_input;
     if (s2n_result_is_ok(result)) {
-        expected_bytes_out.write_cursor += old_hex_in.size / 2;
-        expected_bytes_out.high_water_mark = MAX(expected_bytes_out.write_cursor,
-                old_output.high_water_mark);
-    }
-
-    /* Memory may be allocated on either success or failure,
-     * because we allocated the memory before we start writing. */
-    if (output->blob.size > old_output.blob.size) {
-        expected_bytes_out.blob = output->blob;
+        expected_input.read_cursor += old_output.size * 2;
     }
+    assert(s2n_result_is_ok(s2n_stuffer_validate(input)));
+    assert_stuffer_equivalence(input, &expected_input, &input_saved_byte);
 
-    assert(s2n_result_is_ok(s2n_stuffer_validate(output)));
-    assert_stuffer_equivalence(output, &expected_bytes_out, &output_saved_byte);
-    assert(s2n_result_is_ok(s2n_blob_validate(hex_in)));
-    assert_blob_equivalence(hex_in, &expected_hex_in, &old_hex_in_byte);
+    /* Only the data in the blob changes, so check equivalent without a saved byte */
+    assert(s2n_result_is_ok(s2n_blob_validate(output)));
+    assert_blob_equivalence(output, &old_output, NULL);
 }

tests/testlib/s2n_hex_testlib.c

@@ -25,15 +25,17 @@ S2N_RESULT s2n_stuffer_alloc_from_hex(struct s2n_stuffer *bytes_out, const char
     RESULT_ENSURE_REF(bytes_out);
     RESULT_ENSURE_REF(hex_cstr);
 
-    DEFER_CLEANUP(struct s2n_blob hex = { 0 }, s2n_free);
+    DEFER_CLEANUP(struct s2n_stuffer hex = { 0 }, s2n_stuffer_free);
     /* Copying the hex into heap memory to handle the 'const' isn't exactly efficient,
      * but for a testlib method it is sufficient.
      */
-    RESULT_GUARD_POSIX(s2n_alloc(&hex, strlen(hex_cstr)));
-    RESULT_CHECKED_MEMCPY(hex.data, hex_cstr, hex.size);
+    RESULT_GUARD_POSIX(s2n_stuffer_alloc(&hex, strlen(hex_cstr)));
+    RESULT_GUARD_POSIX(s2n_stuffer_write_str(&hex, hex_cstr));
 
-    RESULT_GUARD_POSIX(s2n_stuffer_alloc(bytes_out, strlen(hex_cstr) / 2));
-    RESULT_GUARD(s2n_stuffer_read_hex(bytes_out, &hex));
+    uint32_t bytes_size = strlen(hex_cstr) / 2;
+    RESULT_GUARD_POSIX(s2n_stuffer_alloc(bytes_out, bytes_size));
+    RESULT_GUARD(s2n_stuffer_read_hex(&hex, &bytes_out->blob));
+    RESULT_GUARD_POSIX(s2n_stuffer_skip_write(bytes_out, bytes_size));
     return S2N_RESULT_OK;
 }
 
@@ -51,16 +53,8 @@ S2N_RESULT s2n_blob_alloc_from_hex_with_whitespace(struct s2n_blob *bytes_out, c
         RESULT_GUARD_POSIX(s2n_stuffer_write_uint8(&hex_in, hex_cstr[i]));
     }
     uint32_t hex_in_size = s2n_stuffer_data_available(&hex_in);
-    hex_in.blob.size = hex_in_size;
 
-    DEFER_CLEANUP(struct s2n_blob bytes_out_mem = { 0 }, s2n_free);
-    RESULT_GUARD_POSIX(s2n_alloc(&bytes_out_mem, hex_in_size / 2));
-
-    struct s2n_stuffer bytes_out_stuffer = { 0 };
-    RESULT_GUARD_POSIX(s2n_stuffer_init(&bytes_out_stuffer, &bytes_out_mem));
-    RESULT_GUARD(s2n_stuffer_read_hex(&bytes_out_stuffer, &hex_in.blob));
-
-    *bytes_out = bytes_out_mem;
-    ZERO_TO_DISABLE_DEFER_CLEANUP(bytes_out_mem);
+    RESULT_GUARD_POSIX(s2n_alloc(bytes_out, hex_in_size / 2));
+    RESULT_GUARD(s2n_stuffer_read_hex(&hex_in, bytes_out));
     return S2N_RESULT_OK;
 }

tests/unit/s2n_self_talk_key_log_test.c

@@ -180,16 +180,10 @@ int main(int argc, char **argv)
         EXPECT_SUCCESS(s2n_blob_init(&raw_bytes, bytes, sizeof(bytes)));
         EXPECT_OK(s2n_stuffer_write_hex(&encoded, &raw_bytes));
 
-        DEFER_CLEANUP(struct s2n_stuffer decoded, s2n_stuffer_free);
-        EXPECT_SUCCESS(s2n_stuffer_alloc(&decoded, sizeof(bytes)));
-
-        encoded.blob.size = s2n_stuffer_data_available(&encoded);
-        EXPECT_OK(s2n_stuffer_read_hex(&decoded, &encoded.blob));
-
-        uint8_t *out = s2n_stuffer_raw_read(&decoded, s2n_stuffer_data_available(&decoded));
-        EXPECT_NOT_NULL(out);
-
-        EXPECT_EQUAL(memcmp(bytes, out, sizeof(bytes)), 0);
+        DEFER_CLEANUP(struct s2n_blob decoded, s2n_free);
+        EXPECT_SUCCESS(s2n_alloc(&decoded, sizeof(bytes)));
+        EXPECT_OK(s2n_stuffer_read_hex(&encoded, &decoded));
+        EXPECT_BYTEARRAY_EQUAL(decoded.data, bytes, sizeof(bytes));
     };
 
     END_TEST();

tests/unit/s2n_stuffer_hex_test.c

@@ -135,14 +135,12 @@ int main(int argc, char **argv)
                 EXPECT_SUCCESS(s2n_stuffer_alloc(&hex_in, expected_size));
                 EXPECT_SUCCESS(s2n_stuffer_write_text(&hex_in, test_cases[i].hex, expected_size));
 
-                DEFER_CLEANUP(struct s2n_stuffer num_out = { 0 }, s2n_stuffer_free);
-                EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&num_out, 0));
-                EXPECT_OK(s2n_stuffer_read_hex(&num_out, &hex_in.blob));
-
                 uint8_t actual_num = 0;
-                EXPECT_SUCCESS(s2n_stuffer_read_uint8(&num_out, &actual_num));
+                struct s2n_blob num_out = { 0 };
+                EXPECT_SUCCESS(s2n_blob_init(&num_out, &actual_num, 1));
+                EXPECT_OK(s2n_stuffer_read_hex(&hex_in, &num_out));
                 EXPECT_EQUAL(actual_num, test_cases[i].num);
-                EXPECT_FALSE(s2n_stuffer_data_available(&num_out));
+                EXPECT_FALSE(s2n_stuffer_data_available(&hex_in));
             };
         }
     };
@@ -237,8 +235,10 @@ int main(int argc, char **argv)
                 EXPECT_SUCCESS(s2n_stuffer_write_text(&hex_in, test_cases[i].hex, expected_size));
 
                 DEFER_CLEANUP(struct s2n_stuffer num_out = { 0 }, s2n_stuffer_free);
-                EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&num_out, 0));
-                EXPECT_OK(s2n_stuffer_read_hex(&num_out, &hex_in.blob));
+                EXPECT_SUCCESS(s2n_stuffer_alloc(&num_out, sizeof(uint16_t)));
+                EXPECT_OK(s2n_stuffer_read_hex(&hex_in, &num_out.blob));
+                EXPECT_SUCCESS(s2n_stuffer_skip_write(&num_out, &num_out.blob.size));
+                EXPECT_FALSE(s2n_stuffer_data_available(&hex_in));
 
                 uint16_t actual_num = 0;
                 EXPECT_SUCCESS(s2n_stuffer_read_uint16(&num_out, &actual_num));
@@ -332,15 +332,11 @@ int main(int argc, char **argv)
                 EXPECT_SUCCESS(s2n_stuffer_alloc(&hex_in, hex_size));
                 EXPECT_SUCCESS(s2n_stuffer_write_text(&hex_in, test_cases[i].hex, hex_size));
 
-                DEFER_CLEANUP(struct s2n_stuffer num_out = { 0 }, s2n_stuffer_free);
-                EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&num_out, 0));
-                EXPECT_OK(s2n_stuffer_read_hex(&num_out, &hex_in.blob));
-
-                size_t actual_size = s2n_stuffer_data_available(&num_out);
-                EXPECT_EQUAL(actual_size, test_cases[i].bytes_size);
-
-                const uint8_t *actual_bytes = s2n_stuffer_raw_read(&num_out, actual_size);
-                EXPECT_BYTEARRAY_EQUAL(actual_bytes, test_cases[i].bytes, actual_size);
+                DEFER_CLEANUP(struct s2n_blob num_out = { 0 }, s2n_free);
+                EXPECT_SUCCESS(s2n_alloc(&num_out, test_cases[i].bytes_size));
+                EXPECT_OK(s2n_stuffer_read_hex(&hex_in, &num_out));
+                EXPECT_BYTEARRAY_EQUAL(num_out.data, test_cases[i].bytes, test_cases[i].bytes_size);
+                EXPECT_FALSE(s2n_stuffer_data_available(&hex_in));
             };
         }
     };
@@ -393,17 +389,13 @@ int main(int argc, char **argv)
                     EXPECT_SUCCESS(s2n_stuffer_alloc(&hex, strlen(test_hex)));
                     EXPECT_SUCCESS(s2n_stuffer_write_str(&hex, test_hex));
 
-                    DEFER_CLEANUP(struct s2n_stuffer out = { 0 }, s2n_stuffer_free);
-                    EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&out, 0));
+                    DEFER_CLEANUP(struct s2n_blob out = { 0 }, s2n_free);
+                    EXPECT_SUCCESS(s2n_alloc(&out, sizeof(uint8_t)));
                     if (i == 0) {
-                        EXPECT_OK(s2n_stuffer_read_hex(&out, &hex.blob));
-                    } else if (strlen(test_hex) == 0) {
-                        /* Unlike s2n_stuffer_read_uint8_hex, read_hex accepts
-                         * empty input since it has no size expectations */
-                        EXPECT_OK(s2n_stuffer_read_hex(&out, &hex.blob));
+                        EXPECT_OK(s2n_stuffer_read_hex(&hex, &out));
                     } else {
                         EXPECT_ERROR_WITH_ERRNO(
-                                s2n_stuffer_read_hex(&out, &hex.blob),
+                                s2n_stuffer_read_hex(&hex, &out),
                                 S2N_ERR_BAD_HEX);
                     }
                 };
@@ -456,18 +448,13 @@ int main(int argc, char **argv)
                     EXPECT_SUCCESS(s2n_stuffer_alloc(&hex, strlen(test_hex)));
                     EXPECT_SUCCESS(s2n_stuffer_write_str(&hex, test_hex));
 
-                    DEFER_CLEANUP(struct s2n_stuffer out = { 0 }, s2n_stuffer_free);
-                    EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&out, 0));
+                    DEFER_CLEANUP(struct s2n_blob out = { 0 }, s2n_free);
+                    EXPECT_SUCCESS(s2n_alloc(&out, sizeof(uint16_t)));
                     if (i == 0) {
-                        EXPECT_OK(s2n_stuffer_read_hex(&out, &hex.blob));
-                    } else if (strlen(test_hex) < sizeof(uint16_t) * 2
-                            && strlen(test_hex) % 2 == 0) {
-                        /* Unlike s2n_stuffer_read_uint16_hex, read_hex accepts
-                         * input of any valid size */
-                        EXPECT_OK(s2n_stuffer_read_hex(&out, &hex.blob));
+                        EXPECT_OK(s2n_stuffer_read_hex(&hex, &out));
                     } else {
                         EXPECT_ERROR_WITH_ERRNO(
-                                s2n_stuffer_read_hex(&out, &hex.blob),
+                                s2n_stuffer_read_hex(&hex, &out),
                                 S2N_ERR_BAD_HEX);
                     }
                 };
@@ -568,14 +555,11 @@ int main(int argc, char **argv)
                     }
                     EXPECT_FALSE(s2n_stuffer_data_available(&hex));
                 } else if (reader_i == S2N_TEST_N) {
-                    DEFER_CLEANUP(struct s2n_stuffer output = { 0 }, s2n_stuffer_free);
-                    EXPECT_SUCCESS(s2n_stuffer_growable_alloc(&output, 0));
-
-                    hex.blob.size = s2n_stuffer_data_available(&hex);
-                    EXPECT_OK(s2n_stuffer_read_hex(&output, &hex.blob));
-
-                    EXPECT_EQUAL(s2n_stuffer_data_available(&output), sizeof(values_u8));
-                    EXPECT_BYTEARRAY_EQUAL(values_u8, output.blob.data, sizeof(values_u8));
+                    DEFER_CLEANUP(struct s2n_blob output = { 0 }, s2n_free);
+                    EXPECT_SUCCESS(s2n_alloc(&output, sizeof(values_u8)));
+                    EXPECT_OK(s2n_stuffer_read_hex(&hex, &output));
+                    EXPECT_EQUAL(s2n_stuffer_data_available(&hex), 0);
+                    EXPECT_BYTEARRAY_EQUAL(values_u8, output.data, output.size);
                 } else {
                     FAIL_MSG("unknown hex method");
                 }

tls/s2n_fingerprint.c

@@ -287,12 +287,13 @@ int s2n_client_hello_get_fingerprint_hash(struct s2n_client_hello *ch, s2n_finge
      * but s2n_fingerprint_get_hash returns a hex string instead.
      * We need to translate back to the raw bytes.
      */
-    struct s2n_stuffer bytes_out = { 0 };
-    POSIX_GUARD(s2n_blob_init(&bytes_out.blob, output, max_output_size));
-    struct s2n_blob hex_in = { 0 };
-    POSIX_GUARD(s2n_blob_init(&hex_in, hex_hash, hex_hash_size));
-    POSIX_GUARD_RESULT(s2n_stuffer_read_hex(&bytes_out, &hex_in));
-    *output_size = s2n_stuffer_data_available(&bytes_out);
+    struct s2n_blob bytes_out = { 0 };
+    POSIX_GUARD(s2n_blob_init(&bytes_out, output, MD5_DIGEST_LENGTH));
+    struct s2n_stuffer hex_in = { 0 };
+    POSIX_GUARD(s2n_blob_init(&hex_in.blob, hex_hash, hex_hash_size));
+    POSIX_GUARD(s2n_stuffer_skip_write(&hex_in, hex_hash_size));
+    POSIX_GUARD_RESULT(s2n_stuffer_read_hex(&hex_in, &bytes_out));
+    *output_size = bytes_out.size;
 
     POSIX_GUARD(s2n_fingerprint_get_raw_size(&fingerprint, str_size));
     return S2N_SUCCESS;