fix error/warning buffer overflow (#2456)

* fix error printing overflow

* make the worst offender shorter

* error as warning

* these don't need that flag set

firmware/config/boards/prometheus/efifeatures.h

@@ -114,7 +114,5 @@
 #define RPM_LOW_THRESHOLD 8				// RPM=8 is an empirical lower sensitivity threshold of MAX9926 for 60-2
 #define NO_RPM_EVENTS_TIMEOUT_SECS 5	// (RPM < 12)
 
-#define EFI_PRINT_ERRORS_AS_WARNINGS TRUE
-
 #define EFI_NARROW_EGO_AVERAGING TRUE
 

firmware/config/boards/skeleton/efifeatures.h

@@ -329,7 +329,6 @@
 
 // Enable file logging (like SD card) logic
 #define EFI_FILE_LOGGING FALSE
-#define EFI_PRINT_ERRORS_AS_WARNINGS TRUE
 
 #define EFI_USB_SERIAL TRUE
 

firmware/config/boards/subaru_eg33/efifeatures.h

@@ -141,8 +141,6 @@
 #define RPM_LOW_THRESHOLD 			8				// RPM=8 is an empirical lower sensitivity threshold of MAX9926 for 60-2
 #define NO_RPM_EVENTS_TIMEOUT_SECS	5	// (RPM < 12)
 
-#define EFI_PRINT_ERRORS_AS_WARNINGS	TRUE
-
 #define EFI_NARROW_EGO_AVERAGING		TRUE
 
 #endif /* EFIFEATURES_SUBARUEG33_H_ */

firmware/console/binary/tunerstudio.cpp

@@ -868,7 +868,7 @@ int TunerStudioBase::handleCrcCommand(TsChannelBase* tsChannel, char *data, int
 		break;
 #endif /* ENABLE_PERF_TRACE */
 	case TS_GET_CONFIG_ERROR: {
-		char * configError = getFirmwareError();
+		const char* configError = getFirmwareError();
 #if HW_CHECK_MODE
 		// analog input errors are returned as firmware error in QC mode
 		if (!hasFirmwareError()) {

firmware/controllers/core/error_handling.cpp

@@ -9,34 +9,9 @@
 #include "os_access.h"
 #include "perf_trace.h"
 
-static char warningBuffer[ERROR_BUFFER_SIZE];
+static critical_msg_t warningBuffer;
 static critical_msg_t criticalErrorMessageBuffer;
 
-#if EFI_SIMULATOR || EFI_PROD_CODE
-//todo: move into simulator global
-#include "memstreams.h"
-class ErrorState {
-public:
-	/**
-	 * Class constructors are a great way to have simple initialization sequence
-	 */
-	ErrorState();
-	MemoryStream warningStream;
-	MemoryStream firmwareErrorMessageStream;
-};
-
-ErrorState::ErrorState() {
-	/**
-	 * these methods only change RAM state of data structures without any HAL access thus safe in contructor
-	 */
-	msObjectInit(&warningStream, (uint8_t *) warningBuffer, ERROR_BUFFER_SIZE, 0);
-	msObjectInit(&firmwareErrorMessageStream, criticalErrorMessageBuffer, sizeof(criticalErrorMessageBuffer), 0);
-}
-
-static ErrorState errorState;
-
-#endif /* EFI_SIMULATOR || EFI_PROD_CODE */
-
 #if EFI_HD44780_LCD
 #include "lcd_HD44780.h"
 #endif /* EFI_HD44780_LCD */
@@ -57,8 +32,8 @@ int dbg_panic_line;
 extern persistent_config_s configWorkingCopy;
 #endif
 
-char *getFirmwareError(void) {
+const char* getFirmwareError(void) {
-	return (char*) criticalErrorMessageBuffer;
+	return criticalErrorMessageBuffer;
 }
 
 #if EFI_PROD_CODE
@@ -106,33 +81,6 @@ void chDbgPanic3(const char *msg, const char * file, int line) {
 	}
 }
 
-// todo: look into chsnprintf
-// todo: move to some util file & reuse for 'firmwareError' method
-static void printToStream(MemoryStream *stream, const char *fmt, va_list ap) {
-	stream->eos = 0; // reset
-	chvprintf((BaseSequentialStream *) stream, fmt, ap);
-
-	// Terminate, but don't write past the end of the buffer
-	int terminatorLocation = minI(stream->eos, stream->size - 1);
-	stream->buffer[terminatorLocation] = '\0';
-}
-
-static void printWarning(const char *fmt, va_list ap) {
-	printToStream(&errorState.warningStream, fmt, ap);
-
-	if (CONFIG(showHumanReadableWarning)) {
-#if EFI_TUNER_STUDIO
- #if defined(EFI_NO_CONFIG_WORKING_COPY)
-  memcpy(persistentState.persistentConfiguration.warning_message, warningBuffer, sizeof(warningBuffer));
- #else /* defined(EFI_NO_CONFIG_WORKING_COPY) */
-  memcpy(configWorkingCopy.warning_message, warningBuffer, sizeof(warningBuffer));
- #endif /* defined(EFI_NO_CONFIG_WORKING_COPY) */
-#endif /* EFI_TUNER_STUDIO */
-	}
-
-	scheduleMsg(&logger, "WARNING: %s", warningBuffer);
-}
-
 #else
 WarningCodeState unitTestWarningCodeState;
 
@@ -162,8 +110,20 @@ bool warning(obd_code_e code, const char *fmt, ...) {
 
 	va_list ap;
 	va_start(ap, fmt);
-	printWarning(fmt, ap);
+	chvsnprintf(warningBuffer, sizeof(warningBuffer), fmt, ap);
 	va_end(ap);
+
+	if (CONFIG(showHumanReadableWarning)) {
+#if EFI_TUNER_STUDIO
+ #if defined(EFI_NO_CONFIG_WORKING_COPY)
+  memcpy(persistentState.persistentConfiguration.warning_message, warningBuffer, sizeof(warningBuffer));
+ #else /* defined(EFI_NO_CONFIG_WORKING_COPY) */
+  memcpy(configWorkingCopy.warning_message, warningBuffer, sizeof(warningBuffer));
+ #endif /* defined(EFI_NO_CONFIG_WORKING_COPY) */
+#endif /* EFI_TUNER_STUDIO */
+	}
+
+	scheduleMsg(&logger, "WARNING: %s", warningBuffer);
 #else
 	// todo: we need access to 'engine' here so that we can migrate to real 'engine->engineState.warnings'
 	unitTestWarningCodeState.addWarningCode(code);
@@ -178,7 +138,7 @@ bool warning(obd_code_e code, const char *fmt, ...) {
 	return false;
 }
 
-char *getWarningMessage(void) {
+const char* getWarningMessage(void) {
 	return warningBuffer;
 }
 
@@ -241,7 +201,7 @@ void firmwareError(obd_code_e code, const char *fmt, ...) {
 #ifdef EFI_PRINT_ERRORS_AS_WARNINGS
 	va_list ap;
 	va_start(ap, fmt);
-	printWarning(fmt, ap);
+	chvsnprintf(warningBuffer, sizeof(warningBuffer), fmt, ap);
 	va_end(ap);
 #endif
 	ON_CRITICAL_ERROR()
@@ -250,20 +210,17 @@ void firmwareError(obd_code_e code, const char *fmt, ...) {
 	if (indexOf(fmt, '%') == -1) {
 		/**
 		 * in case of simple error message let's reduce stack usage
-		 * because chvprintf might be causing an error
+		 * chvsnprintf could cause an overflow if we're already low
 		 */
 		strncpy((char*) criticalErrorMessageBuffer, fmt, sizeof(criticalErrorMessageBuffer) - 1);
 		criticalErrorMessageBuffer[sizeof(criticalErrorMessageBuffer) - 1] = 0; // just to be sure
 	} else {
-		// todo: look into chsnprintf once on Chibios 3
-		errorState.firmwareErrorMessageStream.eos = 0; // reset
 		va_list ap;
 		va_start(ap, fmt);
-		chvprintf((BaseSequentialStream *) &errorState.firmwareErrorMessageStream, fmt, ap);
+		chvsnprintf(criticalErrorMessageBuffer, sizeof(criticalErrorMessageBuffer), fmt, ap);
 		va_end(ap);
-		// todo: reuse warning buffer helper method
-		errorState.firmwareErrorMessageStream.buffer[errorState.firmwareErrorMessageStream.eos] = 0; // need to terminate explicitly
 	}
+
 	int size = strlen((char*)criticalErrorMessageBuffer);
 	static char versionBuffer[32];
 	chsnprintf(versionBuffer, sizeof(versionBuffer), " %d@%s", getRusEfiVersion(), FIRMWARE_ID);

firmware/controllers/core/error_handling.h

@@ -23,7 +23,8 @@ extern "C"
  */
 bool warning(obd_code_e code, const char *fmt, ...);
 
-typedef uint8_t critical_msg_t[ERROR_BUFFER_SIZE];
+typedef char critical_msg_t[ERROR_BUFFER_SIZE];
+
 /**
  * Something really bad had happened - firmware cannot function, we cannot run the engine
  * We definitely use this critical error approach in case of invalid configuration. If user sets a self-contradicting
@@ -38,10 +39,8 @@ extern bool hasFirmwareErrorFlag;
 #define hasFirmwareError() hasFirmwareErrorFlag
 
 // todo: rename to getCriticalErrorMessage
-char *getFirmwareError(void);
+const char* getFirmwareError(void);
-
+const char* getWarningMessage(void);
-// todo: rename to getWarningMessage?
-char *getWarningMessage(void);
 
 // todo: better place for this shared declaration?
 int getRusEfiVersion(void);

firmware/init/sensor/init_tps.cpp

@@ -53,7 +53,7 @@ static bool configureTps(LinearFunc& func, adc_channel_e channel, float closed,
 
 	// If the voltage for closed vs. open is very near, something is wrong with your calibration
 	if (split < 0.5f) {
-		firmwareError(OBD_Throttle_Position_Sensor_Circuit_Malfunction, "Sensor \"%s\" problem: open %f/closed %f calibration values are too close together.  Please check your wiring!", msg,
+		firmwareError(OBD_Throttle_Position_Sensor_Circuit_Malfunction, "\"%s\" problem: open %.2f/closed %.2f cal values are too close together. Check your wiring!", msg,
 				open,
 				closed);
 		return false;