Browse Source

fix(java): Fix deobfuscation for mixed stacktraces (ANR) (#63829)

Same as #63740, but does deep freeze instead of `frozenset` in case
there's a nested dict in a frame to prevent this issue
https://sentry.sentry.io/issues/4914654272/events/9fbb164f5a9347f4ab8296bf93b71de9/

The actual fix is in this commit
59ba866c5ae8210b27c0728abc38aa96c4a11521
Roman Zavarnitsyn 1 year ago
parent
commit
cd66cd9e11

+ 24 - 17
src/sentry/lang/java/plugin.py

@@ -23,6 +23,7 @@ class JavaStacktraceProcessor(StacktraceProcessor):
 
         self.images = get_proguard_images(self.data)
         self.available = len(self.images) > 0
+        self.mapping_views = []
 
     def handles_frame(self, frame, stacktrace_info):
         platform = frame.get("platform") or self.data.get("platform")
@@ -36,7 +37,6 @@ class JavaStacktraceProcessor(StacktraceProcessor):
             dif_paths = ProjectDebugFile.difcache.fetch_difs(
                 self.project, self.images, features=["mapping"]
             )
-            self.mapping_views = []
 
         for debug_id in self.images:
             error_type = None
@@ -71,6 +71,9 @@ class JavaStacktraceProcessor(StacktraceProcessor):
         return True
 
     def process_exception(self, exception):
+        if not self.available:
+            return False
+
         ty = exception.get("type")
         mod = exception.get("module")
         if not ty or not mod:
@@ -149,31 +152,35 @@ class JavaSourceLookupStacktraceProcessor(StacktraceProcessor):
     def __init__(self, *args, **kwargs):
         StacktraceProcessor.__init__(self, *args, **kwargs)
         self.proguard_processor = JavaStacktraceProcessor(*args, **kwargs)
-        self._proguard_processor_handles_frame = None
-        self._handles_frame = None
+        self._proguard_processor_handles_frame = {}
+        self._handles_frame = {}
         self.images = get_jvm_images(self.data)
         self._archives = []
         self.available = len(self.images) > 0
 
+    def _deep_freeze(self, d):
+        if isinstance(d, dict):
+            return frozenset((key, self._deep_freeze(value)) for key, value in d.items())
+        elif isinstance(d, list):
+            return tuple(self._deep_freeze(value) for value in d)
+        return d
+
     def close(self):
         for archive in self._archives:
             archive.close()
 
     def handles_frame(self, frame, stacktrace_info):
-        self._proguard_processor_handles_frame = self.proguard_processor.handles_frame(
+        key = self._deep_freeze(frame)
+        self._proguard_processor_handles_frame[key] = self.proguard_processor.handles_frame(
             frame, stacktrace_info
         )
 
         platform = frame.get("platform") or self.data.get("platform")
-        self._handles_frame = platform == "java" and self.available and "module" in frame
-        return self._proguard_processor_handles_frame or self._handles_frame
+        self._handles_frame[key] = platform == "java" and self.available and "module" in frame
+        return self._proguard_processor_handles_frame[key] or self._handles_frame[key]
 
     def preprocess_step(self, processing_task):
-        proguard_processor_preprocess_rv = False
-        if self._proguard_processor_handles_frame:
-            proguard_processor_preprocess_rv = self.proguard_processor.preprocess_step(
-                processing_task
-            )
+        proguard_processor_preprocess_rv = self.proguard_processor.preprocess_step(processing_task)
 
         if not self.available:
             return proguard_processor_preprocess_rv
@@ -189,9 +196,7 @@ class JavaSourceLookupStacktraceProcessor(StacktraceProcessor):
         return proguard_processor_preprocess_rv or self.available
 
     def process_exception(self, exception):
-        if self._proguard_processor_handles_frame:
-            return self.proguard_processor.process_exception(exception)
-        return False
+        return self.proguard_processor.process_exception(exception)
 
     # if path contains a '$' sign or doesn't contain a '.' it has most likely been obfuscated
     def _is_valid_path(self, abs_path):
@@ -233,8 +238,10 @@ class JavaSourceLookupStacktraceProcessor(StacktraceProcessor):
         new_frames = None
         raw_frames = None
         processing_errors = None
+        bare_frame = processable_frame.frame
+        key = self._deep_freeze(bare_frame)
 
-        if self._proguard_processor_handles_frame:
+        if self._proguard_processor_handles_frame[key]:
             proguard_result = self.proguard_processor.process_frame(
                 processable_frame, processing_task
             )
@@ -242,11 +249,11 @@ class JavaSourceLookupStacktraceProcessor(StacktraceProcessor):
             if proguard_result:
                 new_frames, raw_frames, processing_errors = proguard_result
 
-        if not self._handles_frame:
+        if not self._handles_frame[key]:
             return new_frames, raw_frames, processing_errors
 
         if not new_frames:
-            new_frames = [dict(processable_frame.frame)]
+            new_frames = [dict(bare_frame)]
 
         for new_frame in new_frames:
             lineno = new_frame.get("lineno")

+ 7 - 1
src/sentry/lang/java/processing.py

@@ -9,7 +9,13 @@ def deobfuscate_exception_value(data):
     exception = get_path(data, "exception", "values", -1)
     frame = get_path(exception, "stacktrace", "frames", -1)
     raw_frame = get_path(exception, "raw_stacktrace", "frames", -1)
-    if frame and raw_frame and exception.get("value"):
+    if (
+        frame
+        and raw_frame
+        and frame.get("module")
+        and frame.get("function")
+        and exception.get("value")
+    ):
         deobfuscated_method_name = f"{frame['module']}.{frame['function']}"
         raw_method_name = f"{raw_frame['module']}.{raw_frame['function']}"
         exception["value"] = re.sub(

+ 167 - 0
tests/relay_integration/lang/java/test_plugin.py

@@ -553,6 +553,74 @@ class BasicResolvingIntegrationTest(RelayStoreHelper, TransactionTestCase):
         metrics = event.data["_metrics"]
         assert not metrics.get("flag.processing.error")
 
+    def test_resolving_does_not_fail_when_no_module_or_function(self):
+        self.upload_proguard_mapping(PROGUARD_UUID, PROGUARD_SOURCE)
+
+        event_data = {
+            "user": {"ip_address": "31.172.207.97"},
+            "extra": {},
+            "project": self.project.id,
+            "platform": "java",
+            "debug_meta": {"images": [{"type": "proguard", "uuid": PROGUARD_UUID}]},
+            "exception": {
+                "values": [
+                    {
+                        "stacktrace": {
+                            "frames": [
+                                {
+                                    "function": "a",
+                                    "abs_path": None,
+                                    "module": "org.a.b.g$a",
+                                    "filename": None,
+                                    "lineno": 67,
+                                },
+                                {
+                                    "function": "a",
+                                    "abs_path": None,
+                                    "module": "org.a.b.g$a",
+                                    "filename": None,
+                                    "lineno": 69,
+                                },
+                                {
+                                    "function": "__start_thread",
+                                    "package": "/apex/com.android.art/lib64/libart.so",
+                                    "lineno": 196,
+                                    "in_app": False,
+                                },
+                                {
+                                    "package": "/apex/com.android.art/lib64/libart.so",
+                                    "lineno": 214,
+                                    "in_app": False,
+                                },
+                            ]
+                        },
+                        "module": "org.a.b",
+                        "type": "g$a",
+                        "value": "Attempt to invoke virtual method 'org.a.b.g$a.a' on a null object reference",
+                    }
+                ]
+            },
+            "timestamp": iso_format(before_now(seconds=1)),
+        }
+
+        event = self.post_and_retrieve_event(event_data)
+        if not self.use_relay():
+            # We measure the number of queries after an initial post,
+            # because there are many queries polluting the array
+            # before the actual "processing" happens (like, auth_user)
+            with self.assertWriteQueries(
+                {
+                    "nodestore_node": 2,
+                    "sentry_eventuser": 1,
+                    "sentry_groupedmessage": 1,
+                    "sentry_userreport": 1,
+                }
+            ):
+                self.post_and_retrieve_event(event_data)
+
+        metrics = event.data["_metrics"]
+        assert not metrics.get("flag.processing.error")
+
     def test_sets_inapp_after_resolving(self):
         self.upload_proguard_mapping(PROGUARD_UUID, PROGUARD_SOURCE)
 
@@ -722,6 +790,105 @@ class BasicResolvingIntegrationTest(RelayStoreHelper, TransactionTestCase):
         assert frames[3].filename == "MainActivity.java"
         assert frames[3].module == "io.sentry.sample.MainActivity"
 
+    def test_resolving_inline_with_native_frames(self):
+        self.upload_proguard_mapping(PROGUARD_INLINE_UUID, PROGUARD_INLINE_SOURCE)
+
+        event_data = {
+            "user": {"ip_address": "31.172.207.97"},
+            "extra": {},
+            "project": self.project.id,
+            "platform": "java",
+            "debug_meta": {"images": [{"type": "proguard", "uuid": PROGUARD_INLINE_UUID}]},
+            "exception": {
+                "values": [
+                    {
+                        "stacktrace": {
+                            "frames": [
+                                {
+                                    "function": "onClick",
+                                    "abs_path": None,
+                                    "module": "e.a.c.a",
+                                    "filename": None,
+                                    "lineno": 2,
+                                },
+                                {
+                                    "function": "t",
+                                    "abs_path": None,
+                                    "module": "io.sentry.sample.MainActivity",
+                                    "filename": "MainActivity.java",
+                                    "lineno": 1,
+                                },
+                                {
+                                    "abs_path": "Thread.java",
+                                    "filename": "Thread.java",
+                                    "function": "sleep",
+                                    "lineno": 450,
+                                    "lock": {
+                                        "address": "0x0ddc1f22",
+                                        "class_name": "Object",
+                                        "package_name": "java.lang",
+                                        "type:": 1,
+                                    },
+                                    "module": "java.lang.Thread",
+                                },
+                                {
+                                    "function": "__start_thread",
+                                    "package": "/apex/com.android.art/lib64/libart.so",
+                                    "lineno": 196,
+                                    "in_app": False,
+                                },
+                            ]
+                        },
+                        "module": "org.a.b",
+                        "type": "g$a",
+                        "value": "Oh no",
+                    }
+                ]
+            },
+            "timestamp": iso_format(before_now(seconds=1)),
+        }
+
+        event = self.post_and_retrieve_event(event_data)
+        if not self.use_relay():
+            # We measure the number of queries after an initial post,
+            # because there are many queries polluting the array
+            # before the actual "processing" happens (like, auth_user)
+            with self.assertWriteQueries(
+                {
+                    "nodestore_node": 2,
+                    "sentry_eventuser": 1,
+                    "sentry_groupedmessage": 1,
+                    "sentry_userreport": 1,
+                }
+            ):
+                self.post_and_retrieve_event(event_data)
+
+        exc = event.interfaces["exception"].values[0]
+        bt = exc.stacktrace
+        frames = bt.frames
+
+        assert len(frames) == 6
+
+        assert frames[0].function == "onClick"
+        assert frames[0].module == "io.sentry.sample.-$$Lambda$r3Avcbztes2hicEObh02jjhQqd4"
+
+        assert frames[1].filename == "MainActivity.java"
+        assert frames[1].module == "io.sentry.sample.MainActivity"
+        assert frames[1].function == "onClickHandler"
+        assert frames[1].lineno == 40
+        assert frames[2].function == "foo"
+        assert frames[2].lineno == 44
+        assert frames[3].function == "bar"
+        assert frames[3].lineno == 54
+        assert frames[3].filename == "MainActivity.java"
+        assert frames[3].module == "io.sentry.sample.MainActivity"
+        assert frames[4].function == "sleep"
+        assert frames[4].lineno == 450
+        assert frames[4].filename == "Thread.java"
+        assert frames[4].module == "java.lang.Thread"
+        assert frames[5].function == "__start_thread"
+        assert frames[5].package == "/apex/com.android.art/lib64/libart.so"
+
     def test_error_on_resolving(self):
         url = reverse(
             "sentry-api-0-dsym-files",