tei/MapLibreParticleLab
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[build] Fix undefined behavour sanitizer (#16375)

Browse Source 
* [build] Fix integer overflow runtime error for core part

Temporarily remove circle ci UBSAN build precondition

* [build] Enable all of the ubsans

[build] Check runtime error

[build] Update UBSAN_OPTION

* [build] Add UBSAN blacklist

[build] Ignore system libraries

[build] Ignore vendor library

* [build] Fix implicit conversion runtime error in core

* [build] Fix division by zero runtime error

* [build] Add unfixed error to ubsan blacklist

* [build] Make UBSAN halt on error

Revert "Temporary remove build precondition"

* [build] Fix division by zero error

* [build] Make UBSAN officially work without FIXME prefix

* [build] Fix implicit conversion from int64_t to uint64_t

* [build] Rename style test json file name

* Address review findings
This commit is contained in:
zmiao
2020-04-07 16:30:30 +03:00
committed by GitHub
parent 8986b558eb
commit bf4c7340f3
21 changed files with 110 additions and 51 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
CMakeLists.txt
View File

@@ -27,6 +27,8 @@ add_library(
mbgl-compiler-options INTERFACE
)
set(UBSAN_BLACKLIST ${PROJECT_SOURCE_DIR}/scripts/ubsan.blacklist)
target_compile_options(
mbgl-compiler-options
INTERFACE
@@ -36,9 +38,16 @@ target_compile_options(
$<$<BOOL:${MBGL_WITH_SANITIZER}>:-fno-optimize-sibling-calls>
$<$<STREQUAL:${MBGL_WITH_SANITIZER},address>:-fsanitize=address>
$<$<STREQUAL:${MBGL_WITH_SANITIZER},thread>:-fsanitize=thread>
$<$<STREQUAL:${MBGL_WITH_SANITIZER},undefined>:-fsanitize=implicit-conversion>
$<$<STREQUAL:${MBGL_WITH_SANITIZER},undefined>:-fsanitize=undefined>
$<$<STREQUAL:${MBGL_WITH_SANITIZER},undefined>:-fsanitize=unsigned-integer-overflow>
$<$<STREQUAL:${MBGL_WITH_SANITIZER},undefined>:-fsanitize=implicit-conversion,
-fsanitize-blacklist=${UBSAN_BLACKLIST}>
$<$<STREQUAL:${MBGL_WITH_SANITIZER},undefined>:-fsanitize=undefined,
-fsanitize-blacklist=${UBSAN_BLACKLIST}>
$<$<STREQUAL:${MBGL_WITH_SANITIZER},undefined>:-fsanitize=unsigned-integer-overflow,
-fsanitize-blacklist=${UBSAN_BLACKLIST}>
$<$<STREQUAL:${MBGL_WITH_SANITIZER},undefined>:-fsanitize=nullability,
-fsanitize-blacklist=${UBSAN_BLACKLIST}>
$<$<STREQUAL:${MBGL_WITH_SANITIZER},undefined>:-fsanitize=float-divide-by-zero,
-fsanitize-blacklist=${UBSAN_BLACKLIST}>
$<$<AND:$<COMPILE_LANGUAGE:CXX>,$<NOT:$<BOOL:${MBGL_WITH_RTTI}>>>:-fno-rtti>
$<$<AND:$<COMPILE_LANGUAGE:CXX>,$<NOT:$<PLATFORM_ID:Windows>>>:-Wall>
$<$<AND:$<COMPILE_LANGUAGE:CXX>,$<NOT:$<PLATFORM_ID:Windows>>>:-Wshadow>

5
circle.yml
View File

@@ -114,13 +114,12 @@ workflows:
config_params: '-G Ninja -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DCMAKE_C_COMPILER=clang-8 -DCMAKE_CXX_COMPILER=clang++-8 -DCMAKE_CXX_FLAGS=-DSANITIZE'
test_params: '-j 4 -E "mbgl-node|gl-benchmark" -D ExperimentalMemCheck || true'
- build-template:
name: FIXME-linux-ubsan
name: linux-ubsan
executor_name: ubuntu-disco
target_is_linux: true
requires:
- sanity-checks
config_params: '-G Ninja -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DCMAKE_C_COMPILER=clang-8 -DCMAKE_CXX_COMPILER=clang++-8 -DMBGL_WITH_SANITIZER=undefined'
test_params: '|| true'
style_tests: true
- build-template:
name: qt5-linux-gcc5-release
@@ -244,7 +243,7 @@ executors:
working_directory: /src
environment:
DISPLAY: :99
UBSAN_OPTIONS: print_stacktrace=1:halt_on_error=0
UBSAN_OPTIONS: print_stacktrace=1:halt_on_error=1:report_error_type=1
ASAN_OPTIONS: strict_string_checks=1:detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1:halt_on_error=0
QT_INSTALL_DOCS: /usr/share/qt5/doc
QT_VERSION: 5

0
metrics/FIXME-linux-ubsan-style.json → metrics/linux-ubsan-style.json
View File

11
platform/default/src/mbgl/storage/offline_database.cpp
View File

@@ -1275,7 +1275,8 @@ bool OfflineDatabase::evict(uint64_t neededFreeSize, DatabaseSizeChangeStats& st
const uint64_t tileChanges = tileQuery.changes();
// Update current ambient cache size, based on how many bytes were released.
newAmbientCacheSize = std::max<int64_t>(newAmbientCacheSize - stats.bytesReleased(), 0u);
newAmbientCacheSize = std::max<int64_t>(
static_cast<int64_t>(newAmbientCacheSize) - static_cast<int64_t>(stats.bytesReleased()), 0u);
// The cached value of offlineTileCount does not need to be updated
// here because only non-offline tiles can be removed by eviction.
@@ -1465,9 +1466,9 @@ uint64_t OfflineDatabase::DatabaseSizeChangeStats::pageSize() const {
}
int64_t OfflineDatabase::DatabaseSizeChangeStats::diff() const {
const uint64_t currentSize =
pageSize_ * (db->getPragma<int64_t>("PRAGMA page_count") - db->getPragma<int64_t>("PRAGMA freelist_count"));
return currentSize - initialSize_;
const int64_t currentSize = static_cast<int64_t>(pageSize_) * (db->getPragma<int64_t>("PRAGMA page_count") -
db->getPragma<int64_t>("PRAGMA freelist_count"));
return currentSize - static_cast<int64_t>(initialSize_);
}
uint64_t OfflineDatabase::DatabaseSizeChangeStats::bytesReleased() const {
@@ -1478,7 +1479,7 @@ uint64_t OfflineDatabase::DatabaseSizeChangeStats::bytesReleased() const {
void OfflineDatabase::updateAmbientCacheSize(DatabaseSizeChangeStats& stats) {
assert(currentAmbientCacheSize);
if (currentAmbientCacheSize) {
*currentAmbientCacheSize = std::max<int64_t>(*currentAmbientCacheSize + stats.diff(), 0u);
*currentAmbientCacheSize = std::max<int64_t>(static_cast<int64_t>(*currentAmbientCacheSize) + stats.diff(), 0u);
}
}

3
platform/default/src/mbgl/util/image.cpp
View File

@@ -12,7 +12,8 @@ PremultipliedImage decodeImage(const std::string& string) {
const size_t size = string.size();
if (size >= 4) {
uint32_t magic = (data[0] << 24) | (data[1] << 16) | (data[2] << 8) | data[3];
uint32_t magic = (static_cast<uint32_t>(data[0]) << 24) | (static_cast<uint32_t>(data[1]) << 16) |
(static_cast<uint32_t>(data[2]) << 8) | static_cast<uint32_t>(data[3]);
if (magic == 0x89504E47U) {
return decodePNG(data, size);
}

4
platform/default/src/mbgl/util/timer.cpp
View File

@@ -60,8 +60,8 @@ Timer::Timer()
Timer::~Timer() = default;
void Timer::start(Duration timeout, Duration repeat, std::function<void()>&& cb) {
impl->start(std::chrono::duration_cast<Milliseconds>(timeout).count(),
std::chrono::duration_cast<Milliseconds>(repeat).count(),
impl->start(static_cast<uint64_t>(std::chrono::duration_cast<Milliseconds>(timeout).count()),
static_cast<uint64_t>(std::chrono::duration_cast<Milliseconds>(repeat).count()),
std::move(cb));
}

28
scripts/ubsan.blacklist Normal file
View File

@@ -0,0 +1,28 @@
#############################################################################
# Ignore system libraries.
src:*/usr/*
#############################################################################
# Mostly hash functions where integer overflow is OK.
src:*/mbgl/util/hash.hpp
#############################################################################
# vendor libs
# unsigned integer overflow: unsigned interger index in for loop
src:*/vendor/earcut.hpp/include/mapbox/earcut.hpp
# unsigned integer overflow: hash function: str_hash
src:*/vendor/eternal/*/eternal.hpp
# unsigned integer overflow
src:*/vendor/*/rapidjson/reader.h
# division by zero: ill usage of test data in 'render-tests/filter/in', latitude shouldn't be +/-90
src:*/vendor/*/geojsonvt/convert.hpp
# Boost bug: Implicit conversion
src:*/include/boost/locale/utf.hpp
# Boost bug: unsigned integer overflow
src:*/include/boost/crc.hpp

8
src/mbgl/geometry/line_atlas.cpp
View File

@@ -54,13 +54,17 @@ void addRoundDash(
if (ranges.empty()) return;
for (int y = -n; y <= n; y++) {
int row = yOffset + n + y;
int row = static_cast<int32_t>(yOffset) + n + y;
const uint32_t index = image.size.width * row;
uint32_t currIndex = 0;
DashRange range = ranges[currIndex];
for (uint32_t x = 0; x < image.size.width; ++x) {
if (x / range.right > 1.0f && ++currIndex < ranges.size()) {
if (range.right == 0) {
if (x != 0 && ++currIndex < ranges.size()) {
range = ranges[currIndex];
}
} else if (x / range.right > 1.0f && ++currIndex < ranges.size()) {
range = ranges[currIndex];
}

4
src/mbgl/layout/symbol_layout.cpp
View File

@@ -738,10 +738,12 @@ std::vector<float> SymbolLayout::calculateTileDistances(const GeometryCoordinate
sumForwardLength += util::dist<float>(line[i + 1], line[i]);
}
}
for (std::size_t i = ++segment; i-- > 0;) {
for (std::size_t i = segment;; --i) {
tileDistances[i] = sumBackwardLength;
if (i != 0u) {
sumBackwardLength += util::dist<float>(line[i - 1], line[i]);
} else {
break; // Add break to avoid unsigned integer overflow when i==0
}
}
}

7
src/mbgl/map/transform_state.cpp
View File

@@ -70,8 +70,11 @@ void TransformState::matrixFor(mat4& matrix, const UnwrappedTileID& tileID) cons
const double s = Projection::worldSize(scale) / tileScale;
matrix::identity(matrix);
matrix::translate(
matrix, matrix, int64_t(tileID.canonical.x + tileID.wrap * tileScale) * s, int64_t(tileID.canonical.y) * s, 0);
matrix::translate(matrix,
matrix,
int64_t(tileID.canonical.x + tileID.wrap * static_cast<int64_t>(tileScale)) * s,
int64_t(tileID.canonical.y) * s,
0);
matrix::scale(matrix, matrix, s / util::EXTENT, s / util::EXTENT, 1);
}

2
src/mbgl/renderer/layers/render_line_layer.cpp
View File

@@ -242,7 +242,7 @@ GeometryCollection offsetLine(const GeometryCollection& rings, double offset) {
Point<double> extrude = util::unit(aToB + bToC);
const double cosHalfAngle = extrude.x * bToC.x + extrude.y * bToC.y;
extrude *= (1.0 / cosHalfAngle);
extrude *= (cosHalfAngle != 0) ? (1.0 / cosHalfAngle) : 0;
newRing.emplace_back(convertPoint<int16_t>(extrude * offset) + p);
}

8
src/mbgl/renderer/renderer_impl.cpp
View File

@@ -108,8 +108,8 @@ void Renderer::Impl::render(const RenderTree& renderTree) {
}
parameters.staticData.depthRenderbuffer->setShouldClear(true);
uint32_t i = static_cast<uint32_t>(layerRenderItems.size()) - 1;
for (auto it = layerRenderItems.begin(); it != layerRenderItems.end(); ++it, --i) {
int32_t i = static_cast<int32_t>(layerRenderItems.size()) - 1;
for (auto it = layerRenderItems.begin(); it != layerRenderItems.end() && i >= 0; ++it, --i) {
parameters.currentLayer = i;
const RenderItem& renderItem = it->get();
if (renderItem.hasRenderPass(parameters.pass)) {
@@ -159,8 +159,8 @@ void Renderer::Impl::render(const RenderTree& renderTree) {
parameters.pass = RenderPass::Translucent;
const auto debugGroup(parameters.renderPass->createDebugGroup("translucent"));
uint32_t i = static_cast<uint32_t>(layerRenderItems.size()) - 1;
for (auto it = layerRenderItems.begin(); it != layerRenderItems.end(); ++it, --i) {
int32_t i = static_cast<int32_t>(layerRenderItems.size()) - 1;
for (auto it = layerRenderItems.begin(); it != layerRenderItems.end() && i >= 0; ++it, --i) {
parameters.currentLayer = i;
const RenderItem& renderItem = it->get();
if (renderItem.hasRenderPass(parameters.pass)) {

22
src/mbgl/sprite/sprite_parser.cpp
View File

@@ -17,21 +17,22 @@ namespace mbgl {
std::unique_ptr<style::Image> createStyleImage(const std::string& id,
const PremultipliedImage& image,
const uint32_t srcX,
const uint32_t srcY,
const uint32_t width,
const uint32_t height,
const int32_t srcX,
const int32_t srcY,
const int32_t width,
const int32_t height,
const double ratio,
const bool sdf,
style::ImageStretches&& stretchX,
style::ImageStretches&& stretchY,
const optional<style::ImageContent>& content) {
// Disallow invalid parameter configurations.
if (width <= 0 || height <= 0 || width > 1024 || height > 1024 || ratio <= 0 || ratio > 10 ||
srcX >= image.size.width || srcY >= image.size.height || srcX + width > image.size.width ||
srcY + height > image.size.height) {
if (width <= 0 || height <= 0 || width > 1024 || height > 1024 || ratio <= 0 || ratio > 10 || srcX < 0 ||
srcY < 0 || srcX >= static_cast<int32_t>(image.size.width) || srcY >= static_cast<int32_t>(image.size.height) ||
srcX + width > static_cast<int32_t>(image.size.width) ||
srcY + height > static_cast<int32_t>(image.size.height)) {
Log::Error(Event::Sprite,
"Can't create image with invalid metrics: %ux%u@%u,%u in %ux%u@%sx sprite",
"Can't create image with invalid metrics: %dx%d@%d,%d in %ux%u@%sx sprite",
width,
height,
srcX,
@@ -42,10 +43,11 @@ std::unique_ptr<style::Image> createStyleImage(const std::string& id,
return nullptr;
}
PremultipliedImage dstImage({width, height});
const Size size(static_cast<uint32_t>(width), static_cast<uint32_t>(height));
PremultipliedImage dstImage(size);
// Copy from the source image into our individual sprite image
PremultipliedImage::copy(image, dstImage, {srcX, srcY}, {0, 0}, {width, height});
PremultipliedImage::copy(image, dstImage, {static_cast<uint32_t>(srcX), static_cast<uint32_t>(srcY)}, {0, 0}, size);
try {
return std::make_unique<style::Image>(

8
src/mbgl/sprite/sprite_parser.hpp
View File

@@ -9,10 +9,10 @@ namespace mbgl {
// Extracts an individual image from a spritesheet from the given location.
std::unique_ptr<style::Image> createStyleImage(const std::string& id,
const PremultipliedImage&,
uint32_t srcX,
uint32_t srcY,
uint32_t srcWidth,
uint32_t srcHeight,
int32_t srcX,
int32_t srcY,
int32_t srcWidth,
int32_t srcHeight,
double ratio,
bool sdf,
style::ImageStretches&& stretchX = {},

11
src/mbgl/style/expression/compound_expression.cpp
View File

@@ -15,6 +15,7 @@
#include <mapbox/eternal.hpp>
#include <cmath>
#include <limits>
namespace mbgl {
namespace style {
@@ -498,7 +499,15 @@ const auto& multiplyCompoundExpression() {
}
const auto& divideCompoundExpression() {
static auto signature = detail::makeSignature("/", [](double a, double b) -> Result<double> { return a / b; });
static auto signature = detail::makeSignature("/", [](double a, double b) -> Result<double> {
if (b == 0) {
if (a == 0) return std::numeric_limits<double>::quiet_NaN();
double inf = std::numeric_limits<double>::infinity();
if (a > 0) return inf;
if (a < 0) return -inf;
}
return a / b;
});
return signature;
}

2
src/mbgl/text/tagged_string.cpp
View File

@@ -29,7 +29,7 @@ void TaggedString::addImageSection(const std::string& imageID) {
styledText.first += *nextImageSectionCharCode;
sections.emplace_back(imageID);
styledText.second.resize(styledText.first.size(), sections.size() - 1);
styledText.second.resize(styledText.first.size(), static_cast<uint8_t>(sections.size() - 1));
}
optional<char16_t> TaggedString::getNextImageSectionCharCode() {

5
src/mbgl/tile/raster_dem_tile.cpp
View File

@@ -84,8 +84,9 @@ HillshadeBucket* RasterDEMTile::getBucket() const {
}
void RasterDEMTile::backfillBorder(const RasterDEMTile& borderTile, const DEMTileNeighbors mask) {
int32_t dx = borderTile.id.canonical.x - id.canonical.x;
const int8_t dy = borderTile.id.canonical.y - id.canonical.y;
int32_t dx = static_cast<int32_t>(borderTile.id.canonical.x) - static_cast<int32_t>(id.canonical.x);
const auto dy =
static_cast<int8_t>(static_cast<int32_t>(borderTile.id.canonical.y) - static_cast<int32_t>(id.canonical.y));
const uint32_t dim = pow(2, id.canonical.z);
if (dx == 0 && dy == 0) return;
if (std::abs(dy) > 1) return;

6
src/mbgl/util/premultiply.cpp
View File

@@ -40,9 +40,9 @@ UnassociatedImage unpremultiply(PremultipliedImage&& src) {
uint8_t& b = data[i + 2];
uint8_t& a = data[i + 3];
if (a) {
r = (255 * r + (a / 2)) / a;
g = (255 * g + (a / 2)) / a;
b = (255 * b + (a / 2)) / a;
r = static_cast<uint8_t>((255 * r + (a / 2)) / a);
g = static_cast<uint8_t>((255 * g + (a / 2)) / a);
b = static_cast<uint8_t>((255 * b + (a / 2)) / a);
}
}

2
src/mbgl/util/tiny_sdf.cpp
View File

@@ -35,7 +35,7 @@ void edt1d(std::vector<double>& f,
for (uint32_t q = 0, k = 0; q < n; q++) {
while (z[k + 1] < q) k++;
d[q] = (q - v[k]) * (q - v[k]) + f[v[k]];
d[q] = (static_cast<double>(q) - v[k]) * (static_cast<double>(q) - v[k]) + f[v[k]];
}
}

8
test/sprite/sprite_parser.test.cpp
View File

@@ -54,7 +54,7 @@ TEST(Sprite, SpriteImageCreationInvalid) {
EventSeverity::Error,
Event::Sprite,
int64_t(-1),
"Can't create image with invalid metrics: 4294967295x16@0,0 in 200x299@1x sprite",
"Can't create image with invalid metrics: -1x16@0,0 in 200x299@1x sprite",
}));
EXPECT_EQ(0u, log.uncheckedCount());
@@ -64,7 +64,7 @@ TEST(Sprite, SpriteImageCreationInvalid) {
EventSeverity::Error,
Event::Sprite,
int64_t(-1),
"Can't create image with invalid metrics: 16x4294967295@0,0 in 200x299@1x sprite",
"Can't create image with invalid metrics: 16x-1@0,0 in 200x299@1x sprite",
}));
EXPECT_EQ(0u, log.uncheckedCount());
@@ -124,7 +124,7 @@ TEST(Sprite, SpriteImageCreationInvalid) {
EventSeverity::Error,
Event::Sprite,
int64_t(-1),
"Can't create image with invalid metrics: 16x16@4294967295,0 in 200x299@1x sprite",
"Can't create image with invalid metrics: 16x16@-1,0 in 200x299@1x sprite",
}));
EXPECT_EQ(0u, log.uncheckedCount());
@@ -134,7 +134,7 @@ TEST(Sprite, SpriteImageCreationInvalid) {
EventSeverity::Error,
Event::Sprite,
int64_t(-1),
"Can't create image with invalid metrics: 16x16@0,4294967295 in 200x299@1x sprite",
"Can't create image with invalid metrics: 16x16@0,-1 in 200x299@1x sprite",
}));
EXPECT_EQ(0u, log.uncheckedCount());

2
test/storage/offline_database.test.cpp
View File

@@ -34,7 +34,7 @@ static std::shared_ptr<std::string> randomString(size_t size) {
std::mt19937 random;
for (size_t i = 0; i < size; i++) {
(*result)[i] = random();
(*result)[i] = static_cast<char>(random());
}
return result;