fix: Fix bootloop logging crash (#1357)
## Summary * **What is the goal of this PR?** On a cold boot (or after a crash that corrupts RTC RAM), logHead contains garbage. Then addToLogRingBuffer does: ``strncpy(logMessages[logHead], message, MAX_ENTRY_LEN - 1); `` With garbage logHead, this computes a completely invalid address. The % MAX_LOG_LINES guard on line 16 only runs after the bad store, which is too late. The fix is to clamp logHead before use. ## Additional Context * Add any other information that might be helpful for the reviewer (e.g., performance implications, potential risks, specific areas to focus on). --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? _**NO**_ (did use claude for the magic hash value)
This commit is contained in:
@@ -8,9 +8,23 @@
|
||||
// Simple ring buffer log, useful for error reporting when we encounter a crash
|
||||
RTC_NOINIT_ATTR char logMessages[MAX_LOG_LINES][MAX_ENTRY_LEN];
|
||||
RTC_NOINIT_ATTR size_t logHead = 0;
|
||||
// Magic word written alongside logHead to detect uninitialized RTC memory.
|
||||
// RTC_NOINIT_ATTR is not zeroed on cold boot, so logHead may appear in-range
|
||||
// (0..MAX_LOG_LINES-1) by chance even though logMessages is garbage. The magic
|
||||
// value is only set by clearLastLogs(), so its absence means the buffer was
|
||||
// never properly initialized.
|
||||
RTC_NOINIT_ATTR uint32_t rtcLogMagic;
|
||||
static constexpr uint32_t LOG_RTC_MAGIC = 0xDEADBEEF;
|
||||
|
||||
void addToLogRingBuffer(const char* message) {
|
||||
// Add the message to the ring buffer, overwriting old messages if necessary
|
||||
// Add the message to the ring buffer, overwriting old messages if necessary.
|
||||
// If the magic is wrong or logHead is out of range (RTC_NOINIT_ATTR garbage
|
||||
// on cold boot), clear the entire buffer so subsequent reads are safe.
|
||||
if (rtcLogMagic != LOG_RTC_MAGIC || logHead >= MAX_LOG_LINES) {
|
||||
memset(logMessages, 0, sizeof(logMessages));
|
||||
logHead = 0;
|
||||
rtcLogMagic = LOG_RTC_MAGIC;
|
||||
}
|
||||
strncpy(logMessages[logHead], message, MAX_ENTRY_LEN - 1);
|
||||
logMessages[logHead][MAX_ENTRY_LEN - 1] = '\0';
|
||||
logHead = (logHead + 1) % MAX_LOG_LINES;
|
||||
@@ -63,19 +77,38 @@ void logPrintf(const char* level, const char* origin, const char* format, ...) {
|
||||
}
|
||||
|
||||
std::string getLastLogs() {
|
||||
if (rtcLogMagic != LOG_RTC_MAGIC) {
|
||||
return {};
|
||||
}
|
||||
std::string output;
|
||||
for (size_t i = 0; i < MAX_LOG_LINES; i++) {
|
||||
size_t idx = (logHead + i) % MAX_LOG_LINES;
|
||||
if (logMessages[idx][0] != '\0') {
|
||||
output += logMessages[idx];
|
||||
const size_t len = strnlen(logMessages[idx], MAX_ENTRY_LEN);
|
||||
output.append(logMessages[idx], len);
|
||||
}
|
||||
}
|
||||
return output;
|
||||
}
|
||||
|
||||
// Checks whether the RTC log state is consistent: rtcLogMagic must equal
|
||||
// LOG_RTC_MAGIC and logHead must be in 0..MAX_LOG_LINES-1. Returns true if
|
||||
// corruption is detected, in which case rtcLogMagic is still invalid and
|
||||
// logMessages may contain garbage. Callers (e.g. HalSystem::begin on the
|
||||
// panic-reboot path) must call clearLastLogs() after a true result to fully
|
||||
// reinitialize the ring buffer and stamp the magic before getLastLogs() is used.
|
||||
bool sanitizeLogHead() {
|
||||
if (rtcLogMagic != LOG_RTC_MAGIC || logHead >= MAX_LOG_LINES) {
|
||||
logHead = 0;
|
||||
return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
void clearLastLogs() {
|
||||
for (size_t i = 0; i < MAX_LOG_LINES; i++) {
|
||||
logMessages[i][0] = '\0';
|
||||
}
|
||||
logHead = 0;
|
||||
rtcLogMagic = LOG_RTC_MAGIC;
|
||||
}
|
||||
|
||||
@@ -57,6 +57,11 @@ void logPrintf(const char* level, const char* origin, const char* format, ...);
|
||||
|
||||
std::string getLastLogs();
|
||||
void clearLastLogs();
|
||||
// Validates the RTC log state (magic word + logHead range). Returns true if
|
||||
// corruption was detected (magic mismatch or logHead out of range), meaning
|
||||
// logMessages is untrusted garbage. Callers should call clearLastLogs() when
|
||||
// this returns true so getLastLogs() does not dump corrupt data into crash reports.
|
||||
bool sanitizeLogHead();
|
||||
|
||||
class MySerialImpl : public Print {
|
||||
public:
|
||||
|
||||
@@ -76,6 +76,14 @@ void begin() {
|
||||
// `clearPanic()` to clear it after dumping
|
||||
if (!isRebootFromPanic()) {
|
||||
clearPanic();
|
||||
} else {
|
||||
// Panic reboot: preserve logs and panic info, but clamp logHead in case the
|
||||
// panic occurred before begin() ever ran (e.g. in a static constructor).
|
||||
// If logHead was out of range, logMessages is also garbage — clear it so
|
||||
// getLastLogs() does not dump corrupt data into the crash report.
|
||||
if (sanitizeLogHead()) {
|
||||
clearLastLogs();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user