Commit 148432cd authored by Lucian Grijincu's avatar Lucian Grijincu Committed by Facebook GitHub Bot

folly::io::Cursor: fix UndefinedBehaviorSanitizer: nullptr-with-nonzero-offset

Summary:
Per standard pointer arithmetic is only defined when keeping pointers within the bounds of an array object.

 Applying non-zero offset to nullptr (or making non-nullptr a nullptr by subtracting pointer's integral value from the pointer itself) is undefined behavior.

Since https://reviews.llvm.org/D66608 `[InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null) LLVM middle-end uses those guarantees for transformations.` and mis-compilations have been observed:
- https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20190826/687838.html
- https://github.com/google/filament/pull/1566

To prevent help weed out bugs before they lead to future miscompilations a new UBSAN check has been added: `nullptr-with-nonzero-offset`
- https://reviews.llvm.org/D67122 [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

`folly::io::Cursor` does this type of operations when checking if `crtPos_ + N <= crtEnd_`: when it's empty it becomes `nullptr + N <= nullptr`:
  const uint8_t* crtBegin_{nullptr};
  const uint8_t* crtEnd_{nullptr};
  const uint8_t* crtPos_{nullptr};

Switch to `uintptr_t` space where the math is no longer UB.

Reviewed By: yfeldblum

Differential Revision: D33737556

fbshipit-source-id: 588b91ac1387112a6f183edfda5555ca1b7193d8
parent 30073fb9
...@@ -18,6 +18,7 @@ ...@@ -18,6 +18,7 @@
#include <cassert> #include <cassert>
#include <cstdarg> #include <cstdarg>
#include <cstdint>
#include <cstring> #include <cstring>
#include <memory> #include <memory>
#include <stdexcept> #include <stdexcept>
...@@ -71,7 +72,7 @@ class CursorBase { ...@@ -71,7 +72,7 @@ class CursorBase {
if (crtBuf_) { if (crtBuf_) {
crtPos_ = crtBegin_ = crtBuf_->data(); crtPos_ = crtBegin_ = crtBuf_->data();
crtEnd_ = crtBuf_->tail(); crtEnd_ = crtBuf_->tail();
if (crtPos_ + len < crtEnd_) { if (uintptr_t(crtPos_) + len < uintptr_t(crtEnd_)) {
crtEnd_ = crtPos_ + len; crtEnd_ = crtPos_ + len;
} }
remainingLen_ = len - (crtEnd_ - crtPos_); remainingLen_ = len - (crtEnd_ - crtPos_);
...@@ -106,7 +107,7 @@ class CursorBase { ...@@ -106,7 +107,7 @@ class CursorBase {
if (cursor.isBounded() && len > cursor.remainingLen_ + cursor.length()) { if (cursor.isBounded() && len > cursor.remainingLen_ + cursor.length()) {
throw_exception<std::out_of_range>("underflow"); throw_exception<std::out_of_range>("underflow");
} }
if (crtPos_ + len < crtEnd_) { if (uintptr_t(crtPos_) + len < uintptr_t(crtEnd_)) {
crtEnd_ = crtPos_ + len; crtEnd_ = crtPos_ + len;
} }
remainingLen_ = len - (crtEnd_ - crtPos_); remainingLen_ = len - (crtEnd_ - crtPos_);
...@@ -241,7 +242,7 @@ class CursorBase { ...@@ -241,7 +242,7 @@ class CursorBase {
crtBegin_ = crtBuf_->data(); crtBegin_ = crtBuf_->data();
crtEnd_ = crtBuf_->tail(); crtEnd_ = crtBuf_->tail();
if (isBounded()) { if (isBounded()) {
if (crtBegin_ + remainingLen_ < crtEnd_) { if (uintptr_t(crtBegin_) + remainingLen_ < uintptr_t(crtEnd_)) {
crtEnd_ = crtBegin_ + remainingLen_; crtEnd_ = crtBegin_ + remainingLen_;
} }
remainingLen_ -= crtEnd_ - crtBegin_; remainingLen_ -= crtEnd_ - crtBegin_;
...@@ -303,7 +304,7 @@ class CursorBase { ...@@ -303,7 +304,7 @@ class CursorBase {
template <class T> template <class T>
typename std::enable_if<std::is_arithmetic<T>::value, bool>::type tryRead( typename std::enable_if<std::is_arithmetic<T>::value, bool>::type tryRead(
T& val) { T& val) {
if (LIKELY(crtPos_ + sizeof(T) <= crtEnd_)) { if (LIKELY(uintptr_t(crtPos_) + sizeof(T) <= uintptr_t(crtEnd_))) {
val = loadUnaligned<T>(data()); val = loadUnaligned<T>(data());
crtPos_ += sizeof(T); crtPos_ += sizeof(T);
return true; return true;
...@@ -327,7 +328,7 @@ class CursorBase { ...@@ -327,7 +328,7 @@ class CursorBase {
template <class T> template <class T>
T read() { T read() {
if (LIKELY(crtPos_ + sizeof(T) <= crtEnd_)) { if (LIKELY(uintptr_t(crtPos_) + sizeof(T) <= uintptr_t(crtEnd_))) {
T val = loadUnaligned<T>(data()); T val = loadUnaligned<T>(data());
crtPos_ += sizeof(T); crtPos_ += sizeof(T);
return val; return val;
...@@ -408,7 +409,7 @@ class CursorBase { ...@@ -408,7 +409,7 @@ class CursorBase {
size_t skipAtMost(size_t len) { size_t skipAtMost(size_t len) {
dcheckIntegrity(); dcheckIntegrity();
if (LIKELY(crtPos_ + len < crtEnd_)) { if (LIKELY(uintptr_t(crtPos_) + len < uintptr_t(crtEnd_))) {
crtPos_ += len; crtPos_ += len;
return len; return len;
} }
...@@ -417,7 +418,7 @@ class CursorBase { ...@@ -417,7 +418,7 @@ class CursorBase {
void skip(size_t len) { void skip(size_t len) {
dcheckIntegrity(); dcheckIntegrity();
if (LIKELY(crtPos_ + len < crtEnd_)) { if (LIKELY(uintptr_t(crtPos_) + len < uintptr_t(crtEnd_))) {
crtPos_ += len; crtPos_ += len;
} else { } else {
skipSlow(len); skipSlow(len);
...@@ -454,7 +455,7 @@ class CursorBase { ...@@ -454,7 +455,7 @@ class CursorBase {
size_t pullAtMost(void* buf, size_t len) { size_t pullAtMost(void* buf, size_t len) {
dcheckIntegrity(); dcheckIntegrity();
// Fast path: it all fits in one buffer. // Fast path: it all fits in one buffer.
if (LIKELY(crtPos_ + len <= crtEnd_)) { if (LIKELY(uintptr_t(crtPos_) + len <= uintptr_t(crtEnd_))) {
memcpy(buf, data(), len); memcpy(buf, data(), len);
crtPos_ += len; crtPos_ += len;
return len; return len;
...@@ -467,7 +468,7 @@ class CursorBase { ...@@ -467,7 +468,7 @@ class CursorBase {
return; return;
} }
dcheckIntegrity(); dcheckIntegrity();
if (LIKELY(crtPos_ + len <= crtEnd_)) { if (LIKELY(uintptr_t(crtPos_) + len <= uintptr_t(crtEnd_))) {
memcpy(buf, data(), len); memcpy(buf, data(), len);
crtPos_ += len; crtPos_ += len;
} else { } else {
...@@ -646,7 +647,7 @@ class CursorBase { ...@@ -646,7 +647,7 @@ class CursorBase {
crtPos_ = crtBegin_ = crtBuf_->data(); crtPos_ = crtBegin_ = crtBuf_->data();
crtEnd_ = crtBuf_->tail(); crtEnd_ = crtBuf_->tail();
if (isBounded()) { if (isBounded()) {
if (crtPos_ + remainingLen_ < crtEnd_) { if (uintptr_t(crtPos_) + remainingLen_ < uintptr_t(crtEnd_)) {
crtEnd_ = crtPos_ + remainingLen_; crtEnd_ = crtPos_ + remainingLen_;
} }
remainingLen_ -= crtEnd_ - crtPos_; remainingLen_ -= crtEnd_ - crtPos_;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment