Fix a few UBSan failures hit by tests #6649
Conversation
| // Now load `ValueBase::chunk_size` rows from from the leaf into m_storage. | ||
| if constexpr (std::is_same_v<U, int64_t>) { | ||
| // If it's an integer leaf, then it contains the method get_chunk() which copies | ||
| // these values in a super fast way (only feasible if more than chunk_size in column) |
There was a problem hiding this comment.
"only feasible if more than chunk_size in column" was actually just always incorrect and this always could have used get_chunk().
| // To make Valgrind happy. Todo, I *think* it should work without, now, but if it reappears, add memset again. | ||
| // memset(res, 0, 8*8); | ||
|
|
||
| if (REALM_X86_OR_X64_TRUE && (w == 1 || w == 2 || w == 4) && ndx + 32 < m_size) { |
There was a problem hiding this comment.
This was limited to x86 because unaligned loads work there, but it's illegal c++ regardless and loading byte per byte gets most of the speed benefit anyway.
| @@ -1168,62 +1168,75 @@ void Array::update_width_cache_from_header() noexcept | |||
| } | |||
|
|
|||
| // This method reads 8 concecutive values into res[8], starting from index 'ndx'. It's allowed for the 8 values to | |||
| // exceed array length; in this case, remainder of res[8] will be left untouched. | |||
| // exceed array length; in this case, remainder of res[8] will be be set to 0. | |||
There was a problem hiding this comment.
This comment's been wrong for a long time.
|
Added a changelog entry for the sectioned results bug fix as v13.12.0 was released with the bug. |
| @@ -142,7 +142,7 @@ ref_type ArrayBlob::replace(size_t begin, size_t end, const char* data, size_t d | |||
| return new_root.blob_replace(begin, end, data, data_size, add_zero_term); | |||
| } | |||
|
|
|||
| if (remove_size == add_size && is_read_only() && memcmp(m_data + begin, data, data_size) == 0) | |||
| if (remove_size == add_size && is_read_only() && (data_size == 0 || memcmp(m_data + begin, data, data_size) == 0)) | |||
There was a problem hiding this comment.
What was this one about? Does memcmp actually access the memory if the length is 0?
I'm trying to understand if this could be a fix for stack traces like the one reported in #6463
There was a problem hiding this comment.
memcmp is defined as taking non-null parameters and passing null is UB. In practice all implementations are fine with null if the length is zero but UBSan complains anyway.
The sectioned results change is fixing an actual out of bounds read, but it hasn't been released yet so no changelog entry for it.
Sanitizer failures haven't been resulting in the CI job actually failing so they just slip through unnoticed.
-fno-sanitize-recover=allmakes it so that the process aborts with an error after a sanitizer failure.