memory size/expectation mismatch, hash()/to_base64()

[dhoke4tdb]

sdk release 0.3.0, problem general, but periodically manifests as SIGSTOP on azure pipelines CI mac runner, appears because of data that is allocated right up against end of mapped area with nothing mapped beyond

background... hash.cpp/std::string hash(const std::string &to_sign, const std::vector &key) defines

        unsigned int digest_length = SHA256_DIGEST_LENGTH;
        unsigned char digest[SHA256_DIGEST_LENGTH];

elsewhere with

include\hash.h:#define SHA256_DIGEST_LENGTH    32

and hash() makes call

        return to_base64(std::vector<unsigned char>(digest, digest + digest_length));

with base64.cpp containing

    std::string to_base64(const std::vector<unsigned char> &input)
    {
        return to_base64(input.data(), input.size());
    }

    std::string to_base64(const unsigned char* input, size_t size)
    {
        auto ptr = input;

        std::string result;
        result.reserve((size / 3 + 1) * 4);

        for (; size >= 3;)
...

Observe that SHA256_DIGEST_LENGTH used to define the memory size ('unsigned char digest[SHA256_DIGEST_LENGTH];') passed into to_base64() is NOT a multiple of 3, but that the -inner- to_base64() with its for() loop incrementing by 3 expects and processes data as if the memory underLying 'input' will be a multiple of 3.

This leads to falling out of the loop with a length remainder of 2.

The generated code, on the mac at least, appears to be under some circumstances attempting to reference a 3rd byte from the beginning of that last entity. When the data is not allocated right at the end of the segment, this is presumably referencing logically undefined memory, and hence logically wrong. If the data of the vector happens to be allocated right at the end of a mapped memory area with NO memory mapped beyond that last address, a SIGSTOP is generated. Following are debugger snippets from the SIGSTOP situation.

       r11 = 0xfffffff9a36d1c97
       r12 = 0x0000000108103983  "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"
       r13 = 0x00007ff886cffffe
       r14 = 0x00007ffee8c4bcb8
...
   78           {
   79               const _triple_byte* record = reinterpret_cast<const _triple_byte*>(ptr);
** 80               unsigned char idx0 = record->_0;

    0x107dc825a <+330>: movzwl (%r13), %eax
->  0x107dc825f <+335>: movzbl 0x2(%r13), %ebx
    0x107dc8264 <+340>: shll   $0x10, %ebx
    0x107dc8267 <+343>: orl    %eax, %ebx
    0x107dc8269 <+345>: movl   %ebx, %eax
    0x107dc826b <+347>: shrl   $0x2, %eax
    0x107dc826e <+350>: andl   $0x3f, %eax
...
2022-01-10T12:52:09.2734350Z (lldb) bt all
2022-01-10T12:52:09.2738110Z tiledb_unit was compiled with optimization - stepping may behave oddly; variables may not be available.
2022-01-10T12:52:14.0292050Z * thread #1, stop reason = signal SIGSTOP
2022-01-10T12:52:14.0293960Z   * frame #0: 0x0000000107dc825f tiledb_unit`azure::storage_lite::to_base64(input=<unavailable>, size=2) at base64.cpp:80:42 [opt]
2022-01-10T12:52:14.0295040Z     frame #1: 0x0000000107dc80fb tiledb_unit`azure::storage_lite::to_base64(input=size=32) at base64.cpp:40:16 [opt]
2022-01-10T12:52:14.0295980Z     frame #2: 0x0000000107dc889d tiledb_unit`azure::storage_lite::hash(to_sign=<unavailable>, key=size=64) at hash.cpp:65:16 [opt]
2022-01-10T12:52:14.0297170Z     frame #3: 0x0000000107ddc48f tiledb_unit`azure::storage_lite::shared_key_credential::sign_request(this=<unavailable>, (null)=<unavailable>, h=0x00007ff88c30bdd0, url=0x00007ffee8c4be48, headers=<unavailable>) const at storage_credential.cpp:65:65 [opt]
2022-01-10T12:52:14.0298740Z     frame #4: 0x0000000107de18f7 tiledb_unit`azure::storage_lite::delete_blob_request_base::build_request(this=0x00007ff88c31ab28, a=<unavailable>, h=0x00007ff88c30bdd0) const at delete_blob_request_base.cpp:47:25 [opt]

[Jinming-Hu]

Hi @dhoke4tdb , I can confirm there's a bug in to_base64 implementation.

Size of struct _triple_byte is 3 bytes. Assigning a pointer of only 1 or 2-byte data to this struct is undefined behavior. https://github.com/Azure/azure-storage-cpplite/blob/979c59ebe1c247c1126524953c31d75693110ce9/src/base64.cpp#L68 https://github.com/Azure/azure-storage-cpplite/blob/979c59ebe1c247c1126524953c31d75693110ce9/src/base64.cpp#L79

However, cpplite sdk has been deprecated, so I'm afraid we won't fix it. Please use our Track2 SDK instead.

[dhoke4tdb]

Find attached a zip archive containing files involved in addressing this issue, base64.cpp.patch - a patch gen'd against v0.3.0 of sdk (and applicable to several others that seem to have same exact base64.cpp) verify.base64.cpp.both - a complete file containing both the broken and the fixed implementation that was used attempting to validate that the fix actually did avoid the problem, having the _mod() version succeed followed by the _orig() version failing. base64-patch-related.zip

[Jinming-Hu]

Thanks @dhoke4tdb , I believe your patch can fix the bug!

[dhoke4tdb]

One more file, a CI log from our verification attempt with backtrace indicating the _mod routine had succeeded before the _orig routine failed (in the same fashion we have been sporadically encountering) since _mod executed before _orig in the 'both' code. print-stack-raw.log