From c72bd0717928513bbe3ee003990631d78bef3b2b Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Sun, 5 Sep 2021 02:56:59 -0700 Subject: [PATCH] enable address sanitizer (#3217) * enable asan * lua * this is dead and leaky * leaky nissan * fix pwm generator use-after-stack * initializers * prevent problems in case of invalid trigger shape Co-authored-by: Matthew Kennedy --- firmware/controllers/lua/lua.cpp | 25 ++++++--- .../trigger/decoders/trigger_structure.cpp | 5 ++ .../trigger/decoders/trigger_structure.h | 6 +- unit_tests/tests/test_hip9011.cpp | 1 - .../tests/test_on_demand_parameters.cpp | 56 ------------------- unit_tests/tests/test_parameters.h | 19 ------- unit_tests/tests/test_pwm_generator.cpp | 21 +++---- unit_tests/tests/tests.mk | 1 - .../tests/trigger/test_nissan_vq_vvt.cpp | 23 +++++--- unit_tests/unit_test_rules.mk | 4 +- 10 files changed, 52 insertions(+), 109 deletions(-) delete mode 100644 unit_tests/tests/test_on_demand_parameters.cpp delete mode 100644 unit_tests/tests/test_parameters.h diff --git a/firmware/controllers/lua/lua.cpp b/firmware/controllers/lua/lua.cpp index 450201ae2d..0f92f4c4f1 100644 --- a/firmware/controllers/lua/lua.cpp +++ b/firmware/controllers/lua/lua.cpp @@ -93,6 +93,15 @@ static void* myAlloc(void* /*ud*/, void* ptr, size_t osize, size_t nsize) { // Non-MCU code can use plain realloc function instead of custom implementation template static void* myAlloc(void* /*ud*/, void* ptr, size_t /*osize*/, size_t nsize) { + if (!nsize) { + free(ptr); + return nullptr; + } + + if (!ptr) { + return malloc(nsize); + } + return realloc(ptr, nsize); } #endif // EFI_PROD_CODE @@ -375,23 +384,23 @@ static LuaHandle runScript(const char* script) { auto ls = setupLuaState(myAlloc<0>); if (!ls) { - throw new std::logic_error("Call to setupLuaState failed, returned null"); + throw std::logic_error("Call to setupLuaState failed, returned null"); } if (!loadScript(ls, script)) { - throw new std::logic_error("Call to loadScript failed"); + throw std::logic_error("Call to loadScript failed"); } lua_getglobal(ls, "testFunc"); if (lua_isnil(ls, -1)) { - throw new std::logic_error("Failed to find function testFunc"); + throw std::logic_error("Failed to find function testFunc"); } int status = lua_pcall(ls, 0, 1, 0); if (0 != status) { std::string msg = std::string("lua error while running script: ") + lua_tostring(ls, -1); - throw new std::logic_error(msg); + throw std::logic_error(msg); } return ls; @@ -407,7 +416,7 @@ expected testLuaReturnsNumberOrNil(const char* script) { // If not nil, it should be a number if (!lua_isnumber(ls, -1)) { - throw new std::logic_error("Returned value is not a number"); + throw std::logic_error("Returned value is not a number"); } // pop the return value @@ -431,7 +440,7 @@ int testLuaReturnsInteger(const char* script) { // pop the return value; if (!lua_isinteger(ls, -1)) { - throw new std::logic_error("Returned value is not an integer"); + throw std::logic_error("Returned value is not an integer"); } return lua_tointeger(ls, -1); @@ -441,11 +450,11 @@ void testLuaExecString(const char* script) { auto ls = setupLuaState(myAlloc<0>); if (!ls) { - throw new std::logic_error("Call to setupLuaState failed, returned null"); + throw std::logic_error("Call to setupLuaState failed, returned null"); } if (!loadScript(ls, script)) { - throw new std::logic_error("Call to loadScript failed"); + throw std::logic_error("Call to loadScript failed"); } } diff --git a/firmware/controllers/trigger/decoders/trigger_structure.cpp b/firmware/controllers/trigger/decoders/trigger_structure.cpp index 36011f961d..d0a8e6cc5b 100644 --- a/firmware/controllers/trigger/decoders/trigger_structure.cpp +++ b/firmware/controllers/trigger/decoders/trigger_structure.cpp @@ -437,6 +437,11 @@ void findTriggerPosition(TriggerWaveform *triggerShape, void TriggerWaveform::prepareShape(TriggerFormDetails *details DECLARE_ENGINE_PARAMETER_SUFFIX) { #if EFI_ENGINE_CONTROL && EFI_SHAFT_POSITION_INPUT + if (shapeDefinitionError) { + // Nothing to do here if there's a problem with the trigger shape + return; + } + prepareEventAngles(this, details PASS_ENGINE_PARAMETER_SUFFIX); int engineCycleInt = (int) getEngineCycle(operationMode); diff --git a/firmware/controllers/trigger/decoders/trigger_structure.h b/firmware/controllers/trigger/decoders/trigger_structure.h index 835aafa87f..7e2eb3715e 100644 --- a/firmware/controllers/trigger/decoders/trigger_structure.h +++ b/firmware/controllers/trigger/decoders/trigger_structure.h @@ -105,11 +105,11 @@ public: * this flag tells us if we should ignore events on second input channel * that's the way to ignore noise from the disconnected wire */ - bool needSecondTriggerInput; + bool needSecondTriggerInput = false; /** * true value here means that we do not have a valid trigger configuration */ - bool shapeDefinitionError; + bool shapeDefinitionError = false; /** * https://github.com/rusefi/rusefi/issues/898 @@ -119,7 +119,7 @@ public: * * One day a nicer implementation could be simply ignoring 'useOnlyRisingEdgeForTrigger' in case of 'bothFrontsRequired' */ - bool bothFrontsRequired; + bool bothFrontsRequired = false; /** * this variable is incremented after each trigger shape redefinition diff --git a/unit_tests/tests/test_hip9011.cpp b/unit_tests/tests/test_hip9011.cpp index 08b8c97b43..41330385fa 100644 --- a/unit_tests/tests/test_hip9011.cpp +++ b/unit_tests/tests/test_hip9011.cpp @@ -6,7 +6,6 @@ #include "pch.h" #include "hip9011_logic.h" -#include "test_parameters.h" using ::testing::_; diff --git a/unit_tests/tests/test_on_demand_parameters.cpp b/unit_tests/tests/test_on_demand_parameters.cpp deleted file mode 100644 index 3a29c4c81a..0000000000 --- a/unit_tests/tests/test_on_demand_parameters.cpp +++ /dev/null @@ -1,56 +0,0 @@ -/* - * @file test_on_demand_parameters.cpp - * - * Created on: Jan 16, 2019 - * @author Andrey Belomutskiy, (c) 2012-2020 - */ - -#include "pch.h" -#include "test_parameters.h" - -TestParameters* TestParameters::put(string key, float value) { - values[key] = value; - return this; -} - -float TestParameters::get(string key) { - // WAT? 'contains' method only defined in C++20?! - std::unordered_map::const_iterator got = values.find(key); - if (got == values.end()) - throw "No value for this key: " + key; - return values[key]; -} - -#undef METHOD_SIGNATURE -#define METHOD_SIGNATURE TestParameters *parameterValues - -#define GET_VALUE(x) parameterValues->get(#x) - -static float methodWeWantToTestWhichUsesKey1AndKey2(METHOD_SIGNATURE) { - return GET_VALUE(key1) + GET_VALUE(key2); -} - -static float methodWeWantToTestWhichUsesKey3(METHOD_SIGNATURE) { - return GET_VALUE(key3); -} - -TEST(util, readValues) { - TestParameters* v = (new TestParameters())->put("key2", 20)->put("key1", 10); - - ASSERT_FLOAT_EQ(30, methodWeWantToTestWhichUsesKey1AndKey2(v)); -} - -TEST(util, checkForMissingParameterHandling) { - TestParameters* v = (new TestParameters())->put("key2", 2); - try { - methodWeWantToTestWhichUsesKey3(v); - FAIL() << "Expected 'missing key3' exception"; - } catch(string message) { - // exception about missing value is expected - // type limits this to always be not negative - // todo? do we need this? ASSERT_TRUE(message.find("No value for this key") >= 0); - // todo? do we need this? ASSERT_TRUE(message.find("key3") >= 0); - } -} - - diff --git a/unit_tests/tests/test_parameters.h b/unit_tests/tests/test_parameters.h deleted file mode 100644 index 71b854c08b..0000000000 --- a/unit_tests/tests/test_parameters.h +++ /dev/null @@ -1,19 +0,0 @@ -/* - * test_parameters.h - * - * Created on: Jan 17, 2019 - * @author Andrey Belomutskiy, (c) 2012-2020 - */ - -#pragma once - -#include -using namespace std; - -class TestParameters { -public: - unordered_map values; - TestParameters* put(string key, float value); - float get(string key); -}; - diff --git a/unit_tests/tests/test_pwm_generator.cpp b/unit_tests/tests/test_pwm_generator.cpp index 5c96455e5f..b521d06dc2 100644 --- a/unit_tests/tests/test_pwm_generator.cpp +++ b/unit_tests/tests/test_pwm_generator.cpp @@ -34,9 +34,10 @@ static void test100dutyCycle() { printf("*************************************** test100dutyCycle\r\n"); expectedTimeOfNextEvent = timeNowUs = 0; - TestExecutor executor; - SimplePwm pwm("test PWM1"); + OutputPin pin; + SimplePwm pwm("test PWM1"); + TestExecutor executor; startSimplePwm(&pwm, "unit_test", &executor, @@ -60,9 +61,10 @@ static void testSwitchToNanPeriod() { printf("*************************************** testSwitchToNanPeriod\r\n"); expectedTimeOfNextEvent = timeNowUs = 0; - TestExecutor executor; - SimplePwm pwm("test PWM1"); + OutputPin pin; + SimplePwm pwm("test PWM1"); + TestExecutor executor; startSimplePwm(&pwm, "unit_test", &executor, @@ -95,9 +97,10 @@ TEST(misc, testPwmGenerator) { testSwitchToNanPeriod(); expectedTimeOfNextEvent = timeNowUs = 0; - TestExecutor executor; - SimplePwm pwm("test PWM3"); + OutputPin pin; + SimplePwm pwm("test PWM3"); + TestExecutor executor; startSimplePwm(&pwm, "unit_test", @@ -106,7 +109,6 @@ TEST(misc, testPwmGenerator) { 1000 /* frequency */, 0.80 /* duty cycle */); - expectedTimeOfNextEvent += 800; assertEqualsM2("1@1000/80", expectedTimeOfNextEvent, executor.getForUnitTest(0)->momentX, 0); @@ -145,10 +147,5 @@ TEST(misc, testPwmGenerator) { ASSERT_EQ( 5000, timeNowUs) << "time4"; assertEqualsM2("7@1000/0", expectedTimeOfNextEvent, executor.getForUnitTest(0)->momentX, 0); - assertNextEvent("exec@6", LOW_VALUE /* pin value */, &executor, pin); } - - - - diff --git a/unit_tests/tests/tests.mk b/unit_tests/tests/tests.mk index d5451e1824..f612f41075 100644 --- a/unit_tests/tests/tests.mk +++ b/unit_tests/tests/tests.mk @@ -34,7 +34,6 @@ TESTS_SRC_CPP = \ tests/test_hardware_reinit.cpp \ tests/test_ion.cpp \ tests/test_aux_valves.cpp \ - tests/test_on_demand_parameters.cpp \ tests/test_hip9011.cpp \ tests/test_engine_math.cpp \ tests/test_fasterEngineSpinningUp.cpp \ diff --git a/unit_tests/tests/trigger/test_nissan_vq_vvt.cpp b/unit_tests/tests/trigger/test_nissan_vq_vvt.cpp index 6cc3776823..f3929973fb 100644 --- a/unit_tests/tests/trigger/test_nissan_vq_vvt.cpp +++ b/unit_tests/tests/trigger/test_nissan_vq_vvt.cpp @@ -16,6 +16,7 @@ public: TriggerWaveform *form; bool isVvt; int vvtBankIndex; + scheduling_s sched; }; static void func(TriggerCallback *callback) { @@ -39,7 +40,8 @@ static void scheduleTriggerEvents(TriggerWaveform *shape, int count, bool isVvt, int vvtBankIndex, - int vvtOffset + int vvtOffset, + std::vector>& ptrs DECLARE_ENGINE_PARAMETER_SUFFIX) { int totalIndex = 0; @@ -50,15 +52,17 @@ static void scheduleTriggerEvents(TriggerWaveform *shape, for (int r = 0; r < count; r++) { for (size_t i = 0; i < shape->getSize(); i++) { float angle = vvtOffset + shape->getAngle(totalIndex); - TriggerCallback *param = new TriggerCallback(); + + std::shared_ptr param = std::make_shared(); + ptrs.push_back(param); + param->engine = engine; param->toothIndex = totalIndex; param->form = shape; param->isVvt = isVvt; param->vvtBankIndex = vvtBankIndex; - scheduling_s *sch = new scheduling_s(); - engine->executor.scheduleByTimestamp("test", sch, timeScale * 1000 * angle, { func, param }); + engine->executor.scheduleByTimestamp("test", ¶m->sched, timeScale * 1000 * angle, { func, param.get() }); totalIndex++; } } @@ -66,6 +70,9 @@ static void scheduleTriggerEvents(TriggerWaveform *shape, TEST(nissan, vq_vvt) { + // hold a reference to the heap allocated scheduling events until the test is done + std::vector> ptrs; + WITH_ENGINE_TEST_HELPER (HELLEN_121_NISSAN_6_CYL); engineConfiguration->isIgnitionEnabled = false; engineConfiguration->isInjectionEnabled = false; @@ -78,7 +85,7 @@ TEST(nissan, vq_vvt) { scheduleTriggerEvents(&crank, /* timeScale */ 1, - cyclesCount, false, -1, 0 PASS_ENGINE_PARAMETER_SUFFIX); + cyclesCount, false, -1, 0, ptrs PASS_ENGINE_PARAMETER_SUFFIX); } float vvtTimeScale = 1; @@ -92,7 +99,8 @@ TEST(nissan, vq_vvt) { /* timeScale */ vvtTimeScale, cyclesCount / 6, true, /* vvtBankIndex */ 0, - /* vvtOffset */ testVvtOffset + /* vvtOffset */ testVvtOffset, + ptrs PASS_ENGINE_PARAMETER_SUFFIX); } @@ -104,7 +112,8 @@ TEST(nissan, vq_vvt) { /* timeScale */ vvtTimeScale, cyclesCount / 6, true, /* vvtBankIndex */1, - /* vvtOffset */ testVvtOffset + NISSAN_VQ_CAM_OFFSET + /* vvtOffset */ testVvtOffset + NISSAN_VQ_CAM_OFFSET, + ptrs PASS_ENGINE_PARAMETER_SUFFIX); } diff --git a/unit_tests/unit_test_rules.mk b/unit_tests/unit_test_rules.mk index ad57a5f1b9..1b6ab223bf 100644 --- a/unit_tests/unit_test_rules.mk +++ b/unit_tests/unit_test_rules.mk @@ -28,7 +28,7 @@ endif #USE_OPT += -Wno-error=format= -Wno-error=register -Wno-error=write-strings # See explanation in main firmware Makefile for these three defines -USE_OPT += -DEFI_UNIT_TEST=1 -DEFI_PROD_CODE=0 -DEFI_SIMULATOR=0 +USE_OPT += -DEFI_UNIT_TEST=1 -DEFI_PROD_CODE=0 -DEFI_SIMULATOR=0 -fsanitize=address # Pretend we are all different hardware so that all canned engine configs are included USE_OPT += -DHW_MICRO_RUSEFI=1 -DHW_PROTEUS=1 -DHW_FRANKENSO=1 -DHW_HELLEN=1 @@ -175,7 +175,7 @@ ULIBDIR = # List all user libraries here ULIBS = -lm -ULIBS += --coverage +ULIBS += --coverage -fsanitize=address # # End of user defines