Skip to content
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

Configure Undefined Behaviour Sanitizer #6290

Merged
merged 2 commits into from
Aug 1, 2022
Merged

Configure Undefined Behaviour Sanitizer #6290

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
ce92935
Configure Undefined Behaviour Sanitizer
SiarheiFedartsou Aug 1, 2022
4fd7af0
Merge branch 'master' into sf-ubsan5
mjjbell Aug 1, 2022
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
19 changes: 15 additions & 4 deletions .github/workflows/osrm-backend.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ jobs:
CXXCOMPILER: g++-9
ENABLE_COVERAGE: ON

- name: gcc-9-debug-asan
- name: gcc-9-debug-asan-ubsan
continue-on-error: false
node: 12
runs-on: ubuntu-20.04
Expand All @@ -74,7 +74,9 @@ jobs:
CUCUMBER_TIMEOUT: 20000
CXXCOMPILER: g++-9
ENABLE_SANITIZER: ON
TARGET_ARCH: x86_64-asan
TARGET_ARCH: x86_64-asan-ubsan
OSRM_CONNECTION_RETRIES: 10
OSRM_CONNECTION_EXP_BACKOFF_COEF: 1.5

- name: clang-5.0-debug
continue-on-error: false
Expand All @@ -95,13 +97,13 @@ jobs:
CUCUMBER_TIMEOUT: 60000
ENABLE_CLANG_TIDY: ON

- name: conan-linux-debug-asan
- name: conan-linux-debug-asan-ubsan
continue-on-error: false
node: 12
runs-on: ubuntu-20.04
BUILD_TOOLS: ON
BUILD_TYPE: Release
CLANG_VERSION: 5.0.0
CLANG_VERSION: 11.0.0
ENABLE_CONAN: ON
ENABLE_SANITIZER: ON

Expand Down Expand Up @@ -382,6 +384,8 @@ jobs:
ENABLE_SANITIZER: ${{ matrix.ENABLE_SANITIZER }}
NODE_PACKAGE_TESTS_ONLY: ${{ matrix.NODE_PACKAGE_TESTS_ONLY }}
TARGET_ARCH: ${{ matrix.TARGET_ARCH }}
OSRM_CONNECTION_RETRIES: ${{ matrix.OSRM_CONNECTION_RETRIES }}
OSRM_CONNECTION_EXP_BACKOFF_COEF: ${{ matrix.OSRM_CONNECTION_EXP_BACKOFF_COEF }}
steps:
- uses: actions/checkout@v2

Expand Down Expand Up @@ -429,6 +433,7 @@ jobs:
if [[ "$ENABLE_SANITIZER" == 'ON' ]]; then
# We can only set this after checkout once we know the workspace directory
echo "LSAN_OPTIONS=print_suppressions=0:suppressions=${GITHUB_WORKSPACE}/scripts/ci/leaksanitizer.conf" >> $GITHUB_ENV
echo "UBSAN_OPTIONS=symbolize=1:halt_on_error=1:print_stacktrace=1:suppressions=${GITHUB_WORKSPACE}/scripts/ci/undefinedsanitizer.conf" >> $GITHUB_ENV
fi

if [[ "${RUNNER_OS}" == "Linux" ]]; then
Expand Down Expand Up @@ -562,6 +567,12 @@ jobs:
if: ${{ matrix.NODE_PACKAGE_TESTS_ONLY == 'ON' }}
run: |
npm run nodejs-tests
- name: Upload test logs
Copy link
Member Author

@SiarheiFedartsou SiarheiFedartsou Jul 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

uses: actions/upload-artifact@v3
if: failure()
with:
name: logs
path: test/logs/

- name: Generate code coverage
if: ${{ matrix.ENABLE_COVERAGE == 'ON' }}
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
- Misc:
- FIXED: Fix bug with reading Set values from Lua scripts. [#6285](https://github.com/Project-OSRM/osrm-backend/pull/6285)
- Build:
- CHANGED: Configure Undefined Behaviour Sanitizer. [#6290](https://github.com/Project-OSRM/osrm-backend/pull/6290)
- CHANGED: Use Conan instead of Mason to install code dependencies. [#6284](https://github.com/Project-OSRM/osrm-backend/pull/6284)
- CHANGED: Migrate to C++17. Update sol2 to 3.3.0. [#6279](https://github.com/Project-OSRM/osrm-backend/pull/6279)
- CHANGED: Update macOS CI image to macos-11. [#6286](https://github.com/Project-OSRM/osrm-backend/pull/6286)
Expand Down
11 changes: 7 additions & 4 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,11 +316,14 @@ if (ENABLE_COVERAGE)
set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -O0 -ftest-coverage -fprofile-arcs")
endif()


if (ENABLE_SANITIZER)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=address")
set(OSRM_CXXFLAGS "${OSRM_CXXFLAGS} -fsanitize=address")
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -fsanitize=address")
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -fsanitize=address")
set(SANITIZER_FLAGS "-g -fsanitize=address -fsanitize-address-use-after-scope -fsanitize=undefined -fno-omit-frame-pointer")
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${SANITIZER_FLAGS}")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${SANITIZER_FLAGS}")
set(OSRM_CXXFLAGS "${OSRM_CXXFLAGS} ${SANITIZER_FLAGS}")
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${SANITIZER_FLAGS}")
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} ${SANITIZER_FLAGS}")
endif()

# Configuring compilers
Expand Down
7 changes: 4 additions & 3 deletions features/lib/osrm_loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,12 @@ class OSRMBaseLoader{
var retryCount = 0;
let retry = (err) => {
if (err) {
if (retryCount < 10) {
if (retryCount < this.scope.OSRM_CONNECTION_RETRIES) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Added OSRM_CONNECTION_EXP_BACKOFF_COEF, please take a look.

const timeoutMs = 10 * Math.pow(this.scope.OSRM_CONNECTION_EXP_BACKOFF_COEF, retryCount);
retryCount++;
setTimeout(() => { tryConnect(this.scope.OSRM_IP, this.scope.OSRM_PORT, retry); }, 10);
setTimeout(() => { tryConnect(this.scope.OSRM_IP, this.scope.OSRM_PORT, retry); }, timeoutMs);
} else {
callback(new Error("Could not connect to osrm-routed after ten retries."));
callback(new Error(`Could not connect to osrm-routed after ${this.scope.OSRM_CONNECTION_RETRIES} retries.`));
}
}
else
Expand Down
3 changes: 3 additions & 0 deletions features/support/env.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ module.exports = function () {

this.OSRM_PORT = process.env.OSRM_PORT && parseInt(process.env.OSRM_PORT) || 5000;
this.OSRM_IP = process.env.OSRM_IP || '127.0.0.1';
this.OSRM_CONNECTION_RETRIES = process.env.OSRM_CONNECTION_RETRIES && parseInt(process.env.OSRM_CONNECTION_RETRIES) || 10;
this.OSRM_CONNECTION_EXP_BACKOFF_COEF = process.env.OSRM_CONNECTION_EXP_BACKOFF_COEF && parseFloat(process.env.OSRM_CONNECTION_EXP_BACKOFF_COEF) || 1.0;

this.HOST = `http://${this.OSRM_IP}:${this.OSRM_PORT}`;

this.OSRM_PROFILE = process.env.OSRM_PROFILE;
Expand Down
12 changes: 5 additions & 7 deletions include/contractor/contractor_config.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,8 @@ namespace contractor
struct ContractorConfig final : storage::IOConfig
{
ContractorConfig()
: IOConfig({".osrm.ebg", ".osrm.ebg_nodes", ".osrm.properties"},
{},
{".osrm.hsgr", ".osrm.enw"}),
requested_num_threads(0)
: IOConfig(
{".osrm.ebg", ".osrm.ebg_nodes", ".osrm.properties"}, {}, {".osrm.hsgr", ".osrm.enw"})
{
}

Expand All @@ -62,16 +60,16 @@ struct ContractorConfig final : storage::IOConfig
updater::UpdaterConfig updater_config;

// DEPRECATED to be removed in v6.0
bool use_cached_priority;
bool use_cached_priority = false;

unsigned requested_num_threads;
unsigned requested_num_threads = 0;

// DEPRECATED to be removed in v6.0
// A percentage of vertices that will be contracted for the hierarchy.
// Offers a trade-off between preprocessing and query time.
// The remaining vertices form the core of the hierarchy
//(e.g. 0.8 contracts 80 percent of the hierarchy, leaving a core of 20%)
double core_factor;
double core_factor = 1.0;
};
} // namespace contractor
} // namespace osrm
Expand Down
5 changes: 3 additions & 2 deletions include/engine/trip/trip_farthest_insertion.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,14 @@ GetShortestRoundTrip(const NodeID new_loc,

const auto dist_from = dist_table(*from_node, new_loc);
const auto dist_to = dist_table(new_loc, *to_node);
const auto trip_dist = dist_from + dist_to - dist_table(*from_node, *to_node);

// If the edge_weight is very large (INVALID_EDGE_WEIGHT) then the algorithm will not choose
// this edge in final minimal path. So instead of computing all the permutations after this
// large edge, discard this edge right here and don't consider the path after this edge.
if (dist_from == INVALID_EDGE_WEIGHT || dist_to == INVALID_EDGE_WEIGHT)
continue;

const auto trip_dist = dist_from + dist_to - dist_table(*from_node, *to_node);

// This is not neccessarily true:
// Lets say you have an edge (u, v) with duration 100. If you place a coordinate exactly in
// the middle of the segment yielding (u, v'), the adjusted duration will be 100 * 0.5 = 50.
Expand Down
15 changes: 6 additions & 9 deletions include/extractor/extractor_config.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,7 @@ struct ExtractorConfig final : storage::IOConfig
".osrm.icd",
".osrm.cnbg",
".osrm.cnbg_to_ebg",
".osrm.maneuver_overrides"}),
requested_num_threads(0), parse_conditionals(false), use_locations_cache(true)
".osrm.maneuver_overrides"})
{
}

Expand All @@ -84,14 +83,12 @@ struct ExtractorConfig final : storage::IOConfig
std::vector<boost::filesystem::path> location_dependent_data_paths;
std::string data_version;

unsigned requested_num_threads;
unsigned small_component_size;
unsigned requested_num_threads = 0;
unsigned small_component_size = 1000;

bool generate_edge_lookup;

bool use_metadata;
bool parse_conditionals;
bool use_locations_cache;
bool use_metadata = false;
Copy link
Member Author

Choose a reason for hiding this comment

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

Curious why usual compiler warnings didn't catch that 🤔

bool parse_conditionals = false;
bool use_locations_cache = true;
Copy link
Member Author

Choose a reason for hiding this comment

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

Values are chosen as default values we use in program_options.

};
} // namespace extractor
} // namespace osrm
Expand Down
3 changes: 3 additions & 0 deletions include/util/log.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,9 @@ class Log
return *this;
}

private:
void Init();

protected:
const LogLevel level;
std::ostringstream buffer;
Expand Down
16 changes: 16 additions & 0 deletions scripts/ci/undefinedsanitizer.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
enum:include/tbb/pipeline.h
vptr:src/util/log.cpp
vptr:include/tbb/task.h
vptr:include/tbb/parallel_reduce.h
vptr:include/boost/smart_ptr/detail/shared_count.hpp
vptr:include/boost/smart_ptr/detail/sp_counted_base_gcc_atomic.hpp
vptr:include/boost/program_options/variables_map.hpp
vptr:include/boost/program_options/value_semantic.hpp
vptr:src/tools/contract.cpp
vptr:src/tools/extract.cpp
pointer-overflow:include/partitioner/cell_storage.hpp
signed-integer-overflow:include/engine/internal_route_result.hpp
pointer-overflow:third_party/sol2/sol/stack_core.hpp
pointer-overflow:third_party/rapidjson/include/rapidjson/internal/stack.h
nonnull-attribute:third_party/microtar/src/microtar.c
integer-divide-by-zero:unit_tests/library/route.cpp
8 changes: 5 additions & 3 deletions src/util/log.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,11 @@ std::string LogPolicy::GetLevels()
return "NONE, ERROR, WARNING, INFO, DEBUG";
}

Log::Log(LogLevel level_, std::ostream &ostream) : level(level_), stream(ostream)
Log::Log(LogLevel level_, std::ostream &ostream) : level(level_), stream(ostream) { Init(); }

Log::Log(LogLevel level_) : level(level_), buffer{}, stream{buffer} { Init(); }

void Log::Init()
{
std::lock_guard<std::mutex> lock(get_mutex());
if (!LogPolicy::GetInstance().IsMute() && level <= LogPolicy::GetInstance().GetLevel())
Expand Down Expand Up @@ -91,8 +95,6 @@ Log::Log(LogLevel level_, std::ostream &ostream) : level(level_), stream(ostream
}
}

Log::Log(LogLevel level_) : Log(level_, buffer) {}
Copy link
Member Author

Choose a reason for hiding this comment

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

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


std::mutex &Log::get_mutex()
{
static std::mutex mtx;
Expand Down