Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Fixes

- Detect minified classes and skip instrumentation to avoid build problems ([#362](https://github.com/getsentry/sentry-android-gradle-plugin/pull/362))

### Features

- Bump AGP to 7.2.1 and Gradle to 7.5.0 ([#363](https://github.com/getsentry/sentry-android-gradle-plugin/pull/363))
Expand Down
Expand Up @@ -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)
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Member Author

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:

SentryPlugin.logger.info {
                "$simpleClassName skipped from instrumentation because it's a minified class."
            }

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.

}

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
Expand All @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fun classNameLooksMinified(simpleClassName: String, fullClassName: String): Boolean {
private fun classNameLooksMinified(simpleClassName: String, fullClassName: String): Boolean {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't access it directly from test if I do that.

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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).
*
Expand Down
Expand Up @@ -23,6 +23,7 @@ class SentryPluginWithMinifiedLibsTest :
implementation 'com.google.android.gms:play-services-mlkit-text-recognition:18.0.0'
implementation 'com.adcolony:sdk:4.7.1'
implementation 'com.appboy:android-sdk-ui:19.0.0'
implementation 'com.stripe:stripeterminal-internal-common:2.12.0'
}
""".trimIndent()
)
Expand Down
@@ -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)
}
}
}