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 <makenne@microsoft.com>

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 <int /*ignored*/>
 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<float> 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");
 	}
 }
 

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);

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

unit_tests/tests/test_hip9011.cpp

@@ -6,7 +6,6 @@
 
 #include "pch.h"
 #include "hip9011_logic.h"
-#include "test_parameters.h"
 
 using ::testing::_;
 

unit_tests/tests/test_on_demand_parameters.cpp

@@ -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<std::string, float>::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);
-	}
-}
-
-

unit_tests/tests/test_parameters.h

@@ -1,19 +0,0 @@
-/*
- * test_parameters.h
- *
- *  Created on: Jan 17, 2019
- * @author Andrey Belomutskiy, (c) 2012-2020
- */
-
-#pragma once
-
-#include <unordered_map>
-using namespace std;
-
-class TestParameters {
-public:
-	unordered_map<string, float> values;
-	TestParameters* put(string key, float value);
-	float get(string key);
-};
-

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);
 }
-
-
-
-

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 \

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<std::shared_ptr<TriggerCallback>>& 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<TriggerCallback> param = std::make_shared<TriggerCallback>();
+			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", &param->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<std::shared_ptr<TriggerCallback>> 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);
 	}
 

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