| Audited | JakeWharton/timber at commit 988c9a5917f11e10543df77c18ca3cb754ce9292 |
|---|---|
| Date | 11 October 2026 |
| How it ran | API run on the Nacodex server, full audit, Standard review |
| Verdict after review | Pass with notes (rule: Fail if a High finding remains after review, otherwise Pass with notes) |
Each finding keeps the number it has in the audit report. The rating shown first is the one after review; the first automated rating is listed with it. 7 findings were first rated Medium or High; 1 of them are Medium or High after review. Text marked "From the report" is quoted from the audit report; fixes are suggestions and were not tested. Code locations link to the file and line at the audited commit.
Medium after review (1)
.github/workflows/release.yaml:23From the report
- run: ./gradlew -p mosaic publishsettings.gradle includes only :timber, :timber-lint and :timber-sample. We searched the repository and found no mosaic directory. The line looks copied from another project. Every tag push therefore fails at this step: nothing reaches Maven Central, and the GitHub release and documentation deploy steps after it never run.
Use ./gradlew publish from the repository root, preceded by ./gradlew build (see finding 9).
Review: release.yaml:23 - run: ./gradlew -p mosaic publish. settings.gradle includes only :timber, :timber-lint, :timber-sample; the repo root has no mosaic directory; a recursive grep finds "mosaic" only in release.yaml. gradlew -p <missing dir> errors out (standard Gradle behaviour, not run here), so steps :29-48 (release notes, GitHub release, docs deploy) never run on a tag. Current trunk (fetched through the contents API on 2026-10-11) still contains the same line, so it is not fixed upstream. Severity: breaks the project's CI release path but ships nothing wrong to users, and RELEASING.md describes a manual release path; reviewed MEDIUM-at-most (LOW-MEDIUM). CI file, not library code.
Upstream: No matching upstream report found (11 October 2026).
Low after review (8)
timber-lint/src/main/java/timber/lint/WrongTimberUsageDetector.kt:413 and :453From the report
private fun getStringArgumentTypes(formatString: String): List<String> {
...
private fun getFormatArgumentCount(s: String): Int {Both functions repeat the same matcher loop, including the backslash-skip and %%/%n handling. Only one of them honours explicit argument indexes (%2$s), and that mismatch causes the false positives in finding 17.
Write one tokenizer that returns (argument index, conversion) pairs, and derive both the count and the type list from it.
Review: :413 getStringArgumentTypes and :453 getFormatArgumentCount repeat the matcher/backslash-skip loop. Only getFormatArgumentCount reads group(1) (explicit index, :480-485); getStringArgumentTypes does not. This is the root cause of finding 17. Correct.
.github/workflows/release.yaml:23From the report
- run: ./gradlew -p mosaic publishThe release job goes straight to publish. The regular build workflow ignores tags (tags-ignore: '**'), so the commit that becomes a release is never built, tested, API-checked or linted by the workflow that ships it.
Run ./gradlew build in the release job before publishing.
Review: release.yaml has checkout, setup-java, setup-gradle, then ./gradlew -p mosaic publish (:23); no build/test step. build.yaml:9-10 tags-ignore: '**' confirmed. Nuance: build.yaml does run on trunk pushes (:6-8), so the commit is normally built, but concurrently and not as a gate. Fair LOW.
RELEASING.md:21From the report
5. Manually release and upload artifacts
1. Run `./gradlew clean publish`
2. Visit [Sonatype Nexus](https://oss.sonatype.org/) and promote the artifact.gradle.properties enables automatic Maven Central publishing, and the tag workflow publishes again on push. A maintainer who follows the document publishes the version manually, then pushes the tag. CI then tries to publish the same version a second time, which Central rejects. The Nexus promotion step refers to an old publishing path. Step 3.3 also refers to a Kotlin compatibility table that the README does not contain.
Rewrite the procedure around the CI flow: bump the version, update the changelog, tag and push. Remove the manual publish and Nexus steps, and the stale reference to the compatibility table.
Review: RELEASING.md:21-24 as quoted; gradle.properties:23 mavenCentralAutomaticPublishing=true; RELEASING.md:13 "Update the Kotlin version compatibility table" while README.md has no Kotlin/compat table (grep of README: no "compat"/"kotlin"). The oss.sonatype.org promote step is obsolete under automatic Central publishing. All factual points hold, but this is a stale document, with no code or build effect; a double publish only happens if a maintainer follows step 5 literally and CI (finding 12) were working. Fair LOW.
Upstream: No matching upstream report found (11 October 2026).
From the report
val end = Math.min(newline, i + MAX_LOG_LENGTH)
val part = message.substring(i, end)
Log.println(priority, tag, part)MAX_LOG_LENGTH = 4000 counts UTF-16 characters. The Android logger's limit is about 4 KB of encoded bytes. A 4,000-character chunk of CJK text or emoji can be two to three times that size, and the system cuts it off silently, so part of the log is lost. A split can also land between the two halves of a surrogate pair, which garbles a character at the chunk boundary.
Chunk by UTF-8 byte length, or use a conservative character limit. Never split inside a surrogate pair.
Review: Timber.kt:272 MAX_LOG_LENGTH = 4000 (UTF-16 units), :242 if (message.length < MAX_LOG_LENGTH) short path, :258-259 val end = Math.min(newline, i + MAX_LOG_LENGTH) / message.substring(i, end). Defect is real: Android's per-entry limit is in bytes (~4 KB), so 4000 chars of CJK/Cyrillic/emoji exceed it. Two points the report misses or misstates: the short path (:242) is affected too (a 2,000-char CJK message is never split but still exceeds the limit), and a substring can split a surrogate pair (as the report says). Device truncation itself was not run (CANNOT-VERIFY on device; the byte limit comes from the upstream issue thread, not this repo). Impact is lost debug-log text in DebugTree (debug tooling), no crash: reviewed LOW. Known upstream: YES. Issue #339 "Timber.debugTree splite lose word" (open since 2018-10-22, 9 comments). Comment 2021-03-10 names the cause ("three byte Chinese makes the calculation error", maintainer: "plausible"); 2021-06-01 Cyrillic loses half; 2024-11-14 suggests UTF-8 aware splitting and surrogate checks.
Upstream: Already reported upstream: #339.
From the report
for (tree in treeArray) {
tree.explicitTag.set(tag)
}Timber.tag() sets the tag on every planted tree, and each tree clears it only when that tree handles the call. The fan-out (for example treeArray.forEach { it.d(message, *args) }) has no per-tree isolation. If an earlier tree throws, the later trees never consume their tag, and the next unrelated log call on that thread carries the wrong tag.
Clear every tree's pending tag in a finally after the fan-out, or catch exceptions per tree. Add a test with a throwing tree followed by a normal one.
Review: Timber.kt:422-425 Timber.tag() sets explicitTag on every tree. Each tree consumes it in prepareLog line 149 val tag = tag (getter removes it, :27-31). Forest fan-out (e.g. :300) has no try/finally, so if tree N throws, trees N+1.. keep the tag and the next call on that thread is mislabelled. Mechanism is correct. But it needs a throwing tree, and the exception then propagates to the app's call site anyway (a bigger problem than a wrong tag); the most plausible trigger is finding 16. Consequence = one wrongly tagged log line. Fair LOW. No test covers it in TimberTest.kt.
Upstream: No matching upstream report found (11 October 2026).
timber/src/androidMain/kotlin/timber/log/Timber.kt:173 (line corrected on review; the report cites :174)From the report
actual protected open fun formatMessage(message: String, args: Array<out Any?>): String =
message.format(*args)When arguments are passed, the message goes straight to String.format. Calls such as Timber.e("Disk 50% full: %s", x) or Timber.d("%d", "text") throw a format exception from inside the logging call. That can crash the app on whatever path contains the log statement, and it skips the remaining trees. The bundled lint check catches many of these cases at build time, but not format strings built at runtime, and not projects that ignore lint.
In the default formatting path, catch IllegalFormatException and fall back to the raw message with the arguments appended. Logging should never take down the caller.
Review: Timber.kt:161-162 if (args.isNotEmpty()) { message = formatMessage(message, args), :173-174 message.format(*args). True that Timber.d("%d", "text") throws IllegalFormatConversionException and that "Disk 50% full: %s" with an arg parses % f as a conversion and throws. But: formatting only happens when args are passed (a message with % and no args is safe, :161); behaviour is identical to String.format, which is Timber's documented contract; the shipped lint check (TimberArgCount/TimberArgTypes, ERROR severity) targets exactly this; formatMessage is open, so apps can override it. A design choice (fail loud) rather than a defect. Fair LOW/INFO.
Upstream: No matching upstream report found (11 October 2026).
From the report
if (argumentIndex != numArguments) {
argument = arguments[argumentIndex++]
} else {The count pass honours explicit indexes, but the type pass assigns arguments strictly in order. Timber.d("%1$s and %1$s", x) is valid Java, yet the detector collects two types for one argument and reports an argument-count error. "%2$d %1$s", "a", 1 gets a false type error. These issues are reported at error severity, so they can fail a build that is correct. After a missing argument the loop also keeps reusing the previous argument.
Have the tokenizer return each conversion's argument index, and look up arguments by that index. Stop after reporting a missing argument. Add tests for repeated and reordered indexes.
Review: Trace for Timber.d("%1$s and %1$s", x): getFormatArgumentCount (:453-498) honours %1$ and returns max=1; passed=1, so no early report (:189). getStringArgumentTypes (:413-451) returns two types ['s','s']. Loop :210-224: i=0 takes arguments[1] (:212-213, argumentIndex becomes 2 = numArguments); i=1 hits the else branch :214-223 and reports ISSUE_ARG_COUNT ("requires 1 but supplies 1"). Severity is ERROR (:877-887). Confirmed by reading; not run. "%2$d %1$s", "a", 1 -> types ['d','s'] matched to args in order -> false type mismatch, also confirmed by reading. Impact: false lint ERROR on valid, rare code; can fail builds with abortOnError; workaround is @SuppressLint or rewriting. Shipped (timber/build.gradle:60 lintPublish project(':timber-lint')). Fair LOW. Known upstream: YES. Issue #167 "Timber lint check does not consider formatting string using indexing for backreferences" (open, 2016-09-21), repro Timber.d("setMeasuredDimension(%1$d, %1$d)", size): lint warns about invalid argument count.
Upstream: Already reported upstream: #167.
From the report
isLeftLiteral -> {
"\"${leftOperand.evaluateString()}%s\", ${rightOperand.asSourceString()}"
}The fix inserts the evaluated literal into a new format string without escaping %, quotes or backslashes. Applying it to Timber.d("100% done " + x) produces Timber.d("100% done %s", x). There % d is read as a format conversion, so the rewritten call throws at runtime (see finding 16). The tool meant to make logging safer introduces the crash. The other branches (lines 781 and 791) and the tag-length fix (line 806) have the same escaping gap.
Before embedding a literal, double every % and re-escape it as source syntax. Better, reuse the literal's original source text.
Review: :787-789 isLeftLiteral -> { "\"${leftOperand.evaluateString()}%s\", ${rightOperand.asSourceString()}" }; also :781, :791, :806 have no escaping. evaluateString() yields the unescaped value, so Timber.d("100% done " + x) becomes Timber.d("100% done %s", x): % d is a valid conversion, so it throws at runtime. Notably the original call was safe (no args -> no formatting, line 161), so the fix converts a working call into a crash. Quotes/backslashes in the literal would also yield invalid source. Real, but it needs the developer to apply an optional IDE fix on a literal containing %, " or \; the tag-length fix (:806) is a different case (tag text, quotes only). Fair LOW.
Upstream: No matching upstream report found (11 October 2026).
Info after review (9)
timber-lint/build.gradle:2 and timber-lint/build.gradle:18From the report
apply plugin: 'org.jetbrains.kotlin.kapt'
...
kapt libs.auto.serviceKAPT is in maintenance mode, slows builds by generating Java stubs, and lags behind new Kotlin compiler versions. The only processor here is AutoService, which writes a single service file.
Write META-INF/services/com.android.tools.lint.client.api.IssueRegistry by hand, or switch to a KSP-based AutoService processor. Then remove the kapt plugin.
Review: timber-lint/build.gradle:2 apply plugin: 'org.jetbrains.kotlin.kapt', :18 kapt libs.auto.service. Facts right (only AutoService processor). The lint module is a build-time artifact, nothing breaks; this is a modernisation wish. INFO.
timber-lint/lint-baseline.xml:2From the report
<issues format="6" by="lint 8.12.0" type="baseline" ... name="AGP (8.12.0)" version="8.12.0">The project now pins AGP 9.4.1 with lint 32.4.1. The baseline entries point at detector lines 760 and 778, but that text now sits at lines 850 and 871. A stale baseline can hide new issues or stop matching the ones it was meant to suppress. Either way, nobody can tell which warnings are really accepted.
Regenerate the baseline in a clean CI build with the current toolchain. Better still, fix the few underlying text-format warnings and delete the baseline.
Review: lint-baseline.xml:2 <issues format="6" by="lint 8.12.0" ... version="8.12.0">; libs.versions.toml agp = "9.4.1", androidTools = "32.4.1". Entries at lines 760/778 in the baseline; matching text now at detector lines 850 and 871 (confirmed by grep). But the consequence claimed ("can stop matching") is wrong: LintBaseline.findAndMark (lint-api 32.4.1, client/api/LintBaseline.kt:240-272) matches by message (messageToEntry[message]) then path suffix; line numbers are not compared (they are only written, :774). Version skew is also tolerated. So the baseline is not "stale" in a way that breaks anything. Also noteworthy: 83 of the 88 baseline entries are in the test file, 4 in the detector.
timber-sample/build.gradle:5From the report
compileSdkVersion libs.versions.compileSdk.get().toInteger()
...
lintOptions {compileSdkVersion, minSdkVersion, targetSdkVersion and lintOptions are deprecated, and current major AGP versions remove them step by step. The library module already uses the modern lint { } block, so the sample module is the one most likely to break on the next AGP upgrade.
Switch to compileSdk, minSdk, targetSdk and lint { }. Confirm the AGP and Gradle wrapper pairing against the published compatibility table in the same change.
Review: timber-sample/build.gradle:5 compileSdkVersion libs.versions.compileSdk.get().toInteger(); also minSdkVersion, targetSdkVersion, lintOptions {. Deprecated names are used; timber/build.gradle uses compileSdk =, minSdk =, lint { and timber-lint uses lint {. CI builds the sample (build.yaml:27 ./gradlew build), so it works at this commit presumably; whether AGP 9.4.1 still accepts these cannot be verified statically. Sample app, not shipped.
From the report
trees.add(tree)
treeArray = trees.toTypedArray()plant(tree), plant(vararg), uproot and uprootAll each rebuild treeArray themselves (lines 435, 448, 457 and 466). All four are correct today. But a future mutator that forgets the rebuild would leave every logging call working from a stale tree list, and no compiler or test would catch it.
Route all mutations through one private helper. It should take the lock, apply the change and republish the snapshot.
Review: Timber.kt:434-435 trees.add(tree) / treeArray = trees.toTypedArray(); also :447-448, :456-457, :465-466. All under synchronized(trees), treeArray is @Volatile (:484). The report itself says all are correct today; hypothetical-future-bug finding. INFO.
From the report
val matcher = StringFormatDetector.FORMAT.matcher(formatString)
while (true) {
if (matcher.find(index)) {The detector runs a backtracking regex over every format literal it sees, with no cap on length or match count. On normal code this is harmless. If lint runs on untrusted contributions in CI, a crafted long literal can make the analysis disproportionately slow.
Skip or report format strings above a fixed length (for example 4 KB) or above a fixed number of conversions. Scan each literal once, not twice.
Review: WrongTimberUsageDetector.kt:415 val matcher = StringFormatDetector.FORMAT.matcher(formatString), :419 while (true) {, :420 if (matcher.find(index)) { (the report cites :419 for the quote that sits at :415). The regex is Android lint's, not Timber's (lint-checks StringFormatDetector.kt:1279-1300): %(\d+\$)?([-+#, 0(<]*)?(\d+)?(\.\d+)?([tT])?([a-zA-Z%]). It is anchored on % and has no nested quantifiers. The only overlap is 0 in both flags and width, which by analysis gives at most quadratic work on a pathological %000...0 run (analysis only, not run). It runs on the developer's own source in lint, the same regex Android lint runs on every project. No realistic attack. "Scan once, not twice" is a fair tidy-up.
From the report
private fun checkFormatArguments(context: JavaContext, call: UCallExpression) {
...
private fun getFormatArgumentCount(s: String): Int {The file is over 900 lines. It contains format-string parsing and type checking (about 330 lines), exception-logging checks and the quick-fix builders. These parts share no state, and the size makes the parsing bugs below harder to spot and test in isolation.
Move format parsing and type checking into its own analyser, and the quick-fix builders into a separate file. Keep the detector as a thin visitor.
Review: File is 933 lines (wc). checkFormatArguments at :166, parsing helpers :413-498, quick fixes :740-828. Matches the description (about 330 lines of format logic = 166-498). Maintainability opinion.
From the report
class WrongTimberUsageDetectorTest {About 2,000 lines and more than 80 tests for 8 different issues sit in a single class. That makes coverage gaps hard to see; for example, no test covers positional format arguments (see finding 17).
Split the tests by issue into separate classes that share a small helper.
Review: WrongTimberUsageDetectorTest.kt is 1995 lines (correct), 8 issues (correct). But it holds 41 @Test methods (grep -c "@Test" = 41), not "more than 80". No test uses a positional specifier (grep for %1$ and $s finds nothing): that claim is right.
timber-lint/src/main/java/timber/lint/WrongTimberUsageDetector.kt:190 (line corrected on review; the report cites :215)From the report
issue = ISSUE_ARG_COUNT,
...
"Wrong argument count, format string `${formatString}` requires `${formatArgumentCount}` but format call supplies `${passedArgCount}`",The report at lines 190–198 is copied at lines 215–223. The second copy runs inside a loop that keeps going after reporting, so one bad call can produce several identical warnings. Message changes also have to be made twice.
Extract one small reporting helper and stop the loop after the first missing-argument report.
Review: :190-198 and :215-223 build the same Incident(ISSUE_ARG_COUNT ...) with identical text. In the else branch (:214-224) there is no break/return, so with "%s %s %s" and one arg the report fires for i=1 and i=2. Whether lint de-duplicates identical incidents was not verified. Cosmetic.
From the report
treeArray.forEach { it.v(message, *args) }Note: The same one-line fan-out appears 21 times, once per public overload. The public API and the multiplatform declarations require this shape, so no change is needed. A short comment explaining why would save future reviewers the question.
Review: Report's conclusion (no change needed) is fair.