New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Detect minified classes and skip instrumentation to avoid build problems #362
Changes from all commits
abdfdbe
14dd6be
d1610e5
6d18544
31608d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -44,17 +44,25 @@ internal fun ClassWriter.findClassReader(): ClassReader? { | |||||
return (classReaderField.get(symbolTable) as? ClassReader) | ||||||
} | ||||||
|
||||||
internal fun ClassReader.getSimpleClassName(): String { | ||||||
return className.substringAfterLast("/") | ||||||
} | ||||||
|
||||||
/** | ||||||
* Looks at the constant pool entries and searches for R8 markers | ||||||
*/ | ||||||
internal fun ClassReader.isMinifiedClass(): Boolean { | ||||||
val charBuffer = CharArray(maxStringLength) | ||||||
return isR8Minified(this) || classNameLooksMinified(this.getSimpleClassName(), this.className) | ||||||
} | ||||||
|
||||||
private fun isR8Minified(classReader: ClassReader): Boolean { | ||||||
val charBuffer = CharArray(classReader.maxStringLength) | ||||||
// R8 marker is usually in the first 3-5 entries, so we limit it at 10 to speed it up | ||||||
// (constant pool size can be huge otherwise) | ||||||
val poolSize = minOf(10, itemCount) | ||||||
val poolSize = minOf(10, classReader.itemCount) | ||||||
for (i in 1 until poolSize) { | ||||||
try { | ||||||
val constantPoolEntry = readConst(i, charBuffer) | ||||||
val constantPoolEntry = classReader.readConst(i, charBuffer) | ||||||
if (constantPoolEntry is String && "~~R8" in constantPoolEntry) { | ||||||
// ~~R8 is a marker in the class' constant pool, which r8 itself is looking at when | ||||||
// parsing a .class file. See here -> https://r8.googlesource.com/r8/+/refs/heads/main/src/main/java/com/android/tools/r8/dex/Marker.java#53 | ||||||
|
@@ -68,6 +76,25 @@ internal fun ClassReader.isMinifiedClass(): Boolean { | |||||
return false | ||||||
} | ||||||
|
||||||
/** | ||||||
* See https://github.com/getsentry/sentry-android-gradle-plugin/issues/360 | ||||||
* and https://github.com/getsentry/sentry-android-gradle-plugin/issues/359#issuecomment-1193782500 | ||||||
*/ | ||||||
/* ktlint-disable max-line-length */ | ||||||
private val MINIFIED_CLASSNAME_REGEX = """^(((([a-zA-z])\4{1,}|[a-zA-Z]{1,2})([0-9]{1,})?(([a-zA-Z])\7{1,})?)|([a-zA-Z]([0-9])?))(${'\\'}${'$'}((((\w)\14{1,}|[a-zA-Z]{1,2})([0-9]{1,})?(([a-zA-Z])\17{1,})?)|(\w([0-9])?)))*${'$'}""".toRegex() | ||||||
|
||||||
/** | ||||||
* See https://github.com/getsentry/sentry/blob/c943de2afc785083554e7fdfb10c67d0c0de0f98/static/app/components/events/eventEntries.tsx#L57-L58 | ||||||
*/ | ||||||
private val MINIFIED_CLASSNAME_SENTRY_REGEX = | ||||||
"""^(([\w\${'$'}]\/[\w\${'$'}]{1,2})|([\w\${'$'}]{2}\/[\w\${'$'}]\/[\w\${'$'}]))(\/|${'$'})""".toRegex() | ||||||
/* ktlint-enable max-line-length */ | ||||||
|
||||||
fun classNameLooksMinified(simpleClassName: String, fullClassName: String): Boolean { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't access it directly from test if I do that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we add the annotation that is only visible for the test then? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think they're available in SAGP (yet). |
||||||
return MINIFIED_CLASSNAME_REGEX.matches(simpleClassName) || | ||||||
MINIFIED_CLASSNAME_SENTRY_REGEX.matches(fullClassName) | ||||||
} | ||||||
|
||||||
/** | ||||||
* Gets all fields of the given class and its parents (if any). | ||||||
* | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
package io.sentry.android.gradle.instrumentation.util | ||
|
||
import kotlin.test.assertFalse | ||
import kotlin.test.assertTrue | ||
import org.junit.Test | ||
|
||
class MinifiedClassDetectionTest { | ||
|
||
@Test | ||
fun `detects minified class names`() { | ||
val classNames = listOf( | ||
"l0", | ||
"""a${'$'}a""", | ||
"ccc017zz", | ||
"""ccc017zz${'$'}a""", | ||
"aa", | ||
"aa${'$'}a", | ||
"ab", | ||
"aa${'$'}ab", | ||
"ab${'$'}a" | ||
) | ||
|
||
classNames.forEach { | ||
assertTrue(classNameLooksMinified(it, "com/example/$it"), it) | ||
} | ||
} | ||
|
||
@Test | ||
fun `detects minified class names with minified package name`() { | ||
val classNames = listOf( | ||
"""a${'$'}""", | ||
"aa" | ||
) | ||
|
||
classNames.forEach { | ||
assertTrue(classNameLooksMinified(it, "a/$it"), it) | ||
} | ||
} | ||
|
||
@Test | ||
fun `does not consider non minified classes as minified`() { | ||
val classNames = listOf( | ||
"ConstantPoolHelpers", | ||
"FileUtil", | ||
"""FileUtil${"$"}Inner""" | ||
) | ||
|
||
classNames.forEach { | ||
assertFalse(classNameLooksMinified(it, "com/example/$it"), it) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there are many different ways of checking it now, I am wondering if we'd not have false positives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we call our logger in case
isMinifiedClass
returns true? Let's do it if not, so people know why something isn't being instrumented.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log statement is in place:
I guess it's better to skip instrumentation for too many classes rather than break the build. But yes, we'll surely have some false positives due to the regex.