From ba3e989e7ac91e049e7a5fc468c0c6b0eaa1ab3a Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Wed, 5 Feb 2025 11:03:33 +0100 Subject: [PATCH] Improve Nullability, consider case of null imageSize --- .../sentry/android/ndk/DebugImagesLoader.java | 78 +++++++++++-------- .../android/ndk/DebugImagesLoaderTest.kt | 41 ++++++++-- 2 files changed, 81 insertions(+), 38 deletions(-) diff --git a/sentry-android-ndk/src/main/java/io/sentry/android/ndk/DebugImagesLoader.java b/sentry-android-ndk/src/main/java/io/sentry/android/ndk/DebugImagesLoader.java index 3362d1c99c..82bc7a3bd0 100644 --- a/sentry-android-ndk/src/main/java/io/sentry/android/ndk/DebugImagesLoader.java +++ b/sentry-android-ndk/src/main/java/io/sentry/android/ndk/DebugImagesLoader.java @@ -27,7 +27,7 @@ public final class DebugImagesLoader implements IDebugImagesLoader { private final @NotNull NativeModuleListLoader moduleListLoader; - private static @Nullable List debugImages; + private static volatile @Nullable List debugImages; /** we need to lock it because it could be called from different threads */ protected static final @NotNull AutoClosableReentrantLock debugImagesLock = @@ -81,23 +81,19 @@ public DebugImagesLoader( * Loads debug images for the given set of addresses. * * @param addresses Set of memory addresses to find debug images for - * @return Set of debug images, or null if debug images couldn't be loaded + * @return Set of matching debug images, or null if debug images couldn't be loaded */ - public @Nullable Set loadDebugImagesForAddresses(@NotNull Set addresses) { + public @Nullable Set loadDebugImagesForAddresses(final @NotNull Set addresses) { try (final @NotNull ISentryLifecycleToken ignored = debugImagesLock.acquire()) { - List allDebugImages = loadDebugImages(); + final @Nullable List allDebugImages = loadDebugImages(); if (allDebugImages == null) { return null; } - - Set referencedImages = new HashSet<>(); - for (Long addr : addresses) { - DebugImage image = findImageByAddress(addr, allDebugImages); - if (image != null) { - referencedImages.add(image); - } + if (addresses.isEmpty()) { + return null; } + final Set referencedImages = filterImagesByAddresses(allDebugImages, addresses); if (referencedImages.isEmpty()) { options .getLogger() @@ -118,31 +114,47 @@ public DebugImagesLoader( * * @return The matching debug image or null if not found */ - private @Nullable DebugImage findImageByAddress(long address, List images) { - int left = 0; - int right = images.size() - 1; - - while (left <= right) { - int mid = left + (right - left) / 2; - DebugImage image = images.get(mid); - - if (image.getImageAddr() == null || image.getImageSize() == null) { - return null; - } - - long imageStart = Long.parseLong(image.getImageAddr().replace("0x", ""), 16); - long imageEnd = imageStart + image.getImageSize(); - - if (address >= imageStart && address < imageEnd) { - return image; - } else if (address < imageStart) { - right = mid - 1; - } else { - left = mid + 1; + private @NotNull Set filterImagesByAddresses( + final @NotNull List images, final @NotNull Set addresses) { + final Set result = new HashSet<>(); + + for (int i = 0; i < images.size(); i++) { + final @NotNull DebugImage image = images.get(i); + final @Nullable DebugImage nextDebugImage = + (i + 1) < images.size() ? images.get(i + 1) : null; + final @Nullable String nextDebugImageAddress = + nextDebugImage != null ? nextDebugImage.getImageAddr() : null; + + for (final @NotNull Long address : addresses) { + final @Nullable String imageAddress = image.getImageAddr(); + + if (imageAddress != null) { + try { + final long imageStart = Long.parseLong(imageAddress.replace("0x", ""), 16); + final long imageEnd; + + final @Nullable Long imageSize = image.getImageSize(); + if (imageSize != null) { + imageEnd = imageStart + imageSize; + } else if (nextDebugImageAddress != null) { + imageEnd = Long.parseLong(nextDebugImageAddress.replace("0x", ""), 16); + } else { + imageEnd = Long.MAX_VALUE; + } + if (address >= imageStart && address < imageEnd) { + result.add(image); + // once image is added we can skip the remaining addresses and go straight to the next + // image + break; + } + } catch (NumberFormatException e) { + options.getLogger().log(SentryLevel.WARNING, e, "Failed to parse image address."); + } + } } } - return null; + return result; } @Override diff --git a/sentry-android-ndk/src/test/java/io/sentry/android/ndk/DebugImagesLoaderTest.kt b/sentry-android-ndk/src/test/java/io/sentry/android/ndk/DebugImagesLoaderTest.kt index 45aa37c7a4..650a046d27 100644 --- a/sentry-android-ndk/src/test/java/io/sentry/android/ndk/DebugImagesLoaderTest.kt +++ b/sentry-android-ndk/src/test/java/io/sentry/android/ndk/DebugImagesLoaderTest.kt @@ -17,11 +17,13 @@ class DebugImagesLoaderTest { val options = SentryAndroidOptions() fun getSut(): DebugImagesLoader { - return DebugImagesLoader(options, nativeLoader) + val loader = DebugImagesLoader(options, nativeLoader) + loader.clearDebugImages() + return loader } } - private val fixture = Fixture() + private var fixture = Fixture() @Test fun `get images returns image list`() { @@ -102,11 +104,12 @@ class DebugImagesLoaderTest { val result = sut.loadDebugImagesForAddresses( setOf(0x1500L, 0x2500L) - )!!.toList() + ) + assertNotNull(result) assertEquals(2, result.size) - assertEquals(image1.imageAddr, result[0].imageAddr) - assertEquals(image2.imageAddr, result[1].imageAddr) + assertTrue(result.any { it.imageAddr == image1.imageAddr }) + assertTrue(result.any { it.imageAddr == image2.imageAddr }) } @Test @@ -152,4 +155,32 @@ class DebugImagesLoaderTest { assertNull(result) } + + @Test + fun `invalid image adresses are ignored for loadDebugImagesForAddresses`() { + val sut = fixture.getSut() + + val image1 = io.sentry.ndk.DebugImage().apply { + imageAddr = "0xNotANumber" + imageSize = 0x1000L + } + + val image2 = io.sentry.ndk.DebugImage().apply { + imageAddr = "0x2000" + imageSize = null + } + + val image3 = io.sentry.ndk.DebugImage().apply { + imageAddr = "0x5000" + imageSize = 0x1000L + } + + whenever(fixture.nativeLoader.loadModuleList()).thenReturn(arrayOf(image1, image2, image3)) + + val hexAddresses = setOf(100L, 0x2000L, 0x2000L, 0x5000L) + val result = sut.loadDebugImagesForAddresses(hexAddresses) + + assertNotNull(result) + assertEquals(2, result.size) + } }