[unified_analytics] Guard logFileStats against oversized or corrupted log files - #2575
[unified_analytics] Guard logFileStats against oversized or corrupted log files#2575hkarmoush wants to merge 1 commit into
Conversation
… log files LogHandler.logFileStats() read the entire log file into memory with no size check and no exception handling, unlike LogHandler.save() which already guards against files over kMaxLogFileSize. An abnormally large or corrupted telemetry log (e.g. invalid UTF-8) could exhaust the heap or throw an uncaught FileSystemException, as seen in flutter/flutter#150137. Fixes dart-lang#275
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates LogHandler.logFileStats to avoid reading log files into memory if they exceed kMaxLogFileSize and gracefully handles FileSystemExceptions by logging them as analytics exceptions. Feedback on the changes suggests that if the log file does not exist, statSync().size returns -1, which would bypass the size check and throw an expected FileSystemException that gets incorrectly logged as an error. A code suggestion is provided to handle this case by returning null when size < 0.
| // Avoid reading abnormally large (or corrupted) log files into | ||
| // memory, which can exhaust the heap; see | ||
| // https://github.com/dart-lang/tools/issues/275. | ||
| if (logFile.statSync().size > kMaxLogFileSize) return null; |
There was a problem hiding this comment.
If the log file does not exist (which is a common and expected state, e.g., when a user is opted out or before any events are logged), logFile.statSync().size returns -1 per the Dart SDK specification.
Currently, the code only checks if the size is greater than kMaxLogFileSize. Since -1 is not greater, it proceeds to call logFile.readAsLinesSync(), which throws a FileSystemException (file not found). This exception is then caught and logged as an analyticsException in errorSet, leading to false-positive error reports being sent to Google Analytics.
Checking if size < 0 allows us to safely and quietly return null when the file does not exist, avoiding unnecessary exceptions and false-positive error reports. This approach is also fully compatible with the existing _FakeFile and _FakeFileStat test doubles since they already implement size.
| if (logFile.statSync().size > kMaxLogFileSize) return null; | |
| final size = logFile.statSync().size; | |
| if (size < 0 || size > kMaxLogFileSize) return null; |
| Event.analyticsException( | ||
| workflow: 'LogHandler.logFileStats', | ||
| error: err.runtimeType.toString(), | ||
| description: 'message: ${err.message}\npath: ${err.path}', |
There was a problem hiding this comment.
The file path can't be sent. It might include PII or proprietary information.
| // Avoid reading abnormally large (or corrupted) log files into | ||
| // memory, which can exhaust the heap; see | ||
| // https://github.com/dart-lang/tools/issues/275. | ||
| if (logFile.statSync().size > kMaxLogFileSize) return null; |
2 participants
Summary
Fixes #275.
LogHandler.logFileStats()read the entire telemetry log file into memory viareadAsLinesSync()with no size check and no exception handling. This is the same fileLogHandler.save()already guards against viakMaxLogFileSize, butlogFileStats()had no such protection.An abnormally large or corrupted log file (see the crash report linked from the issue, flutter/flutter#150137) could cause an OOM ("Exhausted heap space") or an uncaught
FileSystemException(e.g. invalid UTF-8 during decode) when read this way.This PR:
nullfromlogFileStats()without reading the file whenlogFile.statSync().size > kMaxLogFileSize, mirroring the existing guard insave().try/on FileSystemExceptionand records anEvent.analyticsExceptioninerrorSet, following the existing pattern already used in this method for per-recordFormatException/TypeErrorhandling, so the failure remains observable via the package's existing error-reporting mechanism.logFileStats()already returnsnullin other failure modes (empty file, all-malformed records), so this doesn't introduce a new contract surprise for callers —AnalyticsImpl.fetchAvailableSurveys()and the publicAnalytics.logFileStats()API already null-check the result.Test plan
readAsLinesSync()(using the existing_FakeFiletest double already used for the equivalentsave()tests).FileSystemExceptionthrown during read is caught,logFileStats()returnsnull, and the expectedEvent.analyticsExceptionis recorded inerrorSet.dart test— all 186 tests pass.dart analyze— no issues.dart format— no changes needed.