serenity icon indicating copy to clipboard operation
serenity copied to clipboard

LibCrypto: UnsignedBigInteger::export_data has unexpected alignment behavior

Open ADKaster opened this issue 2 years ago • 0 comments

Given the signature:

size_t UnsignedBigInteger::export_data(Bytes data, bool remove_leading_zeros) const

I would expect that this method, when passed remove_leading_zeroes, will always left-align the first non-zero byte into the data buffer.

However, this is not how it works. It will always copy a multiple of Word (u32) bytes into the data buffer, and return the total number of 'meaningful' bytes copied.

This means that if data is larger than trimmed_length() * sizeof(Word) (or trimmed_byte_length()), it becomes quite annoying to recover only the meaningful bytes when passing remove_leading_zeroes=true.

In any case, the current behavior seems to be that if the BigInt has an actual byte length of 5, 6 or 7 bytes without leading zeroes (or other non-word-aligned size), the bytes of the first word are right-aligned to the first byte. This is super unintuitive. The parameter name is "remove_leading_zeroes", not "return_how_many_bytes_remain_after_leading_zeroes_removed_please_realign_your_buffer_pointer_accordingly".

This test should be changed like so

TEST_CASE(test_bigint_big_endian_export)
{
    auto number = "448378203247"_bigint;
    char exported[8] { 0 };
    auto exported_length = number.export_data({ exported, 8 }, true);
    EXPECT_EQ(exported_length, 5u);
-   EXPECT(memcmp(exported + 3, "hello", 5) == 0);
+   EXPECT(memcmp(exported, "hello", 5) == 0);
}

and still pass.

ADKaster avatar Mar 14 '24 02:03 ADKaster