Configure Undefined Behaviour Sanitizer #6290

SiarheiFedartsou · 2022-07-28T20:49:35Z

Issue

Configures undefined behaviour sanitizer. Reports trivial to fix were fixed, other ones are suppressed.

Tasklist

CHANGELOG.md entry (How to write a changelog entry)
update relevant Wiki pages
add tests (see testing documentation)
review
adjust for comments
cherry pick to release branch

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

SiarheiFedartsou · 2022-07-28T20:53:15Z

Hi @mjjbell
Sorry for pinging you too often, but kindly ask you to take a look please :)

SiarheiFedartsou · 2022-07-28T20:54:14Z

include/extractor/extractor_config.hpp

-    bool use_metadata;
-    bool parse_conditionals;
-    bool use_locations_cache;
+    bool use_metadata = false;


Curious why usual compiler warnings didn't catch that 🤔

SiarheiFedartsou · 2022-07-28T20:55:26Z

src/util/log.cpp

@@ -91,8 +95,6 @@ Log::Log(LogLevel level_, std::ostream &ostream) : level(level_), stream(ostream
    }
 }

-Log::Log(LogLevel level_) : Log(level_, buffer) {}


It seems this is considered as UB: we are using field of class itself(buffer) when calling constructor of this class.

SiarheiFedartsou · 2022-07-28T20:55:59Z

.github/workflows/osrm-backend.yml

@@ -553,6 +554,12 @@ jobs:
      if: ${{ matrix.NODE_PACKAGE_TESTS_ONLY == 'ON' }}
      run: |
        npm run nodejs-tests
+    - name: Upload test logs


Helpful if we want to see logs of nodejs tests failed on CI.

SiarheiFedartsou · 2022-07-29T20:24:06Z

.github/workflows/osrm-backend.yml

@@ -74,7 +74,8 @@ jobs:
            CUCUMBER_TIMEOUT: 20000
            CXXCOMPILER: g++-9
            ENABLE_SANITIZER: ON
-            TARGET_ARCH: x86_64-asan
+            TARGET_ARCH: x86_64-asan-ubsan
+            OSRM_CONNECTION_RETRIES: 30


Some NodeJs tests became flaky after configuring this sanitizer, because as I understand OSRM startup time has increased and 10 retries we are using by default was not enough, so had to add this option and set it to 30.

SiarheiFedartsou · 2022-07-29T20:28:18Z

include/extractor/extractor_config.hpp

-    bool use_locations_cache;
+    bool use_metadata = false;
+    bool parse_conditionals = false;
+    bool use_locations_cache = true;


Values are chosen as default values we use in program_options.

mjjbell

👍

My only concern is the sanitizer job now taking an hour to run in CI.

I guess the activity in this repo is low enough that it's not going to slow down anyone's productivity though.

mjjbell · 2022-07-30T12:22:11Z

features/lib/osrm_loader.js

@@ -45,11 +45,11 @@ class OSRMBaseLoader{
        var retryCount = 0;
        let retry = (err) => {
          if (err) {
-            if (retryCount < 10) {
+            if (retryCount < this.scope.OSRM_CONNECTION_RETRIES) {


We could also add some exponential back-off, which might be more robust to fluctuations in CI startup times.

Added OSRM_CONNECTION_EXP_BACKOFF_COEF, please take a look.

SiarheiFedartsou · 2022-08-01T17:43:02Z

👍

My only concern is the sanitizer job now taking an hour to run in CI.

I guess the activity in this repo is low enough that it's not going to slow down anyone's productivity though.

Agree here. Also worth noting that it is only gcc based job has this slow down. Tbh I don't see a lot of value in having UBSAN configured for both GCC and UBSAN and we can simply introduce CMake flag to separately enable/disable ASAN and UBSAN and turn off it for GCC job. But not sure if worth spending time on it. WDYT?

mjjbell · 2022-08-01T20:48:21Z

Agree here. Also worth noting that it is only gcc based job has this slow down. Tbh I don't see a lot of value in having UBSAN configured for both GCC and UBSAN and we can simply introduce CMake flag to separately enable/disable ASAN and UBSAN and turn off it for GCC job. But not sure if worth spending time on it. WDYT?

I don't think it's worth spending time on. Some of the other jobs are already taking 40+ minutes, so it won't be a huge saving.
If it becomes a big hit on productivity then it can be explored, but I suspect that would also require finding a more performant CI service.

v5.27.0 - Changes from 5.26.0 - API: - ADDED: Add Flatbuffers support to NodeJS bindings. [Project-OSRM#6338](Project-OSRM#6338) - CHANGED: Add `data_version` field to responses of all services. [Project-OSRM#5387](Project-OSRM#5387) - FIXED: Use Boost.Beast to parse HTTP request. [Project-OSRM#6294](Project-OSRM#6294) - FIXED: Fix inefficient osrm-routed connection handling [Project-OSRM#6113](https://gihub.com/Project-OSRM/osrm-backend/pull/6113) - FIXED: Fix HTTP compression precedence [Project-OSRM#6113](Project-OSRM#6113) - NodeJS: - FIXED: Support `skip_waypoints` in Node bindings [Project-OSRM#6060](Project-OSRM#6060) - Misc: - ADDED: conanbuildinfo.json for easy reading of dependencies [Project-OSRM#6388](Project-OSRM#6388) - CHANGED: Improve performance of JSON rendering. Fix undefined behaviour in JSON numbers formatting. [Project-OSRM#6380](Project-OSRM#6380) - ADDED: Add timestamps for logs. [Project-OSRM#6375](Project-OSRM#6375) - CHANGED: Improve performance of map matching via getPathDistance optimization. [Project-OSRM#6378](Project-OSRM#6378) - CHANGED: Optimize RestrictionParser performance. [Project-OSRM#6344](Project-OSRM#6344) - ADDED: Support floats for speed value in traffic updates CSV. [Project-OSRM#6327](Project-OSRM#6327) - CHANGED: Use Lua 5.4 in Docker image. [Project-OSRM#6346](Project-OSRM#6346) - CHANGED: Remove redundant nullptr check. [Project-OSRM#6326](Project-OSRM#6326) - CHANGED: missing files list is included in exception message. [Project-OSRM#5360](Project-OSRM#5360) - CHANGED: Do not use deprecated Callback::Call overload in Node bindings. [Project-OSRM#6318](Project-OSRM#6318) - FIXED: Fix distance calculation consistency. [Project-OSRM#6315](Project-OSRM#6315) - FIXED: Fix performance issue after migration to sol2 3.3.0. [Project-OSRM#6304](Project-OSRM#6304) - CHANGED: Pass osm_node_ids by reference in osrm::updater::Updater class. [Project-OSRM#6298](Project-OSRM#6298) - FIXED: Fix bug with reading Set values from Lua scripts. [Project-OSRM#6285](Project-OSRM#6285) - FIXED: Bug in bicycle profile that caused exceptions if there is a highway=bicycle in the data. [Project-OSRM#6296](Project-OSRM#6296) - FIXED: Internal refactoring of identifier types used in data facade [Project-OSRM#6044](Project-OSRM#6044) - CHANGED: Update docs to reflect recent build and dependency changes [Project-OSRM#6383](Project-OSRM#6383) - Build: - REMOVED: Get rid of Mason. [Project-OSRM#6387](Project-OSRM#6387) - CHANGED: Use clang-format from CI base image. [Project-OSRM#6391](Project-OSRM#6391) - ADDED: Build Node bindings on Windows. [Project-OSRM#6334](Project-OSRM#6334) - ADDED: Configure cross-compilation for Apple Silicon. [Project-OSRM#6360](Project-OSRM#6360) - CHANGED: Use apt-get to install Clang on CI. [Project-OSRM#6345](Project-OSRM#6345) - CHANGED: Fix TBB in case of Conan + NodeJS build. [Project-OSRM#6333](Project-OSRM#6333) - CHANGED: Migrate to modern TBB version. [Project-OSRM#6300](Project-OSRM#6300) - CHANGED: Enable performance-move-const-arg clang-tidy check. [Project-OSRM#6319](Project-OSRM#6319) - CHANGED: Use the latest node on CI. [Project-OSRM#6317](Project-OSRM#6317) - CHANGED: Migrate Windows CI to GitHub Actions. [Project-OSRM#6312](Project-OSRM#6312) - ADDED: Add smoke test for Docker image. [Project-OSRM#6313](Project-OSRM#6313) - CHANGED: Update libosmium to version 2.18.0. [Project-OSRM#6303](Project-OSRM#6303) - CHANGED: Remove EXACT from find_package if using Conan. [Project-OSRM#6299](Project-OSRM#6299) - CHANGED: Configure Undefined Behaviour Sanitizer. [Project-OSRM#6290](Project-OSRM#6290) - CHANGED: Use Conan instead of Mason to install code dependencies. [Project-OSRM#6284](Project-OSRM#6284) - CHANGED: Migrate to C++17. Update sol2 to 3.3.0. [Project-OSRM#6279](Project-OSRM#6279) - CHANGED: Update macOS CI image to macos-11. [Project-OSRM#6286](Project-OSRM#6286) - CHANGED: Enable even more clang-tidy checks. [Project-OSRM#6273](Project-OSRM#6273) - CHANGED: Configure CMake to not build flatbuffers tests and samples. [Project-OSRM#6274](Project-OSRM#6274) - CHANGED: Enable more clang-tidy checks. [Project-OSRM#6270](Project-OSRM#6270) - CHANGED: Configure clang-tidy job on CI. [Project-OSRM#6261](Project-OSRM#6261) - CHANGED: Use Github Actions for building container images [Project-OSRM#6138](Project-OSRM#6138) - CHANGED: Upgrade Boost dependency to 1.70 [Project-OSRM#6113](Project-OSRM#6113) - CHANGED: Upgrade Ubuntu CI builds to 20.04 [Project-OSRM#6119](Project-OSRM#6119) - CHANGED: Make building osrm-routed optional [Project-OSRM#6144](Project-OSRM#6144) - FIXED: Run all unit tests in CI [Project-OSRM#5248](Project-OSRM#5248) - FIXED: Fix installation of Mason CMake and 32 bit CI build [Project-OSRM#6170](Project-OSRM#6170) - FIXED: Fixed Node docs generation check in CI. [Project-OSRM#6058](Project-OSRM#6058) - CHANGED: Docker build, enabled arm64 build layer [Project-OSRM#6172](Project-OSRM#6172) - CHANGED: Docker build, enabled apt-get update/install caching in separate layer for build phase [Project-OSRM#6175](Project-OSRM#6175) - FIXED: Bump CI complete meta job to ubuntu-20.04 [Project-OSRM#6323](Project-OSRM#6323) - CHANGED: Node packages are now scoped by @Project-OSRM [Project-OSRM#6386](Project-OSRM#6386) - Routing: - CHANGED: Lazily generate optional route path data [Project-OSRM#6045](Project-OSRM#6045) - FIXED: Completed support for no_entry and no_exit turn restrictions. [Project-OSRM#5988](Project-OSRM#5988) - ADDED: Add support for non-round-trips with a single fixed endpoint. [Project-OSRM#6050](Project-OSRM#6050) - FIXED: Improvements to maneuver override processing [Project-OSRM#6125](Project-OSRM#6125) - ADDED: Support snapping to multiple ways at an input location. [Project-OSRM#5953](Project-OSRM#5953) - FIXED: Fix snapping target locations to ways used in turn restrictions. [Project-OSRM#6339](Project-OSRM#6339) - ADDED: Support OSM traffic signal directions. [Project-OSRM#6153](Project-OSRM#6153) - FIXED: Ensure u-turn exists in intersection view. [Project-OSRM#6376](Project-OSRM#6376) - FIXED: Gracefully handle no-turn intersections in guidance processing. [Project-OSRM#6382](Project-OSRM#6382) - Profile: - CHANGED: Bicycle surface speeds [Project-OSRM#6212](Project-OSRM#6212) - Tools: - CHANGED: Do not generate intermediate .osrm file in osrm-extract. [Project-OSRM#6354](Project-OSRM#6354)

SiarheiFedartsou force-pushed the sf-ubsan5 branch from 991dcc2 to 34cfe58 Compare July 28, 2022 20:50

SiarheiFedartsou commented Jul 28, 2022

View reviewed changes

SiarheiFedartsou force-pushed the sf-ubsan5 branch from b9bd3b6 to c93b934 Compare July 29, 2022 20:22

SiarheiFedartsou commented Jul 29, 2022

View reviewed changes

SiarheiFedartsou force-pushed the sf-ubsan5 branch from c93b934 to 2847a87 Compare July 29, 2022 20:26

SiarheiFedartsou commented Jul 29, 2022

View reviewed changes

SiarheiFedartsou force-pushed the sf-ubsan5 branch from 2847a87 to 9d999a9 Compare July 29, 2022 21:01

mjjbell approved these changes Jul 30, 2022

View reviewed changes

Configure Undefined Behaviour Sanitizer

ce92935

SiarheiFedartsou force-pushed the sf-ubsan5 branch from 9d999a9 to ce92935 Compare August 1, 2022 17:40

Merge branch 'master' into sf-ubsan5

4fd7af0

mjjbell merged commit 0081672 into Project-OSRM:master Aug 1, 2022

mattwigway pushed a commit to mattwigway/osrm-backend that referenced this pull request Jul 20, 2023

Configure Undefined Behaviour Sanitizer (Project-OSRM#6290)

7738161

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configure Undefined Behaviour Sanitizer #6290

Configure Undefined Behaviour Sanitizer #6290

SiarheiFedartsou commented Jul 28, 2022

SiarheiFedartsou commented Jul 28, 2022

SiarheiFedartsou Jul 28, 2022

SiarheiFedartsou Jul 28, 2022

SiarheiFedartsou Jul 28, 2022 •

edited

Loading

SiarheiFedartsou Jul 29, 2022

SiarheiFedartsou Jul 29, 2022

mjjbell left a comment

mjjbell Jul 30, 2022

SiarheiFedartsou Aug 1, 2022

SiarheiFedartsou commented Aug 1, 2022

mjjbell commented Aug 1, 2022

Configure Undefined Behaviour Sanitizer #6290

Configure Undefined Behaviour Sanitizer #6290

Conversation

SiarheiFedartsou commented Jul 28, 2022

Issue

Tasklist

Requirements / Relations

SiarheiFedartsou commented Jul 28, 2022

SiarheiFedartsou Jul 28, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Jul 28, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

SiarheiFedartsou Jul 29, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Jul 29, 2022

Choose a reason for hiding this comment

mjjbell left a comment

Choose a reason for hiding this comment

mjjbell Jul 30, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Aug 1, 2022

Choose a reason for hiding this comment

SiarheiFedartsou commented Aug 1, 2022

mjjbell commented Aug 1, 2022

SiarheiFedartsou Jul 28, 2022 •

edited

Loading