ARROW-5380: [C++] Fix memory alignment UBSan errors.#4757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
emkornfield wants to merge 2 commits into from
Closed

ARROW-5380: [C++] Fix memory alignment UBSan errors. #4757

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db49fbb
Ubsan excluding flatbuffers
emkornfield Jun 30, 2019
5528584
remove TODO
emkornfield Jul 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3,409 changes: 1,969 additions & 1,440 deletions cpp/src/arrow/util/bpacking.h
View file
Open in desktop

Large diffs are not rendered by default.

9 changes: 4 additions & 5 deletions cpp/src/arrow/util/hashing.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -149,9 +149,8 @@ hash_t ComputeStringHash(const void* data, int64_t length) {
// the results
uint32_t x, y;
hash_t hx, hy;
// XXX those are unaligned accesses. Should we have a facility for that?
x = *reinterpret_cast<const uint32_t*>(p + n - 4);
y = *reinterpret_cast<const uint32_t*>(p);
x = util::SafeLoadAs<uint32_t>(p + n - 4);
y = util::SafeLoadAs<uint32_t>(p);
hx = ScalarHelper<uint32_t, AlgNum>::ComputeHash(x);
hy = ScalarHelper<uint32_t, AlgNum ^ 1>::ComputeHash(y);
return n ^ hx ^ hy;
Expand All@@ -160,8 +159,8 @@ hash_t ComputeStringHash(const void* data, int64_t length) {
// Apply the same principle as above
uint64_t x, y;
hash_t hx, hy;
x = *reinterpret_cast<const uint64_t*>(p + n - 8);
y = *reinterpret_cast<const uint64_t*>(p);
x = util::SafeLoadAs<uint64_t>(p + n - 8);
y = util::SafeLoadAs<uint64_t>(p);
hx = ScalarHelper<uint64_t, AlgNum>::ComputeHash(x);
hy = ScalarHelper<uint64_t, AlgNum ^ 1>::ComputeHash(y);
return n ^ hx ^ hy;
Expand Down
16 changes: 16 additions & 0 deletions cpp/src/arrow/util/ubsan.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -49,5 +49,21 @@ inline T* MakeNonNull(T* maybe_null) {
return reinterpret_cast<T*>(&internal::non_null_filler);
}

template <typename T>

@fsaintjacquesfsaintjacquesJul 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no C++ guru, but can't you make a single method for this by templating the input type too, e.g.

template <typename T, typename I = uint8_t>
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoad(
const I* unaligned) {
typename std::remove_const<T>::type ret;
std::memcpy(&ret, unaligned, sizeof(T));
return ret;
}

@emkornfieldemkornfieldJul 2, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct, I think I like how the methods are now though since it is more explicit when casting and a pass-through when not. One method could certainly delegate to the other.

inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoadAs(
const uint8_t* unaligned) {
typename std::remove_const<T>::type ret;
std::memcpy(&ret, unaligned, sizeof(T));
return ret;
}

template <typename T>
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoad(
const T* unaligned) {
typename std::remove_const<T>::type ret;
std::memcpy(&ret, unaligned, sizeof(T));
return ret;
}

} // namespace util
} // namespace arrow
20 changes: 10 additions & 10 deletions cpp/src/parquet/arrow/reader.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -83,6 +83,7 @@ namespace arrow {

using ::arrow::BitUtil::FromBigEndian;
using ::arrow::internal::SafeLeftShift;
using ::arrow::util::SafeLoadAs;

template <typename ArrowType>
using ArrayType = typename ::arrow::TypeTraits<ArrowType>::ArrayType;
Expand DownExpand Up@@ -1212,38 +1213,37 @@ static uint64_t BytesToInteger(const uint8_t* bytes, int32_t start, int32_t stop
case 1:
return bytes[start];
case 2:
return FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start));
return FromBigEndian(SafeLoadAs<uint16_t>(bytes + start));
case 3: {
const uint64_t first_two_bytes =
FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start));
const uint64_t first_two_bytes = FromBigEndian(SafeLoadAs<uint16_t>(bytes + start));
const uint64_t last_byte = bytes[stop - 1];
return first_two_bytes << 8 | last_byte;
}
case 4:
return FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
return FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
case 5: {
const uint64_t first_four_bytes =
FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
const uint64_t last_byte = bytes[stop - 1];
return first_four_bytes << 8 | last_byte;
}
case 6: {
const uint64_t first_four_bytes =
FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
const uint64_t last_two_bytes =
FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start + 4));
FromBigEndian(SafeLoadAs<uint16_t>(bytes + start + 4));
return first_four_bytes << 16 | last_two_bytes;
}
case 7: {
const uint64_t first_four_bytes =
FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
const uint64_t second_two_bytes =
FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start + 4));
FromBigEndian(SafeLoadAs<uint16_t>(bytes + start + 4));
const uint64_t last_byte = bytes[stop - 1];
return first_four_bytes << 24 | second_two_bytes << 8 | last_byte;
}
case 8:
return FromBigEndian(*reinterpret_cast<const uint64_t*>(bytes + start));
return FromBigEndian(SafeLoadAs<uint64_t>(bytes + start));
default: {
DCHECK(false);
return UINT64_MAX;
Expand Down
5 changes: 3 additions & 2 deletions cpp/src/parquet/arrow/writer.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -211,8 +211,9 @@ inline void ArrowTimestampToImpalaTimestamp(const int64_t time, Int96* impala_ti
(*impala_timestamp).value[2] = (uint32_t)julian_days;

int64_t last_day_units = time % UnitPerDay;
int64_t* impala_last_day_nanos = reinterpret_cast<int64_t*>(impala_timestamp);
*impala_last_day_nanos = last_day_units * NanosecondsPerUnit;
auto last_day_nanos = last_day_units * NanosecondsPerUnit;
// Strage might be unaligned, so use mempcy instead of reinterpret_cast

@fsaintjacquesfsaintjacquesJul 2, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All even indexed Int96 in a vector will be unaligned (according to int64_t alignment).

@emkornfieldemkornfieldJul 2, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point I'll open up a follow-up PR, we must not hav good test data here or UBSan isn't foolproof, or somehow I didn't run this test properly

@emkornfieldemkornfieldJul 3, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait were you just commenting on my comment?

@fsaintjacquesfsaintjacquesJul 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was commenting on your comment on the "strange" part :)

@pitroupitrouJul 11, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you notice the typo? ("Strage")

std::memcpy(impala_timestamp, &last_day_nanos, sizeof(int64_t));
}

constexpr int64_t kSecondsInNanos = INT64_C(1000000000);
Expand Down
3 changes: 2 additions & 1 deletion cpp/src/parquet/column_reader.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -27,6 +27,7 @@
#include "arrow/util/compression.h"
#include "arrow/util/logging.h"
#include "arrow/util/rle-encoding.h"
#include "arrow/util/ubsan.h"

#include "parquet/column_page.h"
#include "parquet/encoding.h"
Expand All@@ -50,7 +51,7 @@ int LevelDecoder::SetData(Encoding::type encoding, int16_t max_level,
bit_width_ = BitUtil::Log2(max_level + 1);
switch (encoding) {
case Encoding::RLE: {
num_bytes = *reinterpret_cast<const int32_t*>(data);
num_bytes = arrow::util::SafeLoadAs<int32_t>(data);
const uint8_t* decoder_data = data + sizeof(int32_t);
if (!rle_decoder_) {
rle_decoder_.reset(
Expand Down
11 changes: 6 additions & 5 deletions cpp/src/parquet/encoding.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -29,6 +29,7 @@
#include "arrow/util/logging.h"
#include "arrow/util/rle-encoding.h"
#include "arrow/util/string_view.h"
#include "arrow/util/ubsan.h"

#include "parquet/exception.h"
#include "parquet/platform.h"
Expand DownExpand Up@@ -609,7 +610,7 @@ inline int DecodePlain<ByteArray>(const uint8_t* data, int64_t data_size, int nu
int bytes_decoded = 0;
int increment;
for (int i = 0; i < num_values; ++i) {
uint32_t len = out[i].len = *reinterpret_cast<const uint32_t*>(data);
uint32_t len = out[i].len = arrow::util::SafeLoadAs<uint32_t>(data);
increment = static_cast<int>(sizeof(uint32_t) + len);
if (data_size < increment) ParquetException::EofException();
out[i].ptr = data + sizeof(uint32_t);
Expand DownExpand Up@@ -719,7 +720,7 @@ class PlainByteArrayDecoder : public PlainDecoder<ByteArrayType>,
int bytes_decoded = 0;
while (i < num_values) {
if (bit_reader.IsSet()) {
uint32_t len = *reinterpret_cast<const uint32_t*>(data);
uint32_t len = arrow::util::SafeLoadAs<uint32_t>(data);
increment = static_cast<int>(sizeof(uint32_t) + len);
if (data_size < increment) {
ParquetException::EofException();
Expand DownExpand Up@@ -752,7 +753,7 @@ class PlainByteArrayDecoder : public PlainDecoder<ByteArrayType>,
int bytes_decoded = 0;

while (i < num_values) {
uint32_t len = *reinterpret_cast<const uint32_t*>(data);
uint32_t len = arrow::util::SafeLoadAs<uint32_t>(data);
int increment = static_cast<int>(sizeof(uint32_t) + len);
if (data_size < increment) ParquetException::EofException();
builder->Append(data + sizeof(uint32_t), len);
Expand DownExpand Up@@ -1103,7 +1104,7 @@ class DeltaLengthByteArrayDecoder : public DecoderImpl,
virtual void SetData(int num_values, const uint8_t* data, int len) {
num_values_ = num_values;
if (len == 0) return;
int total_lengths_len = *reinterpret_cast<const int*>(data);
int total_lengths_len = arrow::util::SafeLoadAs<int32_t>(data);
data += 4;
this->len_decoder_.SetData(num_values, data, total_lengths_len);
data_ = data + total_lengths_len;
Expand DownExpand Up@@ -1145,7 +1146,7 @@ class DeltaByteArrayDecoder : public DecoderImpl,
virtual void SetData(int num_values, const uint8_t* data, int len) {
num_values_ = num_values;
if (len == 0) return;
int prefix_len_length = *reinterpret_cast<const int*>(data);
int prefix_len_length = arrow::util::SafeLoadAs<int32_t>(data);
data += 4;
len -= 4;
prefix_len_decoder_.SetData(num_values, data, prefix_len_length);
Expand Down
3 changes: 2 additions & 1 deletion cpp/src/parquet/file_reader.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -28,6 +28,7 @@
#include "arrow/io/file.h"
#include "arrow/status.h"
#include "arrow/util/logging.h"
#include "arrow/util/ubsan.h"

#include "parquet/column_reader.h"
#include "parquet/column_scanner.h"
Expand DownExpand Up@@ -179,7 +180,7 @@ class SerializedFile : public ParquetFileReader::Contents {
throw ParquetException("Invalid parquet file. Corrupt footer.");
}

uint32_t metadata_len = *reinterpret_cast<const uint32_t*>(
uint32_t metadata_len = arrow::util::SafeLoadAs<uint32_t>(
reinterpret_cast<const uint8_t*>(footer_buffer->data()) + footer_read_size -
kFooterSize);
int64_t metadata_start = file_size - kFooterSize - metadata_len;
Expand Down
4 changes: 3 additions & 1 deletion cpp/src/plasma/common.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -19,6 +19,8 @@

#include <limits>

#include "arrow/util/ubsan.h"

#include "plasma/plasma_generated.h"

namespace fb = plasma::flatbuf;
Expand DownExpand Up@@ -64,7 +66,7 @@ uint64_t MurmurHash64A(const void* key, int len, unsigned int seed) {
const uint64_t* end = data + (len / 8);

while (data != end) {
uint64_t k = *data++;
uint64_t k = arrow::util::SafeLoad(data++);

k *= m;
k ^= k >> r;
Expand Down
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
ARROW-5380: [C++] Fix memory alignment UBSan errors.#4757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Closed

emkornfield
wants to merge
2
commits into

from

 Closed


ARROW-5380: [C++] Fix memory alignment UBSan errors.
#4757
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


2 commits


Select commit
Hold shift + click to select a range
db49fbb

Ubsan excluding flatbuffers


emkornfield Jun 30, 2019
5528584

remove TODO


emkornfield Jul 1, 2019

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


3,409 changes: 1,969 additions & 1,440 deletions
cpp/src/arrow/util/bpacking.h

View file


Open in desktop


Large diffs are not rendered by default.


9 changes: 4 additions & 5 deletions
cpp/src/arrow/util/hashing.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -149,9 +149,8 @@ hash_t ComputeStringHash(const void* data, int64_t length) {
 // the results
 uint32_t x, y;
 hash_t hx, hy;
 // XXX those are unaligned accesses. Should we have a facility for that?
 x = *reinterpret_cast<const uint32_t*>(p + n - 4);
 y = *reinterpret_cast<const uint32_t*>(p);
 x = util::SafeLoadAs<uint32_t>(p + n - 4);
 y = util::SafeLoadAs<uint32_t>(p);
 hx = ScalarHelper<uint32_t, AlgNum>::ComputeHash(x);
 hy = ScalarHelper<uint32_t, AlgNum ^ 1>::ComputeHash(y);
 return n ^ hx ^ hy;
Expand All@@ -160,8 +159,8 @@ hash_t ComputeStringHash(const void* data, int64_t length) {
 // Apply the same principle as above
 uint64_t x, y;
 hash_t hx, hy;
 x = *reinterpret_cast<const uint64_t*>(p + n - 8);
 y = *reinterpret_cast<const uint64_t*>(p);
 x = util::SafeLoadAs<uint64_t>(p + n - 8);
 y = util::SafeLoadAs<uint64_t>(p);
 hx = ScalarHelper<uint64_t, AlgNum>::ComputeHash(x);
 hy = ScalarHelper<uint64_t, AlgNum ^ 1>::ComputeHash(y);
 return n ^ hx ^ hy;
Expand Down

16 changes: 16 additions & 0 deletions
cpp/src/arrow/util/ubsan.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -49,5 +49,21 @@ inline T* MakeNonNull(T* maybe_null) {
 return reinterpret_cast<T*>(&internal::non_null_filler);
}

template <typename T>
@fsaintjacquesfsaintjacquesJul 2, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no C++ guru, but can't you make a single method for this by templating the input type too, e.g.
template <typename T, typename I = uint8_t>
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoad(
const I* unaligned) {
typename std::remove_const<T>::type ret;
std::memcpy(&ret, unaligned, sizeof(T));
return ret;
}

@emkornfieldemkornfieldJul 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct, I think I like how the methods are now though since it is more explicit when casting and a pass-through when not. One method could certainly delegate to the other.
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoadAs(
 const uint8_t* unaligned) {
 typename std::remove_const<T>::type ret;
 std::memcpy(&ret, unaligned, sizeof(T));
 return ret;
}

template <typename T>
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoad(
 const T* unaligned) {
 typename std::remove_const<T>::type ret;
 std::memcpy(&ret, unaligned, sizeof(T));
 return ret;
}

} // namespace util
} // namespace arrow

20 changes: 10 additions & 10 deletions
cpp/src/parquet/arrow/reader.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -83,6 +83,7 @@ namespace arrow {

using ::arrow::BitUtil::FromBigEndian;
using ::arrow::internal::SafeLeftShift;
using ::arrow::util::SafeLoadAs;

template <typename ArrowType>
using ArrayType = typename ::arrow::TypeTraits<ArrowType>::ArrayType;
Expand DownExpand Up@@ -1212,38 +1213,37 @@ static uint64_t BytesToInteger(const uint8_t* bytes, int32_t start, int32_t stop
 case 1:
 return bytes[start];
 case 2:
 return FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start));
 return FromBigEndian(SafeLoadAs<uint16_t>(bytes + start));
 case 3: {
 const uint64_t first_two_bytes =
 FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start));
 const uint64_t first_two_bytes = FromBigEndian(SafeLoadAs<uint16_t>(bytes + start));
 const uint64_t last_byte = bytes[stop - 1];
 return first_two_bytes << 8 | last_byte;
 }
 case 4:
 return FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 return FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 case 5: {
 const uint64_t first_four_bytes =
 FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 const uint64_t last_byte = bytes[stop - 1];
 return first_four_bytes << 8 | last_byte;
 }
 case 6: {
 const uint64_t first_four_bytes =
 FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 const uint64_t last_two_bytes =
 FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start + 4));
 FromBigEndian(SafeLoadAs<uint16_t>(bytes + start + 4));
 return first_four_bytes << 16 | last_two_bytes;
 }
 case 7: {
 const uint64_t first_four_bytes =
 FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 const uint64_t second_two_bytes =
 FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start + 4));
 FromBigEndian(SafeLoadAs<uint16_t>(bytes + start + 4));
 const uint64_t last_byte = bytes[stop - 1];
 return first_four_bytes << 24 | second_two_bytes << 8 | last_byte;
 }
 case 8:
 return FromBigEndian(*reinterpret_cast<const uint64_t*>(bytes + start));
 return FromBigEndian(SafeLoadAs<uint64_t>(bytes + start));
 default: {
 DCHECK(false);
 return UINT64_MAX;
Expand Down

5 changes: 3 additions & 2 deletions
cpp/src/parquet/arrow/writer.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -211,8 +211,9 @@ inline void ArrowTimestampToImpalaTimestamp(const int64_t time, Int96* impala_ti
 (*impala_timestamp).value[2] = (uint32_t)julian_days;

 int64_t last_day_units = time % UnitPerDay;
 int64_t* impala_last_day_nanos = reinterpret_cast<int64_t*>(impala_timestamp);
 *impala_last_day_nanos = last_day_units * NanosecondsPerUnit;
 auto last_day_nanos = last_day_units * NanosecondsPerUnit;
 // Strage might be unaligned, so use mempcy instead of reinterpret_cast
@fsaintjacquesfsaintjacquesJul 2, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All even indexed Int96 in a vector will be unaligned (according to int64_t alignment).
@emkornfieldemkornfieldJul 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point I'll open up a follow-up PR, we must not hav good test data here or UBSan isn't foolproof, or somehow I didn't run this test properly
@emkornfieldemkornfieldJul 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait were you just commenting on my comment?
@fsaintjacquesfsaintjacquesJul 11, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was commenting on your comment on the "strange" part :)
@pitroupitrouJul 11, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you notice the typo? ("Strage")
 std::memcpy(impala_timestamp, &last_day_nanos, sizeof(int64_t));
}

constexpr int64_t kSecondsInNanos = INT64_C(1000000000);
Expand Down

3 changes: 2 additions & 1 deletion
cpp/src/parquet/column_reader.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -27,6 +27,7 @@
#include "arrow/util/compression.h"
#include "arrow/util/logging.h"
#include "arrow/util/rle-encoding.h"
#include "arrow/util/ubsan.h"

#include "parquet/column_page.h"
#include "parquet/encoding.h"
Expand All@@ -50,7 +51,7 @@ int LevelDecoder::SetData(Encoding::type encoding, int16_t max_level,
 bit_width_ = BitUtil::Log2(max_level + 1);
 switch (encoding) {
 case Encoding::RLE: {
 num_bytes = *reinterpret_cast<const int32_t*>(data);
 num_bytes = arrow::util::SafeLoadAs<int32_t>(data);
 const uint8_t* decoder_data = data + sizeof(int32_t);
 if (!rle_decoder_) {
 rle_decoder_.reset(
Expand Down

11 changes: 6 additions & 5 deletions
cpp/src/parquet/encoding.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -29,6 +29,7 @@
#include "arrow/util/logging.h"
#include "arrow/util/rle-encoding.h"
#include "arrow/util/string_view.h"
#include "arrow/util/ubsan.h"

#include "parquet/exception.h"
#include "parquet/platform.h"
Expand DownExpand Up@@ -609,7 +610,7 @@ inline int DecodePlain<ByteArray>(const uint8_t* data, int64_t data_size, int nu
 int bytes_decoded = 0;
 int increment;
 for (int i = 0; i < num_values; ++i) {
 uint32_t len = out[i].len = *reinterpret_cast<const uint32_t*>(data);
 uint32_t len = out[i].len = arrow::util::SafeLoadAs<uint32_t>(data);
 increment = static_cast<int>(sizeof(uint32_t) + len);
 if (data_size < increment) ParquetException::EofException();
 out[i].ptr = data + sizeof(uint32_t);
Expand DownExpand Up@@ -719,7 +720,7 @@ class PlainByteArrayDecoder : public PlainDecoder<ByteArrayType>,
 int bytes_decoded = 0;
 while (i < num_values) {
 if (bit_reader.IsSet()) {
 uint32_t len = *reinterpret_cast<const uint32_t*>(data);
 uint32_t len = arrow::util::SafeLoadAs<uint32_t>(data);
 increment = static_cast<int>(sizeof(uint32_t) + len);
 if (data_size < increment) {
 ParquetException::EofException();
Expand DownExpand Up@@ -752,7 +753,7 @@ class PlainByteArrayDecoder : public PlainDecoder<ByteArrayType>,
 int bytes_decoded = 0;

 while (i < num_values) {
 uint32_t len = *reinterpret_cast<const uint32_t*>(data);
 uint32_t len = arrow::util::SafeLoadAs<uint32_t>(data);
 int increment = static_cast<int>(sizeof(uint32_t) + len);
 if (data_size < increment) ParquetException::EofException();
 builder->Append(data + sizeof(uint32_t), len);
Expand DownExpand Up@@ -1103,7 +1104,7 @@ class DeltaLengthByteArrayDecoder : public DecoderImpl,
 virtual void SetData(int num_values, const uint8_t* data, int len) {
 num_values_ = num_values;
 if (len == 0) return;
 int total_lengths_len = *reinterpret_cast<const int*>(data);
 int total_lengths_len = arrow::util::SafeLoadAs<int32_t>(data);
 data += 4;
 this->len_decoder_.SetData(num_values, data, total_lengths_len);
 data_ = data + total_lengths_len;
Expand DownExpand Up@@ -1145,7 +1146,7 @@ class DeltaByteArrayDecoder : public DecoderImpl,
 virtual void SetData(int num_values, const uint8_t* data, int len) {
 num_values_ = num_values;
 if (len == 0) return;
 int prefix_len_length = *reinterpret_cast<const int*>(data);
 int prefix_len_length = arrow::util::SafeLoadAs<int32_t>(data);
 data += 4;
 len -= 4;
 prefix_len_decoder_.SetData(num_values, data, prefix_len_length);
Expand Down

3 changes: 2 additions & 1 deletion
cpp/src/parquet/file_reader.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -28,6 +28,7 @@
#include "arrow/io/file.h"
#include "arrow/status.h"
#include "arrow/util/logging.h"
#include "arrow/util/ubsan.h"

#include "parquet/column_reader.h"
#include "parquet/column_scanner.h"
Expand DownExpand Up@@ -179,7 +180,7 @@ class SerializedFile : public ParquetFileReader::Contents {
 throw ParquetException("Invalid parquet file. Corrupt footer.");
 }

 uint32_t metadata_len = *reinterpret_cast<const uint32_t*>(
 uint32_t metadata_len = arrow::util::SafeLoadAs<uint32_t>(
 reinterpret_cast<const uint8_t*>(footer_buffer->data()) + footer_read_size -
 kFooterSize);
 int64_t metadata_start = file_size - kFooterSize - metadata_len;
Expand Down

4 changes: 3 additions & 1 deletion
cpp/src/plasma/common.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -19,6 +19,8 @@

#include <limits>

#include "arrow/util/ubsan.h"

#include "plasma/plasma_generated.h"

namespace fb = plasma::flatbuf;
Expand DownExpand Up@@ -64,7 +66,7 @@ uint64_t MurmurHash64A(const void* key, int len, unsigned int seed) {
 const uint64_t* end = data + (len / 8);

 while (data != end) {
 uint64_t k = *data++;
 uint64_t k = arrow::util::SafeLoad(data++);

 k *= m;
 k ^= k >> r;
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-5380: [C++] Fix memory alignment UBSan errors.#4757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Closed

emkornfield
wants to merge
2
commits into

from

 Closed


ARROW-5380: [C++] Fix memory alignment UBSan errors.
#4757
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


2 commits


Select commit
Hold shift + click to select a range
db49fbb

Ubsan excluding flatbuffers


emkornfield Jun 30, 2019
5528584

remove TODO


emkornfield Jul 1, 2019

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


3,409 changes: 1,969 additions & 1,440 deletions
cpp/src/arrow/util/bpacking.h

View file


Open in desktop


Large diffs are not rendered by default.


9 changes: 4 additions & 5 deletions
cpp/src/arrow/util/hashing.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -149,9 +149,8 @@ hash_t ComputeStringHash(const void* data, int64_t length) {
 // the results
 uint32_t x, y;
 hash_t hx, hy;
 // XXX those are unaligned accesses. Should we have a facility for that?
 x = *reinterpret_cast<const uint32_t*>(p + n - 4);
 y = *reinterpret_cast<const uint32_t*>(p);
 x = util::SafeLoadAs<uint32_t>(p + n - 4);
 y = util::SafeLoadAs<uint32_t>(p);
 hx = ScalarHelper<uint32_t, AlgNum>::ComputeHash(x);
 hy = ScalarHelper<uint32_t, AlgNum ^ 1>::ComputeHash(y);
 return n ^ hx ^ hy;
Expand All@@ -160,8 +159,8 @@ hash_t ComputeStringHash(const void* data, int64_t length) {
 // Apply the same principle as above
 uint64_t x, y;
 hash_t hx, hy;
 x = *reinterpret_cast<const uint64_t*>(p + n - 8);
 y = *reinterpret_cast<const uint64_t*>(p);
 x = util::SafeLoadAs<uint64_t>(p + n - 8);
 y = util::SafeLoadAs<uint64_t>(p);
 hx = ScalarHelper<uint64_t, AlgNum>::ComputeHash(x);
 hy = ScalarHelper<uint64_t, AlgNum ^ 1>::ComputeHash(y);
 return n ^ hx ^ hy;
Expand Down

16 changes: 16 additions & 0 deletions
cpp/src/arrow/util/ubsan.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -49,5 +49,21 @@ inline T* MakeNonNull(T* maybe_null) {
 return reinterpret_cast<T*>(&internal::non_null_filler);
}

template <typename T>
@fsaintjacquesfsaintjacquesJul 2, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no C++ guru, but can't you make a single method for this by templating the input type too, e.g.
template <typename T, typename I = uint8_t>
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoad(
const I* unaligned) {
typename std::remove_const<T>::type ret;
std::memcpy(&ret, unaligned, sizeof(T));
return ret;
}

@emkornfieldemkornfieldJul 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct, I think I like how the methods are now though since it is more explicit when casting and a pass-through when not. One method could certainly delegate to the other.
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoadAs(
 const uint8_t* unaligned) {
 typename std::remove_const<T>::type ret;
 std::memcpy(&ret, unaligned, sizeof(T));
 return ret;
}

template <typename T>
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoad(
 const T* unaligned) {
 typename std::remove_const<T>::type ret;
 std::memcpy(&ret, unaligned, sizeof(T));
 return ret;
}

} // namespace util
} // namespace arrow

20 changes: 10 additions & 10 deletions
cpp/src/parquet/arrow/reader.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -83,6 +83,7 @@ namespace arrow {

using ::arrow::BitUtil::FromBigEndian;
using ::arrow::internal::SafeLeftShift;
using ::arrow::util::SafeLoadAs;

template <typename ArrowType>
using ArrayType = typename ::arrow::TypeTraits<ArrowType>::ArrayType;
Expand DownExpand Up@@ -1212,38 +1213,37 @@ static uint64_t BytesToInteger(const uint8_t* bytes, int32_t start, int32_t stop
 case 1:
 return bytes[start];
 case 2:
 return FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start));
 return FromBigEndian(SafeLoadAs<uint16_t>(bytes + start));
 case 3: {
 const uint64_t first_two_bytes =
 FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start));
 const uint64_t first_two_bytes = FromBigEndian(SafeLoadAs<uint16_t>(bytes + start));
 const uint64_t last_byte = bytes[stop - 1];
 return first_two_bytes << 8 | last_byte;
 }
 case 4:
 return FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 return FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 case 5: {
 const uint64_t first_four_bytes =
 FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 const uint64_t last_byte = bytes[stop - 1];
 return first_four_bytes << 8 | last_byte;
 }
 case 6: {
 const uint64_t first_four_bytes =
 FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 const uint64_t last_two_bytes =
 FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start + 4));
 FromBigEndian(SafeLoadAs<uint16_t>(bytes + start + 4));
 return first_four_bytes << 16 | last_two_bytes;
 }
 case 7: {
 const uint64_t first_four_bytes =
 FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 const uint64_t second_two_bytes =
 FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start + 4));
 FromBigEndian(SafeLoadAs<uint16_t>(bytes + start + 4));
 const uint64_t last_byte = bytes[stop - 1];
 return first_four_bytes << 24 | second_two_bytes << 8 | last_byte;
 }
 case 8:
 return FromBigEndian(*reinterpret_cast<const uint64_t*>(bytes + start));
 return FromBigEndian(SafeLoadAs<uint64_t>(bytes + start));
 default: {
 DCHECK(false);
 return UINT64_MAX;
Expand Down

5 changes: 3 additions & 2 deletions
cpp/src/parquet/arrow/writer.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -211,8 +211,9 @@ inline void ArrowTimestampToImpalaTimestamp(const int64_t time, Int96* impala_ti
 (*impala_timestamp).value[2] = (uint32_t)julian_days;

 int64_t last_day_units = time % UnitPerDay;
 int64_t* impala_last_day_nanos = reinterpret_cast<int64_t*>(impala_timestamp);
 *impala_last_day_nanos = last_day_units * NanosecondsPerUnit;
 auto last_day_nanos = last_day_units * NanosecondsPerUnit;
 // Strage might be unaligned, so use mempcy instead of reinterpret_cast
@fsaintjacquesfsaintjacquesJul 2, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All even indexed Int96 in a vector will be unaligned (according to int64_t alignment).
@emkornfieldemkornfieldJul 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point I'll open up a follow-up PR, we must not hav good test data here or UBSan isn't foolproof, or somehow I didn't run this test properly
@emkornfieldemkornfieldJul 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait were you just commenting on my comment?
@fsaintjacquesfsaintjacquesJul 11, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was commenting on your comment on the "strange" part :)
@pitroupitrouJul 11, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you notice the typo? ("Strage")
 std::memcpy(impala_timestamp, &last_day_nanos, sizeof(int64_t));
}

constexpr int64_t kSecondsInNanos = INT64_C(1000000000);
Expand Down

3 changes: 2 additions & 1 deletion
cpp/src/parquet/column_reader.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -27,6 +27,7 @@
#include "arrow/util/compression.h"
#include "arrow/util/logging.h"
#include "arrow/util/rle-encoding.h"
#include "arrow/util/ubsan.h"

#include "parquet/column_page.h"
#include "parquet/encoding.h"
Expand All@@ -50,7 +51,7 @@ int LevelDecoder::SetData(Encoding::type encoding, int16_t max_level,
 bit_width_ = BitUtil::Log2(max_level + 1);
 switch (encoding) {
 case Encoding::RLE: {
 num_bytes = *reinterpret_cast<const int32_t*>(data);
 num_bytes = arrow::util::SafeLoadAs<int32_t>(data);
 const uint8_t* decoder_data = data + sizeof(int32_t);
 if (!rle_decoder_) {
 rle_decoder_.reset(
Expand Down

11 changes: 6 additions & 5 deletions
cpp/src/parquet/encoding.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -29,6 +29,7 @@
#include "arrow/util/logging.h"
#include "arrow/util/rle-encoding.h"
#include "arrow/util/string_view.h"
#include "arrow/util/ubsan.h"

#include "parquet/exception.h"
#include "parquet/platform.h"
Expand DownExpand Up@@ -609,7 +610,7 @@ inline int DecodePlain<ByteArray>(const uint8_t* data, int64_t data_size, int nu
 int bytes_decoded = 0;
 int increment;
 for (int i = 0; i < num_values; ++i) {
 uint32_t len = out[i].len = *reinterpret_cast<const uint32_t*>(data);
 uint32_t len = out[i].len = arrow::util::SafeLoadAs<uint32_t>(data);
 increment = static_cast<int>(sizeof(uint32_t) + len);
 if (data_size < increment) ParquetException::EofException();
 out[i].ptr = data + sizeof(uint32_t);
Expand DownExpand Up@@ -719,7 +720,7 @@ class PlainByteArrayDecoder : public PlainDecoder<ByteArrayType>,
 int bytes_decoded = 0;
 while (i < num_values) {
 if (bit_reader.IsSet()) {
 uint32_t len = *reinterpret_cast<const uint32_t*>(data);
 uint32_t len = arrow::util::SafeLoadAs<uint32_t>(data);
 increment = static_cast<int>(sizeof(uint32_t) + len);
 if (data_size < increment) {
 ParquetException::EofException();
Expand DownExpand Up@@ -752,7 +753,7 @@ class PlainByteArrayDecoder : public PlainDecoder<ByteArrayType>,
 int bytes_decoded = 0;

 while (i < num_values) {
 uint32_t len = *reinterpret_cast<const uint32_t*>(data);
 uint32_t len = arrow::util::SafeLoadAs<uint32_t>(data);
 int increment = static_cast<int>(sizeof(uint32_t) + len);
 if (data_size < increment) ParquetException::EofException();
 builder->Append(data + sizeof(uint32_t), len);
Expand DownExpand Up@@ -1103,7 +1104,7 @@ class DeltaLengthByteArrayDecoder : public DecoderImpl,
 virtual void SetData(int num_values, const uint8_t* data, int len) {
 num_values_ = num_values;
 if (len == 0) return;
 int total_lengths_len = *reinterpret_cast<const int*>(data);
 int total_lengths_len = arrow::util::SafeLoadAs<int32_t>(data);
 data += 4;
 this->len_decoder_.SetData(num_values, data, total_lengths_len);
 data_ = data + total_lengths_len;
Expand DownExpand Up@@ -1145,7 +1146,7 @@ class DeltaByteArrayDecoder : public DecoderImpl,
 virtual void SetData(int num_values, const uint8_t* data, int len) {
 num_values_ = num_values;
 if (len == 0) return;
 int prefix_len_length = *reinterpret_cast<const int*>(data);
 int prefix_len_length = arrow::util::SafeLoadAs<int32_t>(data);
 data += 4;
 len -= 4;
 prefix_len_decoder_.SetData(num_values, data, prefix_len_length);
Expand Down

3 changes: 2 additions & 1 deletion
cpp/src/parquet/file_reader.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -28,6 +28,7 @@
#include "arrow/io/file.h"
#include "arrow/status.h"
#include "arrow/util/logging.h"
#include "arrow/util/ubsan.h"

#include "parquet/column_reader.h"
#include "parquet/column_scanner.h"
Expand DownExpand Up@@ -179,7 +180,7 @@ class SerializedFile : public ParquetFileReader::Contents {
 throw ParquetException("Invalid parquet file. Corrupt footer.");
 }

 uint32_t metadata_len = *reinterpret_cast<const uint32_t*>(
 uint32_t metadata_len = arrow::util::SafeLoadAs<uint32_t>(
 reinterpret_cast<const uint8_t*>(footer_buffer->data()) + footer_read_size -
 kFooterSize);
 int64_t metadata_start = file_size - kFooterSize - metadata_len;
Expand Down

4 changes: 3 additions & 1 deletion
cpp/src/plasma/common.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -19,6 +19,8 @@

#include <limits>

#include "arrow/util/ubsan.h"

#include "plasma/plasma_generated.h"

namespace fb = plasma::flatbuf;
Expand DownExpand Up@@ -64,7 +66,7 @@ uint64_t MurmurHash64A(const void* key, int len, unsigned int seed) {
 const uint64_t* end = data + (len / 8);

 while (data != end) {
 uint64_t k = *data++;
 uint64_t k = arrow::util::SafeLoad(data++);

 k *= m;
 k ^= k >> r;
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-5380: [C++] Fix memory alignment UBSan errors.#4757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Closed

emkornfield
wants to merge
2
commits into

from

 Closed


ARROW-5380: [C++] Fix memory alignment UBSan errors.
#4757
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


2 commits


Select commit
Hold shift + click to select a range
db49fbb

Ubsan excluding flatbuffers


emkornfield Jun 30, 2019
5528584

remove TODO


emkornfield Jul 1, 2019

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


3,409 changes: 1,969 additions & 1,440 deletions
cpp/src/arrow/util/bpacking.h

View file


Open in desktop


Large diffs are not rendered by default.


9 changes: 4 additions & 5 deletions
cpp/src/arrow/util/hashing.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -149,9 +149,8 @@ hash_t ComputeStringHash(const void* data, int64_t length) {
 // the results
 uint32_t x, y;
 hash_t hx, hy;
 // XXX those are unaligned accesses. Should we have a facility for that?
 x = *reinterpret_cast<const uint32_t*>(p + n - 4);
 y = *reinterpret_cast<const uint32_t*>(p);
 x = util::SafeLoadAs<uint32_t>(p + n - 4);
 y = util::SafeLoadAs<uint32_t>(p);
 hx = ScalarHelper<uint32_t, AlgNum>::ComputeHash(x);
 hy = ScalarHelper<uint32_t, AlgNum ^ 1>::ComputeHash(y);
 return n ^ hx ^ hy;
Expand All@@ -160,8 +159,8 @@ hash_t ComputeStringHash(const void* data, int64_t length) {
 // Apply the same principle as above
 uint64_t x, y;
 hash_t hx, hy;
 x = *reinterpret_cast<const uint64_t*>(p + n - 8);
 y = *reinterpret_cast<const uint64_t*>(p);
 x = util::SafeLoadAs<uint64_t>(p + n - 8);
 y = util::SafeLoadAs<uint64_t>(p);
 hx = ScalarHelper<uint64_t, AlgNum>::ComputeHash(x);
 hy = ScalarHelper<uint64_t, AlgNum ^ 1>::ComputeHash(y);
 return n ^ hx ^ hy;
Expand Down

16 changes: 16 additions & 0 deletions
cpp/src/arrow/util/ubsan.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -49,5 +49,21 @@ inline T* MakeNonNull(T* maybe_null) {
 return reinterpret_cast<T*>(&internal::non_null_filler);
}

template <typename T>
@fsaintjacquesfsaintjacquesJul 2, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no C++ guru, but can't you make a single method for this by templating the input type too, e.g.
template <typename T, typename I = uint8_t>
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoad(
const I* unaligned) {
typename std::remove_const<T>::type ret;
std::memcpy(&ret, unaligned, sizeof(T));
return ret;
}

@emkornfieldemkornfieldJul 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct, I think I like how the methods are now though since it is more explicit when casting and a pass-through when not. One method could certainly delegate to the other.
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoadAs(
 const uint8_t* unaligned) {
 typename std::remove_const<T>::type ret;
 std::memcpy(&ret, unaligned, sizeof(T));
 return ret;
}

template <typename T>
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoad(
 const T* unaligned) {
 typename std::remove_const<T>::type ret;
 std::memcpy(&ret, unaligned, sizeof(T));
 return ret;
}

} // namespace util
} // namespace arrow

20 changes: 10 additions & 10 deletions
cpp/src/parquet/arrow/reader.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -83,6 +83,7 @@ namespace arrow {

using ::arrow::BitUtil::FromBigEndian;
using ::arrow::internal::SafeLeftShift;
using ::arrow::util::SafeLoadAs;

template <typename ArrowType>
using ArrayType = typename ::arrow::TypeTraits<ArrowType>::ArrayType;
Expand DownExpand Up@@ -1212,38 +1213,37 @@ static uint64_t BytesToInteger(const uint8_t* bytes, int32_t start, int32_t stop
 case 1:
 return bytes[start];
 case 2:
 return FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start));
 return FromBigEndian(SafeLoadAs<uint16_t>(bytes + start));
 case 3: {
 const uint64_t first_two_bytes =
 FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start));
 const uint64_t first_two_bytes = FromBigEndian(SafeLoadAs<uint16_t>(bytes + start));
 const uint64_t last_byte = bytes[stop - 1];
 return first_two_bytes << 8 | last_byte;
 }
 case 4:
 return FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 return FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 case 5: {
 const uint64_t first_four_bytes =
 FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 const uint64_t last_byte = bytes[stop - 1];
 return first_four_bytes << 8 | last_byte;
 }
 case 6: {
 const uint64_t first_four_bytes =
 FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 const uint64_t last_two_bytes =
 FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start + 4));
 FromBigEndian(SafeLoadAs<uint16_t>(bytes + start + 4));
 return first_four_bytes << 16 | last_two_bytes;
 }
 case 7: {
 const uint64_t first_four_bytes =
 FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 const uint64_t second_two_bytes =
 FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start + 4));
 FromBigEndian(SafeLoadAs<uint16_t>(bytes + start + 4));
 const uint64_t last_byte = bytes[stop - 1];
 return first_four_bytes << 24 | second_two_bytes << 8 | last_byte;
 }
 case 8:
 return FromBigEndian(*reinterpret_cast<const uint64_t*>(bytes + start));
 return FromBigEndian(SafeLoadAs<uint64_t>(bytes + start));
 default: {
 DCHECK(false);
 return UINT64_MAX;
Expand Down

5 changes: 3 additions & 2 deletions
cpp/src/parquet/arrow/writer.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -211,8 +211,9 @@ inline void ArrowTimestampToImpalaTimestamp(const int64_t time, Int96* impala_ti
 (*impala_timestamp).value[2] = (uint32_t)julian_days;

 int64_t last_day_units = time % UnitPerDay;
 int64_t* impala_last_day_nanos = reinterpret_cast<int64_t*>(impala_timestamp);
 *impala_last_day_nanos = last_day_units * NanosecondsPerUnit;
 auto last_day_nanos = last_day_units * NanosecondsPerUnit;
 // Strage might be unaligned, so use mempcy instead of reinterpret_cast
@fsaintjacquesfsaintjacquesJul 2, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All even indexed Int96 in a vector will be unaligned (according to int64_t alignment).
@emkornfieldemkornfieldJul 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point I'll open up a follow-up PR, we must not hav good test data here or UBSan isn't foolproof, or somehow I didn't run this test properly
@emkornfieldemkornfieldJul 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait were you just commenting on my comment?
@fsaintjacquesfsaintjacquesJul 11, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was commenting on your comment on the "strange" part :)
@pitroupitrouJul 11, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you notice the typo? ("Strage")
 std::memcpy(impala_timestamp, &last_day_nanos, sizeof(int64_t));
}

constexpr int64_t kSecondsInNanos = INT64_C(1000000000);
Expand Down

3 changes: 2 additions & 1 deletion
cpp/src/parquet/column_reader.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -27,6 +27,7 @@
#include "arrow/util/compression.h"
#include "arrow/util/logging.h"
#include "arrow/util/rle-encoding.h"
#include "arrow/util/ubsan.h"

#include "parquet/column_page.h"
#include "parquet/encoding.h"
Expand All@@ -50,7 +51,7 @@ int LevelDecoder::SetData(Encoding::type encoding, int16_t max_level,
 bit_width_ = BitUtil::Log2(max_level + 1);
 switch (encoding) {
 case Encoding::RLE: {
 num_bytes = *reinterpret_cast<const int32_t*>(data);
 num_bytes = arrow::util::SafeLoadAs<int32_t>(data);
 const uint8_t* decoder_data = data + sizeof(int32_t);
 if (!rle_decoder_) {
 rle_decoder_.reset(
Expand Down

11 changes: 6 additions & 5 deletions
cpp/src/parquet/encoding.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -29,6 +29,7 @@
#include "arrow/util/logging.h"
#include "arrow/util/rle-encoding.h"
#include "arrow/util/string_view.h"
#include "arrow/util/ubsan.h"

#include "parquet/exception.h"
#include "parquet/platform.h"
Expand DownExpand Up@@ -609,7 +610,7 @@ inline int DecodePlain<ByteArray>(const uint8_t* data, int64_t data_size, int nu
 int bytes_decoded = 0;
 int increment;
 for (int i = 0; i < num_values; ++i) {
 uint32_t len = out[i].len = *reinterpret_cast<const uint32_t*>(data);
 uint32_t len = out[i].len = arrow::util::SafeLoadAs<uint32_t>(data);
 increment = static_cast<int>(sizeof(uint32_t) + len);
 if (data_size < increment) ParquetException::EofException();
 out[i].ptr = data + sizeof(uint32_t);
Expand DownExpand Up@@ -719,7 +720,7 @@ class PlainByteArrayDecoder : public PlainDecoder<ByteArrayType>,
 int bytes_decoded = 0;
 while (i < num_values) {
 if (bit_reader.IsSet()) {
 uint32_t len = *reinterpret_cast<const uint32_t*>(data);
 uint32_t len = arrow::util::SafeLoadAs<uint32_t>(data);
 increment = static_cast<int>(sizeof(uint32_t) + len);
 if (data_size < increment) {
 ParquetException::EofException();
Expand DownExpand Up@@ -752,7 +753,7 @@ class PlainByteArrayDecoder : public PlainDecoder<ByteArrayType>,
 int bytes_decoded = 0;

 while (i < num_values) {
 uint32_t len = *reinterpret_cast<const uint32_t*>(data);
 uint32_t len = arrow::util::SafeLoadAs<uint32_t>(data);
 int increment = static_cast<int>(sizeof(uint32_t) + len);
 if (data_size < increment) ParquetException::EofException();
 builder->Append(data + sizeof(uint32_t), len);
Expand DownExpand Up@@ -1103,7 +1104,7 @@ class DeltaLengthByteArrayDecoder : public DecoderImpl,
 virtual void SetData(int num_values, const uint8_t* data, int len) {
 num_values_ = num_values;
 if (len == 0) return;
 int total_lengths_len = *reinterpret_cast<const int*>(data);
 int total_lengths_len = arrow::util::SafeLoadAs<int32_t>(data);
 data += 4;
 this->len_decoder_.SetData(num_values, data, total_lengths_len);
 data_ = data + total_lengths_len;
Expand DownExpand Up@@ -1145,7 +1146,7 @@ class DeltaByteArrayDecoder : public DecoderImpl,
 virtual void SetData(int num_values, const uint8_t* data, int len) {
 num_values_ = num_values;
 if (len == 0) return;
 int prefix_len_length = *reinterpret_cast<const int*>(data);
 int prefix_len_length = arrow::util::SafeLoadAs<int32_t>(data);
 data += 4;
 len -= 4;
 prefix_len_decoder_.SetData(num_values, data, prefix_len_length);
Expand Down

3 changes: 2 additions & 1 deletion
cpp/src/parquet/file_reader.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -28,6 +28,7 @@
#include "arrow/io/file.h"
#include "arrow/status.h"
#include "arrow/util/logging.h"
#include "arrow/util/ubsan.h"

#include "parquet/column_reader.h"
#include "parquet/column_scanner.h"
Expand DownExpand Up@@ -179,7 +180,7 @@ class SerializedFile : public ParquetFileReader::Contents {
 throw ParquetException("Invalid parquet file. Corrupt footer.");
 }

 uint32_t metadata_len = *reinterpret_cast<const uint32_t*>(
 uint32_t metadata_len = arrow::util::SafeLoadAs<uint32_t>(
 reinterpret_cast<const uint8_t*>(footer_buffer->data()) + footer_read_size -
 kFooterSize);
 int64_t metadata_start = file_size - kFooterSize - metadata_len;
Expand Down

4 changes: 3 additions & 1 deletion
cpp/src/plasma/common.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -19,6 +19,8 @@

#include <limits>

#include "arrow/util/ubsan.h"

#include "plasma/plasma_generated.h"

namespace fb = plasma::flatbuf;
Expand DownExpand Up@@ -64,7 +66,7 @@ uint64_t MurmurHash64A(const void* key, int len, unsigned int seed) {
 const uint64_t* end = data + (len / 8);

 while (data != end) {
 uint64_t k = *data++;
 uint64_t k = arrow::util::SafeLoad(data++);

 k *= m;
 k ^= k >> r;
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
ARROW-5380: [C++] Fix memory alignment UBSan errors.#4757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Closed

emkornfield
wants to merge
2
commits into

from

 Closed


ARROW-5380: [C++] Fix memory alignment UBSan errors.
#4757
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


2 commits


Select commit
Hold shift + click to select a range
db49fbb

Ubsan excluding flatbuffers


emkornfield Jun 30, 2019
5528584

remove TODO


emkornfield Jul 1, 2019

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


3,409 changes: 1,969 additions & 1,440 deletions
cpp/src/arrow/util/bpacking.h

View file


Open in desktop


Large diffs are not rendered by default.


9 changes: 4 additions & 5 deletions
cpp/src/arrow/util/hashing.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -149,9 +149,8 @@ hash_t ComputeStringHash(const void* data, int64_t length) {
 // the results
 uint32_t x, y;
 hash_t hx, hy;
 // XXX those are unaligned accesses. Should we have a facility for that?
 x = *reinterpret_cast<const uint32_t*>(p + n - 4);
 y = *reinterpret_cast<const uint32_t*>(p);
 x = util::SafeLoadAs<uint32_t>(p + n - 4);
 y = util::SafeLoadAs<uint32_t>(p);
 hx = ScalarHelper<uint32_t, AlgNum>::ComputeHash(x);
 hy = ScalarHelper<uint32_t, AlgNum ^ 1>::ComputeHash(y);
 return n ^ hx ^ hy;
Expand All@@ -160,8 +159,8 @@ hash_t ComputeStringHash(const void* data, int64_t length) {
 // Apply the same principle as above
 uint64_t x, y;
 hash_t hx, hy;
 x = *reinterpret_cast<const uint64_t*>(p + n - 8);
 y = *reinterpret_cast<const uint64_t*>(p);
 x = util::SafeLoadAs<uint64_t>(p + n - 8);
 y = util::SafeLoadAs<uint64_t>(p);
 hx = ScalarHelper<uint64_t, AlgNum>::ComputeHash(x);
 hy = ScalarHelper<uint64_t, AlgNum ^ 1>::ComputeHash(y);
 return n ^ hx ^ hy;
Expand Down

16 changes: 16 additions & 0 deletions
cpp/src/arrow/util/ubsan.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -49,5 +49,21 @@ inline T* MakeNonNull(T* maybe_null) {
 return reinterpret_cast<T*>(&internal::non_null_filler);
}

template <typename T>
@fsaintjacquesfsaintjacquesJul 2, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no C++ guru, but can't you make a single method for this by templating the input type too, e.g.
template <typename T, typename I = uint8_t>
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoad(
const I* unaligned) {
typename std::remove_const<T>::type ret;
std::memcpy(&ret, unaligned, sizeof(T));
return ret;
}

@emkornfieldemkornfieldJul 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct, I think I like how the methods are now though since it is more explicit when casting and a pass-through when not. One method could certainly delegate to the other.
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoadAs(
 const uint8_t* unaligned) {
 typename std::remove_const<T>::type ret;
 std::memcpy(&ret, unaligned, sizeof(T));
 return ret;
}

template <typename T>
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoad(
 const T* unaligned) {
 typename std::remove_const<T>::type ret;
 std::memcpy(&ret, unaligned, sizeof(T));
 return ret;
}

} // namespace util
} // namespace arrow

20 changes: 10 additions & 10 deletions
cpp/src/parquet/arrow/reader.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -83,6 +83,7 @@ namespace arrow {

using ::arrow::BitUtil::FromBigEndian;
using ::arrow::internal::SafeLeftShift;
using ::arrow::util::SafeLoadAs;

template <typename ArrowType>
using ArrayType = typename ::arrow::TypeTraits<ArrowType>::ArrayType;
Expand DownExpand Up@@ -1212,38 +1213,37 @@ static uint64_t BytesToInteger(const uint8_t* bytes, int32_t start, int32_t stop
 case 1:
 return bytes[start];
 case 2:
 return FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start));
 return FromBigEndian(SafeLoadAs<uint16_t>(bytes + start));
 case 3: {
 const uint64_t first_two_bytes =
 FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start));
 const uint64_t first_two_bytes = FromBigEndian(SafeLoadAs<uint16_t>(bytes + start));
 const uint64_t last_byte = bytes[stop - 1];
 return first_two_bytes << 8 | last_byte;
 }
 case 4:
 return FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 return FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 case 5: {
 const uint64_t first_four_bytes =
 FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 const uint64_t last_byte = bytes[stop - 1];
 return first_four_bytes << 8 | last_byte;
 }
 case 6: {
 const uint64_t first_four_bytes =
 FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 const uint64_t last_two_bytes =
 FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start + 4));
 FromBigEndian(SafeLoadAs<uint16_t>(bytes + start + 4));
 return first_four_bytes << 16 | last_two_bytes;
 }
 case 7: {
 const uint64_t first_four_bytes =
 FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 const uint64_t second_two_bytes =
 FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start + 4));
 FromBigEndian(SafeLoadAs<uint16_t>(bytes + start + 4));
 const uint64_t last_byte = bytes[stop - 1];
 return first_four_bytes << 24 | second_two_bytes << 8 | last_byte;
 }
 case 8:
 return FromBigEndian(*reinterpret_cast<const uint64_t*>(bytes + start));
 return FromBigEndian(SafeLoadAs<uint64_t>(bytes + start));
 default: {
 DCHECK(false);
 return UINT64_MAX;
Expand Down

5 changes: 3 additions & 2 deletions
cpp/src/parquet/arrow/writer.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -211,8 +211,9 @@ inline void ArrowTimestampToImpalaTimestamp(const int64_t time, Int96* impala_ti
 (*impala_timestamp).value[2] = (uint32_t)julian_days;

 int64_t last_day_units = time % UnitPerDay;
 int64_t* impala_last_day_nanos = reinterpret_cast<int64_t*>(impala_timestamp);
 *impala_last_day_nanos = last_day_units * NanosecondsPerUnit;
 auto last_day_nanos = last_day_units * NanosecondsPerUnit;
 // Strage might be unaligned, so use mempcy instead of reinterpret_cast
@fsaintjacquesfsaintjacquesJul 2, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All even indexed Int96 in a vector will be unaligned (according to int64_t alignment).
@emkornfieldemkornfieldJul 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point I'll open up a follow-up PR, we must not hav good test data here or UBSan isn't foolproof, or somehow I didn't run this test properly
@emkornfieldemkornfieldJul 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait were you just commenting on my comment?
@fsaintjacquesfsaintjacquesJul 11, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was commenting on your comment on the "strange" part :)
@pitroupitrouJul 11, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you notice the typo? ("Strage")
 std::memcpy(impala_timestamp, &last_day_nanos, sizeof(int64_t));
}

constexpr int64_t kSecondsInNanos = INT64_C(1000000000);
Expand Down

3 changes: 2 additions & 1 deletion
cpp/src/parquet/column_reader.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -27,6 +27,7 @@
#include "arrow/util/compression.h"
#include "arrow/util/logging.h"
#include "arrow/util/rle-encoding.h"
#include "arrow/util/ubsan.h"

#include "parquet/column_page.h"
#include "parquet/encoding.h"
Expand All@@ -50,7 +51,7 @@ int LevelDecoder::SetData(Encoding::type encoding, int16_t max_level,
 bit_width_ = BitUtil::Log2(max_level + 1);
 switch (encoding) {
 case Encoding::RLE: {
 num_bytes = *reinterpret_cast<const int32_t*>(data);
 num_bytes = arrow::util::SafeLoadAs<int32_t>(data);
 const uint8_t* decoder_data = data + sizeof(int32_t);
 if (!rle_decoder_) {
 rle_decoder_.reset(
Expand Down

11 changes: 6 additions & 5 deletions
cpp/src/parquet/encoding.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -29,6 +29,7 @@
#include "arrow/util/logging.h"
#include "arrow/util/rle-encoding.h"
#include "arrow/util/string_view.h"
#include "arrow/util/ubsan.h"

#include "parquet/exception.h"
#include "parquet/platform.h"
Expand DownExpand Up@@ -609,7 +610,7 @@ inline int DecodePlain<ByteArray>(const uint8_t* data, int64_t data_size, int nu
 int bytes_decoded = 0;
 int increment;
 for (int i = 0; i < num_values; ++i) {
 uint32_t len = out[i].len = *reinterpret_cast<const uint32_t*>(data);
 uint32_t len = out[i].len = arrow::util::SafeLoadAs<uint32_t>(data);
 increment = static_cast<int>(sizeof(uint32_t) + len);
 if (data_size < increment) ParquetException::EofException();
 out[i].ptr = data + sizeof(uint32_t);
Expand DownExpand Up@@ -719,7 +720,7 @@ class PlainByteArrayDecoder : public PlainDecoder<ByteArrayType>,
 int bytes_decoded = 0;
 while (i < num_values) {
 if (bit_reader.IsSet()) {
 uint32_t len = *reinterpret_cast<const uint32_t*>(data);
 uint32_t len = arrow::util::SafeLoadAs<uint32_t>(data);
 increment = static_cast<int>(sizeof(uint32_t) + len);
 if (data_size < increment) {
 ParquetException::EofException();
Expand DownExpand Up@@ -752,7 +753,7 @@ class PlainByteArrayDecoder : public PlainDecoder<ByteArrayType>,
 int bytes_decoded = 0;

 while (i < num_values) {
 uint32_t len = *reinterpret_cast<const uint32_t*>(data);
 uint32_t len = arrow::util::SafeLoadAs<uint32_t>(data);
 int increment = static_cast<int>(sizeof(uint32_t) + len);
 if (data_size < increment) ParquetException::EofException();
 builder->Append(data + sizeof(uint32_t), len);
Expand DownExpand Up@@ -1103,7 +1104,7 @@ class DeltaLengthByteArrayDecoder : public DecoderImpl,
 virtual void SetData(int num_values, const uint8_t* data, int len) {
 num_values_ = num_values;
 if (len == 0) return;
 int total_lengths_len = *reinterpret_cast<const int*>(data);
 int total_lengths_len = arrow::util::SafeLoadAs<int32_t>(data);
 data += 4;
 this->len_decoder_.SetData(num_values, data, total_lengths_len);
 data_ = data + total_lengths_len;
Expand DownExpand Up@@ -1145,7 +1146,7 @@ class DeltaByteArrayDecoder : public DecoderImpl,
 virtual void SetData(int num_values, const uint8_t* data, int len) {
 num_values_ = num_values;
 if (len == 0) return;
 int prefix_len_length = *reinterpret_cast<const int*>(data);
 int prefix_len_length = arrow::util::SafeLoadAs<int32_t>(data);
 data += 4;
 len -= 4;
 prefix_len_decoder_.SetData(num_values, data, prefix_len_length);
Expand Down

3 changes: 2 additions & 1 deletion
cpp/src/parquet/file_reader.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -28,6 +28,7 @@
#include "arrow/io/file.h"
#include "arrow/status.h"
#include "arrow/util/logging.h"
#include "arrow/util/ubsan.h"

#include "parquet/column_reader.h"
#include "parquet/column_scanner.h"
Expand DownExpand Up@@ -179,7 +180,7 @@ class SerializedFile : public ParquetFileReader::Contents {
 throw ParquetException("Invalid parquet file. Corrupt footer.");
 }

 uint32_t metadata_len = *reinterpret_cast<const uint32_t*>(
 uint32_t metadata_len = arrow::util::SafeLoadAs<uint32_t>(
 reinterpret_cast<const uint8_t*>(footer_buffer->data()) + footer_read_size -
 kFooterSize);
 int64_t metadata_start = file_size - kFooterSize - metadata_len;
Expand Down

4 changes: 3 additions & 1 deletion
cpp/src/plasma/common.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -19,6 +19,8 @@

#include <limits>

#include "arrow/util/ubsan.h"

#include "plasma/plasma_generated.h"

namespace fb = plasma::flatbuf;
Expand DownExpand Up@@ -64,7 +66,7 @@ uint64_t MurmurHash64A(const void* key, int len, unsigned int seed) {
 const uint64_t* end = data + (len / 8);

 while (data != end) {
 uint64_t k = *data++;
 uint64_t k = arrow::util::SafeLoad(data++);

 k *= m;
 k ^= k >> r;
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-5380: [C++] Fix memory alignment UBSan errors.#4757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Closed

emkornfield
wants to merge
2
commits into

from

 Closed


ARROW-5380: [C++] Fix memory alignment UBSan errors.
#4757
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


2 commits


Select commit
Hold shift + click to select a range
db49fbb

Ubsan excluding flatbuffers


emkornfield Jun 30, 2019
5528584

remove TODO


emkornfield Jul 1, 2019

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


3,409 changes: 1,969 additions & 1,440 deletions
cpp/src/arrow/util/bpacking.h

View file


Open in desktop


Large diffs are not rendered by default.


9 changes: 4 additions & 5 deletions
cpp/src/arrow/util/hashing.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -149,9 +149,8 @@ hash_t ComputeStringHash(const void* data, int64_t length) {
 // the results
 uint32_t x, y;
 hash_t hx, hy;
 // XXX those are unaligned accesses. Should we have a facility for that?
 x = *reinterpret_cast<const uint32_t*>(p + n - 4);
 y = *reinterpret_cast<const uint32_t*>(p);
 x = util::SafeLoadAs<uint32_t>(p + n - 4);
 y = util::SafeLoadAs<uint32_t>(p);
 hx = ScalarHelper<uint32_t, AlgNum>::ComputeHash(x);
 hy = ScalarHelper<uint32_t, AlgNum ^ 1>::ComputeHash(y);
 return n ^ hx ^ hy;
Expand All@@ -160,8 +159,8 @@ hash_t ComputeStringHash(const void* data, int64_t length) {
 // Apply the same principle as above
 uint64_t x, y;
 hash_t hx, hy;
 x = *reinterpret_cast<const uint64_t*>(p + n - 8);
 y = *reinterpret_cast<const uint64_t*>(p);
 x = util::SafeLoadAs<uint64_t>(p + n - 8);
 y = util::SafeLoadAs<uint64_t>(p);
 hx = ScalarHelper<uint64_t, AlgNum>::ComputeHash(x);
 hy = ScalarHelper<uint64_t, AlgNum ^ 1>::ComputeHash(y);
 return n ^ hx ^ hy;
Expand Down

16 changes: 16 additions & 0 deletions
cpp/src/arrow/util/ubsan.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -49,5 +49,21 @@ inline T* MakeNonNull(T* maybe_null) {
 return reinterpret_cast<T*>(&internal::non_null_filler);
}

template <typename T>
@fsaintjacquesfsaintjacquesJul 2, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no C++ guru, but can't you make a single method for this by templating the input type too, e.g.
template <typename T, typename I = uint8_t>
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoad(
const I* unaligned) {
typename std::remove_const<T>::type ret;
std::memcpy(&ret, unaligned, sizeof(T));
return ret;
}

@emkornfieldemkornfieldJul 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct, I think I like how the methods are now though since it is more explicit when casting and a pass-through when not. One method could certainly delegate to the other.
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoadAs(
 const uint8_t* unaligned) {
 typename std::remove_const<T>::type ret;
 std::memcpy(&ret, unaligned, sizeof(T));
 return ret;
}

template <typename T>
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoad(
 const T* unaligned) {
 typename std::remove_const<T>::type ret;
 std::memcpy(&ret, unaligned, sizeof(T));
 return ret;
}

} // namespace util
} // namespace arrow

20 changes: 10 additions & 10 deletions
cpp/src/parquet/arrow/reader.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -83,6 +83,7 @@ namespace arrow {

using ::arrow::BitUtil::FromBigEndian;
using ::arrow::internal::SafeLeftShift;
using ::arrow::util::SafeLoadAs;

template <typename ArrowType>
using ArrayType = typename ::arrow::TypeTraits<ArrowType>::ArrayType;
Expand DownExpand Up@@ -1212,38 +1213,37 @@ static uint64_t BytesToInteger(const uint8_t* bytes, int32_t start, int32_t stop
 case 1:
 return bytes[start];
 case 2:
 return FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start));
 return FromBigEndian(SafeLoadAs<uint16_t>(bytes + start));
 case 3: {
 const uint64_t first_two_bytes =
 FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start));
 const uint64_t first_two_bytes = FromBigEndian(SafeLoadAs<uint16_t>(bytes + start));
 const uint64_t last_byte = bytes[stop - 1];
 return first_two_bytes << 8 | last_byte;
 }
 case 4:
 return FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 return FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 case 5: {
 const uint64_t first_four_bytes =
 FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 const uint64_t last_byte = bytes[stop - 1];
 return first_four_bytes << 8 | last_byte;
 }
 case 6: {
 const uint64_t first_four_bytes =
 FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 const uint64_t last_two_bytes =
 FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start + 4));
 FromBigEndian(SafeLoadAs<uint16_t>(bytes + start + 4));
 return first_four_bytes << 16 | last_two_bytes;
 }
 case 7: {
 const uint64_t first_four_bytes =
 FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 const uint64_t second_two_bytes =
 FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start + 4));
 FromBigEndian(SafeLoadAs<uint16_t>(bytes + start + 4));
 const uint64_t last_byte = bytes[stop - 1];
 return first_four_bytes << 24 | second_two_bytes << 8 | last_byte;
 }
 case 8:
 return FromBigEndian(*reinterpret_cast<const uint64_t*>(bytes + start));
 return FromBigEndian(SafeLoadAs<uint64_t>(bytes + start));
 default: {
 DCHECK(false);
 return UINT64_MAX;
Expand Down

5 changes: 3 additions & 2 deletions
cpp/src/parquet/arrow/writer.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -211,8 +211,9 @@ inline void ArrowTimestampToImpalaTimestamp(const int64_t time, Int96* impala_ti
 (*impala_timestamp).value[2] = (uint32_t)julian_days;

 int64_t last_day_units = time % UnitPerDay;
 int64_t* impala_last_day_nanos = reinterpret_cast<int64_t*>(impala_timestamp);
 *impala_last_day_nanos = last_day_units * NanosecondsPerUnit;
 auto last_day_nanos = last_day_units * NanosecondsPerUnit;
 // Strage might be unaligned, so use mempcy instead of reinterpret_cast
@fsaintjacquesfsaintjacquesJul 2, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All even indexed Int96 in a vector will be unaligned (according to int64_t alignment).
@emkornfieldemkornfieldJul 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point I'll open up a follow-up PR, we must not hav good test data here or UBSan isn't foolproof, or somehow I didn't run this test properly
@emkornfieldemkornfieldJul 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait were you just commenting on my comment?
@fsaintjacquesfsaintjacquesJul 11, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was commenting on your comment on the "strange" part :)
@pitroupitrouJul 11, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you notice the typo? ("Strage")
 std::memcpy(impala_timestamp, &last_day_nanos, sizeof(int64_t));
}

constexpr int64_t kSecondsInNanos = INT64_C(1000000000);
Expand Down

3 changes: 2 additions & 1 deletion
cpp/src/parquet/column_reader.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -27,6 +27,7 @@
#include "arrow/util/compression.h"
#include "arrow/util/logging.h"
#include "arrow/util/rle-encoding.h"
#include "arrow/util/ubsan.h"

#include "parquet/column_page.h"
#include "parquet/encoding.h"
Expand All@@ -50,7 +51,7 @@ int LevelDecoder::SetData(Encoding::type encoding, int16_t max_level,
 bit_width_ = BitUtil::Log2(max_level + 1);
 switch (encoding) {
 case Encoding::RLE: {
 num_bytes = *reinterpret_cast<const int32_t*>(data);
 num_bytes = arrow::util::SafeLoadAs<int32_t>(data);
 const uint8_t* decoder_data = data + sizeof(int32_t);
 if (!rle_decoder_) {
 rle_decoder_.reset(
Expand Down

11 changes: 6 additions & 5 deletions
cpp/src/parquet/encoding.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -29,6 +29,7 @@
#include "arrow/util/logging.h"
#include "arrow/util/rle-encoding.h"
#include "arrow/util/string_view.h"
#include "arrow/util/ubsan.h"

#include "parquet/exception.h"
#include "parquet/platform.h"
Expand DownExpand Up@@ -609,7 +610,7 @@ inline int DecodePlain<ByteArray>(const uint8_t* data, int64_t data_size, int nu
 int bytes_decoded = 0;
 int increment;
 for (int i = 0; i < num_values; ++i) {
 uint32_t len = out[i].len = *reinterpret_cast<const uint32_t*>(data);
 uint32_t len = out[i].len = arrow::util::SafeLoadAs<uint32_t>(data);
 increment = static_cast<int>(sizeof(uint32_t) + len);
 if (data_size < increment) ParquetException::EofException();
 out[i].ptr = data + sizeof(uint32_t);
Expand DownExpand Up@@ -719,7 +720,7 @@ class PlainByteArrayDecoder : public PlainDecoder<ByteArrayType>,
 int bytes_decoded = 0;
 while (i < num_values) {
 if (bit_reader.IsSet()) {
 uint32_t len = *reinterpret_cast<const uint32_t*>(data);
 uint32_t len = arrow::util::SafeLoadAs<uint32_t>(data);
 increment = static_cast<int>(sizeof(uint32_t) + len);
 if (data_size < increment) {
 ParquetException::EofException();
Expand DownExpand Up@@ -752,7 +753,7 @@ class PlainByteArrayDecoder : public PlainDecoder<ByteArrayType>,
 int bytes_decoded = 0;

 while (i < num_values) {
 uint32_t len = *reinterpret_cast<const uint32_t*>(data);
 uint32_t len = arrow::util::SafeLoadAs<uint32_t>(data);
 int increment = static_cast<int>(sizeof(uint32_t) + len);
 if (data_size < increment) ParquetException::EofException();
 builder->Append(data + sizeof(uint32_t), len);
Expand DownExpand Up@@ -1103,7 +1104,7 @@ class DeltaLengthByteArrayDecoder : public DecoderImpl,
 virtual void SetData(int num_values, const uint8_t* data, int len) {
 num_values_ = num_values;
 if (len == 0) return;
 int total_lengths_len = *reinterpret_cast<const int*>(data);
 int total_lengths_len = arrow::util::SafeLoadAs<int32_t>(data);
 data += 4;
 this->len_decoder_.SetData(num_values, data, total_lengths_len);
 data_ = data + total_lengths_len;
Expand DownExpand Up@@ -1145,7 +1146,7 @@ class DeltaByteArrayDecoder : public DecoderImpl,
 virtual void SetData(int num_values, const uint8_t* data, int len) {
 num_values_ = num_values;
 if (len == 0) return;
 int prefix_len_length = *reinterpret_cast<const int*>(data);
 int prefix_len_length = arrow::util::SafeLoadAs<int32_t>(data);
 data += 4;
 len -= 4;
 prefix_len_decoder_.SetData(num_values, data, prefix_len_length);
Expand Down

3 changes: 2 additions & 1 deletion
cpp/src/parquet/file_reader.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -28,6 +28,7 @@
#include "arrow/io/file.h"
#include "arrow/status.h"
#include "arrow/util/logging.h"
#include "arrow/util/ubsan.h"

#include "parquet/column_reader.h"
#include "parquet/column_scanner.h"
Expand DownExpand Up@@ -179,7 +180,7 @@ class SerializedFile : public ParquetFileReader::Contents {
 throw ParquetException("Invalid parquet file. Corrupt footer.");
 }

 uint32_t metadata_len = *reinterpret_cast<const uint32_t*>(
 uint32_t metadata_len = arrow::util::SafeLoadAs<uint32_t>(
 reinterpret_cast<const uint8_t*>(footer_buffer->data()) + footer_read_size -
 kFooterSize);
 int64_t metadata_start = file_size - kFooterSize - metadata_len;
Expand Down

4 changes: 3 additions & 1 deletion
cpp/src/plasma/common.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -19,6 +19,8 @@

#include <limits>

#include "arrow/util/ubsan.h"

#include "plasma/plasma_generated.h"

namespace fb = plasma::flatbuf;
Expand DownExpand Up@@ -64,7 +66,7 @@ uint64_t MurmurHash64A(const void* key, int len, unsigned int seed) {
 const uint64_t* end = data + (len / 8);

 while (data != end) {
 uint64_t k = *data++;
 uint64_t k = arrow::util::SafeLoad(data++);

 k *= m;
 k ^= k >> r;
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-5380: [C++] Fix memory alignment UBSan errors.#4757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Closed

emkornfield
wants to merge
2
commits into

from

 Closed


ARROW-5380: [C++] Fix memory alignment UBSan errors.
#4757
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


2 commits


Select commit
Hold shift + click to select a range
db49fbb

Ubsan excluding flatbuffers


emkornfield Jun 30, 2019
5528584

remove TODO


emkornfield Jul 1, 2019

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


3,409 changes: 1,969 additions & 1,440 deletions
cpp/src/arrow/util/bpacking.h

View file


Open in desktop


Large diffs are not rendered by default.


9 changes: 4 additions & 5 deletions
cpp/src/arrow/util/hashing.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -149,9 +149,8 @@ hash_t ComputeStringHash(const void* data, int64_t length) {
 // the results
 uint32_t x, y;
 hash_t hx, hy;
 // XXX those are unaligned accesses. Should we have a facility for that?
 x = *reinterpret_cast<const uint32_t*>(p + n - 4);
 y = *reinterpret_cast<const uint32_t*>(p);
 x = util::SafeLoadAs<uint32_t>(p + n - 4);
 y = util::SafeLoadAs<uint32_t>(p);
 hx = ScalarHelper<uint32_t, AlgNum>::ComputeHash(x);
 hy = ScalarHelper<uint32_t, AlgNum ^ 1>::ComputeHash(y);
 return n ^ hx ^ hy;
Expand All@@ -160,8 +159,8 @@ hash_t ComputeStringHash(const void* data, int64_t length) {
 // Apply the same principle as above
 uint64_t x, y;
 hash_t hx, hy;
 x = *reinterpret_cast<const uint64_t*>(p + n - 8);
 y = *reinterpret_cast<const uint64_t*>(p);
 x = util::SafeLoadAs<uint64_t>(p + n - 8);
 y = util::SafeLoadAs<uint64_t>(p);
 hx = ScalarHelper<uint64_t, AlgNum>::ComputeHash(x);
 hy = ScalarHelper<uint64_t, AlgNum ^ 1>::ComputeHash(y);
 return n ^ hx ^ hy;
Expand Down

16 changes: 16 additions & 0 deletions
cpp/src/arrow/util/ubsan.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -49,5 +49,21 @@ inline T* MakeNonNull(T* maybe_null) {
 return reinterpret_cast<T*>(&internal::non_null_filler);
}

template <typename T>
@fsaintjacquesfsaintjacquesJul 2, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no C++ guru, but can't you make a single method for this by templating the input type too, e.g.
template <typename T, typename I = uint8_t>
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoad(
const I* unaligned) {
typename std::remove_const<T>::type ret;
std::memcpy(&ret, unaligned, sizeof(T));
return ret;
}

@emkornfieldemkornfieldJul 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct, I think I like how the methods are now though since it is more explicit when casting and a pass-through when not. One method could certainly delegate to the other.
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoadAs(
 const uint8_t* unaligned) {
 typename std::remove_const<T>::type ret;
 std::memcpy(&ret, unaligned, sizeof(T));
 return ret;
}

template <typename T>
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoad(
 const T* unaligned) {
 typename std::remove_const<T>::type ret;
 std::memcpy(&ret, unaligned, sizeof(T));
 return ret;
}

} // namespace util
} // namespace arrow

20 changes: 10 additions & 10 deletions
cpp/src/parquet/arrow/reader.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -83,6 +83,7 @@ namespace arrow {

using ::arrow::BitUtil::FromBigEndian;
using ::arrow::internal::SafeLeftShift;
using ::arrow::util::SafeLoadAs;

template <typename ArrowType>
using ArrayType = typename ::arrow::TypeTraits<ArrowType>::ArrayType;
Expand DownExpand Up@@ -1212,38 +1213,37 @@ static uint64_t BytesToInteger(const uint8_t* bytes, int32_t start, int32_t stop
 case 1:
 return bytes[start];
 case 2:
 return FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start));
 return FromBigEndian(SafeLoadAs<uint16_t>(bytes + start));
 case 3: {
 const uint64_t first_two_bytes =
 FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start));
 const uint64_t first_two_bytes = FromBigEndian(SafeLoadAs<uint16_t>(bytes + start));
 const uint64_t last_byte = bytes[stop - 1];
 return first_two_bytes << 8 | last_byte;
 }
 case 4:
 return FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 return FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 case 5: {
 const uint64_t first_four_bytes =
 FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 const uint64_t last_byte = bytes[stop - 1];
 return first_four_bytes << 8 | last_byte;
 }
 case 6: {
 const uint64_t first_four_bytes =
 FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 const uint64_t last_two_bytes =
 FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start + 4));
 FromBigEndian(SafeLoadAs<uint16_t>(bytes + start + 4));
 return first_four_bytes << 16 | last_two_bytes;
 }
 case 7: {
 const uint64_t first_four_bytes =
 FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 const uint64_t second_two_bytes =
 FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start + 4));
 FromBigEndian(SafeLoadAs<uint16_t>(bytes + start + 4));
 const uint64_t last_byte = bytes[stop - 1];
 return first_four_bytes << 24 | second_two_bytes << 8 | last_byte;
 }
 case 8:
 return FromBigEndian(*reinterpret_cast<const uint64_t*>(bytes + start));
 return FromBigEndian(SafeLoadAs<uint64_t>(bytes + start));
 default: {
 DCHECK(false);
 return UINT64_MAX;
Expand Down

5 changes: 3 additions & 2 deletions
cpp/src/parquet/arrow/writer.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -211,8 +211,9 @@ inline void ArrowTimestampToImpalaTimestamp(const int64_t time, Int96* impala_ti
 (*impala_timestamp).value[2] = (uint32_t)julian_days;

 int64_t last_day_units = time % UnitPerDay;
 int64_t* impala_last_day_nanos = reinterpret_cast<int64_t*>(impala_timestamp);
 *impala_last_day_nanos = last_day_units * NanosecondsPerUnit;
 auto last_day_nanos = last_day_units * NanosecondsPerUnit;
 // Strage might be unaligned, so use mempcy instead of reinterpret_cast
@fsaintjacquesfsaintjacquesJul 2, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All even indexed Int96 in a vector will be unaligned (according to int64_t alignment).
@emkornfieldemkornfieldJul 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point I'll open up a follow-up PR, we must not hav good test data here or UBSan isn't foolproof, or somehow I didn't run this test properly
@emkornfieldemkornfieldJul 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait were you just commenting on my comment?
@fsaintjacquesfsaintjacquesJul 11, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was commenting on your comment on the "strange" part :)
@pitroupitrouJul 11, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you notice the typo? ("Strage")
 std::memcpy(impala_timestamp, &last_day_nanos, sizeof(int64_t));
}

constexpr int64_t kSecondsInNanos = INT64_C(1000000000);
Expand Down

3 changes: 2 additions & 1 deletion
cpp/src/parquet/column_reader.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -27,6 +27,7 @@
#include "arrow/util/compression.h"
#include "arrow/util/logging.h"
#include "arrow/util/rle-encoding.h"
#include "arrow/util/ubsan.h"

#include "parquet/column_page.h"
#include "parquet/encoding.h"
Expand All@@ -50,7 +51,7 @@ int LevelDecoder::SetData(Encoding::type encoding, int16_t max_level,
 bit_width_ = BitUtil::Log2(max_level + 1);
 switch (encoding) {
 case Encoding::RLE: {
 num_bytes = *reinterpret_cast<const int32_t*>(data);
 num_bytes = arrow::util::SafeLoadAs<int32_t>(data);
 const uint8_t* decoder_data = data + sizeof(int32_t);
 if (!rle_decoder_) {
 rle_decoder_.reset(
Expand Down

11 changes: 6 additions & 5 deletions
cpp/src/parquet/encoding.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -29,6 +29,7 @@
#include "arrow/util/logging.h"
#include "arrow/util/rle-encoding.h"
#include "arrow/util/string_view.h"
#include "arrow/util/ubsan.h"

#include "parquet/exception.h"
#include "parquet/platform.h"
Expand DownExpand Up@@ -609,7 +610,7 @@ inline int DecodePlain<ByteArray>(const uint8_t* data, int64_t data_size, int nu
 int bytes_decoded = 0;
 int increment;
 for (int i = 0; i < num_values; ++i) {
 uint32_t len = out[i].len = *reinterpret_cast<const uint32_t*>(data);
 uint32_t len = out[i].len = arrow::util::SafeLoadAs<uint32_t>(data);
 increment = static_cast<int>(sizeof(uint32_t) + len);
 if (data_size < increment) ParquetException::EofException();
 out[i].ptr = data + sizeof(uint32_t);
Expand DownExpand Up@@ -719,7 +720,7 @@ class PlainByteArrayDecoder : public PlainDecoder<ByteArrayType>,
 int bytes_decoded = 0;
 while (i < num_values) {
 if (bit_reader.IsSet()) {
 uint32_t len = *reinterpret_cast<const uint32_t*>(data);
 uint32_t len = arrow::util::SafeLoadAs<uint32_t>(data);
 increment = static_cast<int>(sizeof(uint32_t) + len);
 if (data_size < increment) {
 ParquetException::EofException();
Expand DownExpand Up@@ -752,7 +753,7 @@ class PlainByteArrayDecoder : public PlainDecoder<ByteArrayType>,
 int bytes_decoded = 0;

 while (i < num_values) {
 uint32_t len = *reinterpret_cast<const uint32_t*>(data);
 uint32_t len = arrow::util::SafeLoadAs<uint32_t>(data);
 int increment = static_cast<int>(sizeof(uint32_t) + len);
 if (data_size < increment) ParquetException::EofException();
 builder->Append(data + sizeof(uint32_t), len);
Expand DownExpand Up@@ -1103,7 +1104,7 @@ class DeltaLengthByteArrayDecoder : public DecoderImpl,
 virtual void SetData(int num_values, const uint8_t* data, int len) {
 num_values_ = num_values;
 if (len == 0) return;
 int total_lengths_len = *reinterpret_cast<const int*>(data);
 int total_lengths_len = arrow::util::SafeLoadAs<int32_t>(data);
 data += 4;
 this->len_decoder_.SetData(num_values, data, total_lengths_len);
 data_ = data + total_lengths_len;
Expand DownExpand Up@@ -1145,7 +1146,7 @@ class DeltaByteArrayDecoder : public DecoderImpl,
 virtual void SetData(int num_values, const uint8_t* data, int len) {
 num_values_ = num_values;
 if (len == 0) return;
 int prefix_len_length = *reinterpret_cast<const int*>(data);
 int prefix_len_length = arrow::util::SafeLoadAs<int32_t>(data);
 data += 4;
 len -= 4;
 prefix_len_decoder_.SetData(num_values, data, prefix_len_length);
Expand Down

3 changes: 2 additions & 1 deletion
cpp/src/parquet/file_reader.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -28,6 +28,7 @@
#include "arrow/io/file.h"
#include "arrow/status.h"
#include "arrow/util/logging.h"
#include "arrow/util/ubsan.h"

#include "parquet/column_reader.h"
#include "parquet/column_scanner.h"
Expand DownExpand Up@@ -179,7 +180,7 @@ class SerializedFile : public ParquetFileReader::Contents {
 throw ParquetException("Invalid parquet file. Corrupt footer.");
 }

 uint32_t metadata_len = *reinterpret_cast<const uint32_t*>(
 uint32_t metadata_len = arrow::util::SafeLoadAs<uint32_t>(
 reinterpret_cast<const uint8_t*>(footer_buffer->data()) + footer_read_size -
 kFooterSize);
 int64_t metadata_start = file_size - kFooterSize - metadata_len;
Expand Down

4 changes: 3 additions & 1 deletion
cpp/src/plasma/common.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -19,6 +19,8 @@

#include <limits>

#include "arrow/util/ubsan.h"

#include "plasma/plasma_generated.h"

namespace fb = plasma::flatbuf;
Expand DownExpand Up@@ -64,7 +66,7 @@ uint64_t MurmurHash64A(const void* key, int len, unsigned int seed) {
 const uint64_t* end = data + (len / 8);

 while (data != end) {
 uint64_t k = *data++;
 uint64_t k = arrow::util::SafeLoad(data++);

 k *= m;
 k ^= k >> r;
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
ARROW-5380: [C++] Fix memory alignment UBSan errors.#4757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Closed

emkornfield
wants to merge
2
commits into

from

 Closed


ARROW-5380: [C++] Fix memory alignment UBSan errors.
#4757
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


2 commits


Select commit
Hold shift + click to select a range
db49fbb

Ubsan excluding flatbuffers


emkornfield Jun 30, 2019
5528584

remove TODO


emkornfield Jul 1, 2019

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


3,409 changes: 1,969 additions & 1,440 deletions
cpp/src/arrow/util/bpacking.h

View file


Open in desktop


Large diffs are not rendered by default.


9 changes: 4 additions & 5 deletions
cpp/src/arrow/util/hashing.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -149,9 +149,8 @@ hash_t ComputeStringHash(const void* data, int64_t length) {
 // the results
 uint32_t x, y;
 hash_t hx, hy;
 // XXX those are unaligned accesses. Should we have a facility for that?
 x = *reinterpret_cast<const uint32_t*>(p + n - 4);
 y = *reinterpret_cast<const uint32_t*>(p);
 x = util::SafeLoadAs<uint32_t>(p + n - 4);
 y = util::SafeLoadAs<uint32_t>(p);
 hx = ScalarHelper<uint32_t, AlgNum>::ComputeHash(x);
 hy = ScalarHelper<uint32_t, AlgNum ^ 1>::ComputeHash(y);
 return n ^ hx ^ hy;
Expand All@@ -160,8 +159,8 @@ hash_t ComputeStringHash(const void* data, int64_t length) {
 // Apply the same principle as above
 uint64_t x, y;
 hash_t hx, hy;
 x = *reinterpret_cast<const uint64_t*>(p + n - 8);
 y = *reinterpret_cast<const uint64_t*>(p);
 x = util::SafeLoadAs<uint64_t>(p + n - 8);
 y = util::SafeLoadAs<uint64_t>(p);
 hx = ScalarHelper<uint64_t, AlgNum>::ComputeHash(x);
 hy = ScalarHelper<uint64_t, AlgNum ^ 1>::ComputeHash(y);
 return n ^ hx ^ hy;
Expand Down

16 changes: 16 additions & 0 deletions
cpp/src/arrow/util/ubsan.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -49,5 +49,21 @@ inline T* MakeNonNull(T* maybe_null) {
 return reinterpret_cast<T*>(&internal::non_null_filler);
}

template <typename T>
@fsaintjacquesfsaintjacquesJul 2, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no C++ guru, but can't you make a single method for this by templating the input type too, e.g.
template <typename T, typename I = uint8_t>
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoad(
const I* unaligned) {
typename std::remove_const<T>::type ret;
std::memcpy(&ret, unaligned, sizeof(T));
return ret;
}

@emkornfieldemkornfieldJul 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct, I think I like how the methods are now though since it is more explicit when casting and a pass-through when not. One method could certainly delegate to the other.
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoadAs(
 const uint8_t* unaligned) {
 typename std::remove_const<T>::type ret;
 std::memcpy(&ret, unaligned, sizeof(T));
 return ret;
}

template <typename T>
inline typename std::enable_if<std::is_integral<T>::value, T>::type SafeLoad(
 const T* unaligned) {
 typename std::remove_const<T>::type ret;
 std::memcpy(&ret, unaligned, sizeof(T));
 return ret;
}

} // namespace util
} // namespace arrow

20 changes: 10 additions & 10 deletions
cpp/src/parquet/arrow/reader.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -83,6 +83,7 @@ namespace arrow {

using ::arrow::BitUtil::FromBigEndian;
using ::arrow::internal::SafeLeftShift;
using ::arrow::util::SafeLoadAs;

template <typename ArrowType>
using ArrayType = typename ::arrow::TypeTraits<ArrowType>::ArrayType;
Expand DownExpand Up@@ -1212,38 +1213,37 @@ static uint64_t BytesToInteger(const uint8_t* bytes, int32_t start, int32_t stop
 case 1:
 return bytes[start];
 case 2:
 return FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start));
 return FromBigEndian(SafeLoadAs<uint16_t>(bytes + start));
 case 3: {
 const uint64_t first_two_bytes =
 FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start));
 const uint64_t first_two_bytes = FromBigEndian(SafeLoadAs<uint16_t>(bytes + start));
 const uint64_t last_byte = bytes[stop - 1];
 return first_two_bytes << 8 | last_byte;
 }
 case 4:
 return FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 return FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 case 5: {
 const uint64_t first_four_bytes =
 FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 const uint64_t last_byte = bytes[stop - 1];
 return first_four_bytes << 8 | last_byte;
 }
 case 6: {
 const uint64_t first_four_bytes =
 FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 const uint64_t last_two_bytes =
 FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start + 4));
 FromBigEndian(SafeLoadAs<uint16_t>(bytes + start + 4));
 return first_four_bytes << 16 | last_two_bytes;
 }
 case 7: {
 const uint64_t first_four_bytes =
 FromBigEndian(*reinterpret_cast<const uint32_t*>(bytes + start));
 FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
 const uint64_t second_two_bytes =
 FromBigEndian(*reinterpret_cast<const uint16_t*>(bytes + start + 4));
 FromBigEndian(SafeLoadAs<uint16_t>(bytes + start + 4));
 const uint64_t last_byte = bytes[stop - 1];
 return first_four_bytes << 24 | second_two_bytes << 8 | last_byte;
 }
 case 8:
 return FromBigEndian(*reinterpret_cast<const uint64_t*>(bytes + start));
 return FromBigEndian(SafeLoadAs<uint64_t>(bytes + start));
 default: {
 DCHECK(false);
 return UINT64_MAX;
Expand Down

5 changes: 3 additions & 2 deletions
cpp/src/parquet/arrow/writer.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -211,8 +211,9 @@ inline void ArrowTimestampToImpalaTimestamp(const int64_t time, Int96* impala_ti
 (*impala_timestamp).value[2] = (uint32_t)julian_days;

 int64_t last_day_units = time % UnitPerDay;
 int64_t* impala_last_day_nanos = reinterpret_cast<int64_t*>(impala_timestamp);
 *impala_last_day_nanos = last_day_units * NanosecondsPerUnit;
 auto last_day_nanos = last_day_units * NanosecondsPerUnit;
 // Strage might be unaligned, so use mempcy instead of reinterpret_cast
@fsaintjacquesfsaintjacquesJul 2, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All even indexed Int96 in a vector will be unaligned (according to int64_t alignment).
@emkornfieldemkornfieldJul 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point I'll open up a follow-up PR, we must not hav good test data here or UBSan isn't foolproof, or somehow I didn't run this test properly
@emkornfieldemkornfieldJul 3, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait were you just commenting on my comment?
@fsaintjacquesfsaintjacquesJul 11, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was commenting on your comment on the "strange" part :)
@pitroupitrouJul 11, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you notice the typo? ("Strage")
 std::memcpy(impala_timestamp, &last_day_nanos, sizeof(int64_t));
}

constexpr int64_t kSecondsInNanos = INT64_C(1000000000);
Expand Down

3 changes: 2 additions & 1 deletion
cpp/src/parquet/column_reader.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -27,6 +27,7 @@
#include "arrow/util/compression.h"
#include "arrow/util/logging.h"
#include "arrow/util/rle-encoding.h"
#include "arrow/util/ubsan.h"

#include "parquet/column_page.h"
#include "parquet/encoding.h"
Expand All@@ -50,7 +51,7 @@ int LevelDecoder::SetData(Encoding::type encoding, int16_t max_level,
 bit_width_ = BitUtil::Log2(max_level + 1);
 switch (encoding) {
 case Encoding::RLE: {
 num_bytes = *reinterpret_cast<const int32_t*>(data);
 num_bytes = arrow::util::SafeLoadAs<int32_t>(data);
 const uint8_t* decoder_data = data + sizeof(int32_t);
 if (!rle_decoder_) {
 rle_decoder_.reset(
Expand Down

11 changes: 6 additions & 5 deletions
cpp/src/parquet/encoding.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -29,6 +29,7 @@
#include "arrow/util/logging.h"
#include "arrow/util/rle-encoding.h"
#include "arrow/util/string_view.h"
#include "arrow/util/ubsan.h"

#include "parquet/exception.h"
#include "parquet/platform.h"
Expand DownExpand Up@@ -609,7 +610,7 @@ inline int DecodePlain<ByteArray>(const uint8_t* data, int64_t data_size, int nu
 int bytes_decoded = 0;
 int increment;
 for (int i = 0; i < num_values; ++i) {
 uint32_t len = out[i].len = *reinterpret_cast<const uint32_t*>(data);
 uint32_t len = out[i].len = arrow::util::SafeLoadAs<uint32_t>(data);
 increment = static_cast<int>(sizeof(uint32_t) + len);
 if (data_size < increment) ParquetException::EofException();
 out[i].ptr = data + sizeof(uint32_t);
Expand DownExpand Up@@ -719,7 +720,7 @@ class PlainByteArrayDecoder : public PlainDecoder<ByteArrayType>,
 int bytes_decoded = 0;
 while (i < num_values) {
 if (bit_reader.IsSet()) {
 uint32_t len = *reinterpret_cast<const uint32_t*>(data);
 uint32_t len = arrow::util::SafeLoadAs<uint32_t>(data);
 increment = static_cast<int>(sizeof(uint32_t) + len);
 if (data_size < increment) {
 ParquetException::EofException();
Expand DownExpand Up@@ -752,7 +753,7 @@ class PlainByteArrayDecoder : public PlainDecoder<ByteArrayType>,
 int bytes_decoded = 0;

 while (i < num_values) {
 uint32_t len = *reinterpret_cast<const uint32_t*>(data);
 uint32_t len = arrow::util::SafeLoadAs<uint32_t>(data);
 int increment = static_cast<int>(sizeof(uint32_t) + len);
 if (data_size < increment) ParquetException::EofException();
 builder->Append(data + sizeof(uint32_t), len);
Expand DownExpand Up@@ -1103,7 +1104,7 @@ class DeltaLengthByteArrayDecoder : public DecoderImpl,
 virtual void SetData(int num_values, const uint8_t* data, int len) {
 num_values_ = num_values;
 if (len == 0) return;
 int total_lengths_len = *reinterpret_cast<const int*>(data);
 int total_lengths_len = arrow::util::SafeLoadAs<int32_t>(data);
 data += 4;
 this->len_decoder_.SetData(num_values, data, total_lengths_len);
 data_ = data + total_lengths_len;
Expand DownExpand Up@@ -1145,7 +1146,7 @@ class DeltaByteArrayDecoder : public DecoderImpl,
 virtual void SetData(int num_values, const uint8_t* data, int len) {
 num_values_ = num_values;
 if (len == 0) return;
 int prefix_len_length = *reinterpret_cast<const int*>(data);
 int prefix_len_length = arrow::util::SafeLoadAs<int32_t>(data);
 data += 4;
 len -= 4;
 prefix_len_decoder_.SetData(num_values, data, prefix_len_length);
Expand Down

3 changes: 2 additions & 1 deletion
cpp/src/parquet/file_reader.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -28,6 +28,7 @@
#include "arrow/io/file.h"
#include "arrow/status.h"
#include "arrow/util/logging.h"
#include "arrow/util/ubsan.h"

#include "parquet/column_reader.h"
#include "parquet/column_scanner.h"
Expand DownExpand Up@@ -179,7 +180,7 @@ class SerializedFile : public ParquetFileReader::Contents {
 throw ParquetException("Invalid parquet file. Corrupt footer.");
 }

 uint32_t metadata_len = *reinterpret_cast<const uint32_t*>(
 uint32_t metadata_len = arrow::util::SafeLoadAs<uint32_t>(
 reinterpret_cast<const uint8_t*>(footer_buffer->data()) + footer_read_size -
 kFooterSize);
 int64_t metadata_start = file_size - kFooterSize - metadata_len;
Expand Down

4 changes: 3 additions & 1 deletion
cpp/src/plasma/common.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -19,6 +19,8 @@

#include <limits>

#include "arrow/util/ubsan.h"

#include "plasma/plasma_generated.h"

namespace fb = plasma::flatbuf;
Expand DownExpand Up@@ -64,7 +66,7 @@ uint64_t MurmurHash64A(const void* key, int len, unsigned int seed) {
 const uint64_t* end = data + (len / 8);

 while (data != end) {
 uint64_t k = *data++;
 uint64_t k = arrow::util::SafeLoad(data++);

 k *= m;
 k ^= k >> r;
Expand Down