Skip to content

Commit

Permalink
folly::io::Cursor: fix UndefinedBehaviorSanitizer: nullptr-with-nonze…
Browse files Browse the repository at this point in the history
…ro-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
- google/filament#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
  • Loading branch information
luciang authored and facebook-github-bot committed Jan 26, 2022
1 parent 30073fb commit 148432c
Showing 1 changed file with 11 additions and 10 deletions.
21 changes: 11 additions & 10 deletions folly/io/Cursor.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

#include <cassert>
#include <cstdarg>
#include <cstdint>
#include <cstring>
#include <memory>
#include <stdexcept>
Expand Down Expand Up @@ -71,7 +72,7 @@ class CursorBase {
if (crtBuf_) {
crtPos_ = crtBegin_ = crtBuf_->data();
crtEnd_ = crtBuf_->tail();
if (crtPos_ + len < crtEnd_) {
if (uintptr_t(crtPos_) + len < uintptr_t(crtEnd_)) {
crtEnd_ = crtPos_ + len;
}
remainingLen_ = len - (crtEnd_ - crtPos_);
Expand Down Expand Up @@ -106,7 +107,7 @@ class CursorBase {
if (cursor.isBounded() && len > cursor.remainingLen_ + cursor.length()) {
throw_exception<std::out_of_range>("underflow");
}
if (crtPos_ + len < crtEnd_) {
if (uintptr_t(crtPos_) + len < uintptr_t(crtEnd_)) {
crtEnd_ = crtPos_ + len;
}
remainingLen_ = len - (crtEnd_ - crtPos_);
Expand Down Expand Up @@ -241,7 +242,7 @@ class CursorBase {
crtBegin_ = crtBuf_->data();
crtEnd_ = crtBuf_->tail();
if (isBounded()) {
if (crtBegin_ + remainingLen_ < crtEnd_) {
if (uintptr_t(crtBegin_) + remainingLen_ < uintptr_t(crtEnd_)) {
crtEnd_ = crtBegin_ + remainingLen_;
}
remainingLen_ -= crtEnd_ - crtBegin_;
Expand Down Expand Up @@ -303,7 +304,7 @@ class CursorBase {
template <class T>
typename std::enable_if<std::is_arithmetic<T>::value, bool>::type tryRead(
T& val) {
if (LIKELY(crtPos_ + sizeof(T) <= crtEnd_)) {
if (LIKELY(uintptr_t(crtPos_) + sizeof(T) <= uintptr_t(crtEnd_))) {
val = loadUnaligned<T>(data());
crtPos_ += sizeof(T);
return true;
Expand All @@ -327,7 +328,7 @@ class CursorBase {

template <class T>
T read() {
if (LIKELY(crtPos_ + sizeof(T) <= crtEnd_)) {
if (LIKELY(uintptr_t(crtPos_) + sizeof(T) <= uintptr_t(crtEnd_))) {
T val = loadUnaligned<T>(data());
crtPos_ += sizeof(T);
return val;
Expand Down Expand Up @@ -408,7 +409,7 @@ class CursorBase {

size_t skipAtMost(size_t len) {
dcheckIntegrity();
if (LIKELY(crtPos_ + len < crtEnd_)) {
if (LIKELY(uintptr_t(crtPos_) + len < uintptr_t(crtEnd_))) {
crtPos_ += len;
return len;
}
Expand All @@ -417,7 +418,7 @@ class CursorBase {

void skip(size_t len) {
dcheckIntegrity();
if (LIKELY(crtPos_ + len < crtEnd_)) {
if (LIKELY(uintptr_t(crtPos_) + len < uintptr_t(crtEnd_))) {
crtPos_ += len;
} else {
skipSlow(len);
Expand Down Expand Up @@ -454,7 +455,7 @@ class CursorBase {
size_t pullAtMost(void* buf, size_t len) {
dcheckIntegrity();
// 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);
crtPos_ += len;
return len;
Expand All @@ -467,7 +468,7 @@ class CursorBase {
return;
}
dcheckIntegrity();
if (LIKELY(crtPos_ + len <= crtEnd_)) {
if (LIKELY(uintptr_t(crtPos_) + len <= uintptr_t(crtEnd_))) {
memcpy(buf, data(), len);
crtPos_ += len;
} else {
Expand Down Expand Up @@ -646,7 +647,7 @@ class CursorBase {
crtPos_ = crtBegin_ = crtBuf_->data();
crtEnd_ = crtBuf_->tail();
if (isBounded()) {
if (crtPos_ + remainingLen_ < crtEnd_) {
if (uintptr_t(crtPos_) + remainingLen_ < uintptr_t(crtEnd_)) {
crtEnd_ = crtPos_ + remainingLen_;
}
remainingLen_ -= crtEnd_ - crtPos_;
Expand Down

0 comments on commit 148432c

Please sign in to comment.