From dc5b90229f97442a69b71dee12e71979f1c93558 Mon Sep 17 00:00:00 2001 From: Phillip Davis Date: Mon, 23 Feb 2026 12:40:52 -0500 Subject: [PATCH] fix: NPE from unsynchronized MucOptions access - ConversationGetMucOptionsRaceTest is in its own file to make the instrumentation barriers a little easier to see and enforce - It simulates basically the following situation: i. On app startup, all accounts connect to all MUCs, which eventually triggers joinMuc, which calls Conversation.resetMucOptions. This happens on account's individual XMPP connection Thread. ii. At the same time, on the UI thread, we bind ConversationsOverviewFragment, which eventually leads to ConversationAdapter.onBindViewHolder, which calls getMucOptions. iii. Conversation.resetMucOptions is not synchronized, so it can run after the null-check in getMucOptions, resulting in a null return value. - Used AtomicReference instead of synchronizing resetMucOptions because joinMuc is run on a directExecutor associated with the XmppConnection thread. While the synchronized getMucOptions was called right after this in the existing code, the documentation for directExecutor has this to say: When a ListenableFuture listener is registered to run under directExecutor, the listener can execute in any of three possible threads: - When a thread attaches a listener to a ListenableFuture that's already complete, the listener runs immediately in that thread. - When a thread attaches a listener to a ListenableFuture that's incomplete and the ListenableFuture later completes normally, the listener runs in the thread that completes the ListenableFuture. - When a listener is attached to a ListenableFuture and the ListenableFuture gets cancelled, the listener runs immediately in the thread that cancelled the Future. - (This is applicable, since directExecutor uses in XmppConnectionService are extensively attached to ListenableFuture) The docs continue: A specific warning about locking: Code that executes user-supplied tasks, such as ListenableFuture listeners, should take care not to do so while holding a lock. Additionally, as a further line of defense, prefer not to perform any locking inside a task that will be run under directExecutor: Not only might the wait for a lock be long, but if the running thread was holding a lock, the listener may deadlock or break lock isolation. - Altogether, AtomicReference seems much more aligned with how Google intends directExecutor to be used. Anyway, here's the stacktrace that reported this error: ``` 2026-02-20 10:41:09.089 7610-7610 AndroidRuntime com.cheogram.android E FATAL EXCEPTION: main (Ask Gemini) Process: com.cheogram.android, PID: 7610 java.lang.NullPointerException: Attempt to invoke virtual method 'java.lang.String eu.siacs.conversations.entities.MucOptions.getName()' on a null object reference at eu.siacs.conversations.entities.Conversation.getName(Conversation.java:1068) at eu.siacs.conversations.entities.Conversation.getAvatarName(Conversation.java:1784) at eu.siacs.conversations.ui.util.AvatarWorkerTask.setContentDescription(AvatarWorkerTask.java:135) at eu.siacs.conversations.ui.util.AvatarWorkerTask.loadAvatar(AvatarWorkerTask.java:105) at eu.siacs.conversations.ui.adapter.ConversationAdapter.onBindViewHolder(ConversationAdapter.java:255) at eu.siacs.conversations.ui.adapter.ConversationAdapter.onBindViewHolder(ConversationAdapter.java:33) at androidx.recyclerview.widget.RecyclerView$Adapter.onBindViewHolder(RecyclerView.java:7846) at androidx.recyclerview.widget.RecyclerView$Adapter.bindViewHolder(RecyclerView.java:7953) at androidx.recyclerview.widget.RecyclerView$Recycler.tryBindViewHolderByDeadline(RecyclerView.java:6742) at androidx.recyclerview.widget.RecyclerView$Recycler.tryGetViewHolderForPositionByDeadline(RecyclerView.java:7013) at androidx.recyclerview.widget.RecyclerView$Recycler.getViewForPosition(RecyclerView.java:6853) at androidx.recyclerview.widget.RecyclerView$Recycler.getViewForPosition(RecyclerView.java:6849) at androidx.recyclerview.widget.LinearLayoutManager$LayoutState.next(LinearLayoutManager.java:2422) at androidx.recyclerview.widget.LinearLayoutManager.layoutChunk(LinearLayoutManager.java:1722) at androidx.recyclerview.widget.LinearLayoutManager.fill(LinearLayoutManager.java:1682) at androidx.recyclerview.widget.LinearLayoutManager.onLayoutChildren(LinearLayoutManager.java:747) at androidx.recyclerview.widget.RecyclerView.dispatchLayoutStep2(RecyclerView.java:4737) at androidx.recyclerview.widget.RecyclerView.dispatchLayout(RecyclerView.java:4459) at androidx.recyclerview.widget.RecyclerView.onLayout(RecyclerView.java:5011) at android.view.View.layout(View.java:25626) at android.view.ViewGroup.layout(ViewGroup.java:6460) at android.widget.LinearLayout.setChildFrame(LinearLayout.java:1891) at android.widget.LinearLayout.layoutVertical(LinearLayout.java:1729) at android.widget.LinearLayout.onLayout(LinearLayout.java:1638) at android.view.View.layout(View.java:25626) at android.view.ViewGroup.layout(ViewGroup.java:6460) at androidx.coordinatorlayout.widget.CoordinatorLayout.layoutChild(CoordinatorLayout.java:1213) at androidx.coordinatorlayout.widget.CoordinatorLayout.onLayoutChild(CoordinatorLayout.java:899) at androidx.coordinatorlayout.widget.CoordinatorLayout.onLayout(CoordinatorLayout.java:919) at android.view.View.layout(View.java:25626) at android.view.ViewGroup.layout(ViewGroup.java:6460) at android.widget.FrameLayout.layoutChildren(FrameLayout.java:332) at android.widget.FrameLayout.onLayout(FrameLayout.java:270) at android.view.View.layout(View.java:25626) at android.view.ViewGroup.layout(ViewGroup.java:6460) at android.widget.LinearLayout.setChildFrame(LinearLayout.java:1891) at android.widget.LinearLayout.layoutVertical(LinearLayout.java:1729) at android.widget.LinearLayout.onLayout(LinearLayout.java:1638) at android.view.View.layout(View.java:25626) at android.view.ViewGroup.layout(ViewGroup.java:6460) at androidx.drawerlayout.widget.DrawerLayout.onLayout(DrawerLayout.java:1273) at android.view.View.layout(View.java:25626) at android.view.ViewGroup.layout(ViewGroup.java:6460) at android.widget.FrameLayout.layoutChildren(FrameLayout.java:332) at android.widget.FrameLayout.onLayout(FrameLayout.java:270) at android.view.View.layout(View.java:25626) at android.view.ViewGroup.layout(ViewGroup.java:6460) at android.widget.FrameLayout.layoutChildren(FrameLayout.java:332) at android.widget.FrameLayout.onLayout(FrameLayout.java:270) at android.view.View.layout(View.java:25626) at android.view.ViewGroup.layout(ViewGroup.java:6460) ``` --- build.gradle | 3 + .../conversations/entities/Conversation.java | 20 +- .../xmpp/manager/MultiUserChatManager.java | 3 +- .../ConversationGetMucOptionsRaceTest.java | 183 ++++++++++++++++++ 4 files changed, 198 insertions(+), 11 deletions(-) create mode 100644 src/test/java/eu/siacs/conversations/entities/ConversationGetMucOptionsRaceTest.java diff --git a/build.gradle b/build.gradle index 8caaf0b0e6385639d625f3159c312b556b668f24..94e28a41eacf8369500de3823d6569419312f2da 100644 --- a/build.gradle +++ b/build.gradle @@ -134,6 +134,9 @@ dependencies { testImplementation 'junit:junit:4.13.2' testImplementation 'org.robolectric:robolectric:4.14.1' + testImplementation 'org.mockito:mockito-core:5.14.2' + testImplementation 'net.bytebuddy:byte-buddy:1.15.11' + testImplementation 'net.bytebuddy:byte-buddy-agent:1.15.11' androidTestImplementation 'androidx.test.ext:junit:1.2.1' } diff --git a/src/main/java/eu/siacs/conversations/entities/Conversation.java b/src/main/java/eu/siacs/conversations/entities/Conversation.java index 63f0c1579cf3fb548be8bf744bd7a0825dcb6ec7..8c68246bb3dfb19a566ddcce77b72b76fd231716 100644 --- a/src/main/java/eu/siacs/conversations/entities/Conversation.java +++ b/src/main/java/eu/siacs/conversations/entities/Conversation.java @@ -110,6 +110,7 @@ import java.util.HashSet; import java.util.List; import java.util.ListIterator; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; import java.util.Set; import java.util.Timer; @@ -218,7 +219,7 @@ public class Conversation extends AbstractEntity private int mode; private final JSONObject attributes; private Jid nextCounterpart; - private transient MucOptions mucOptions = null; + private final transient AtomicReference mucOptions = new AtomicReference<>(); private boolean messagesLeftOnServer = true; private ChatState mOutgoingChatState = Config.DEFAULT_CHAT_STATE; private ChatState mIncomingChatState = Config.DEFAULT_CHAT_STATE; @@ -1141,15 +1142,16 @@ public class Conversation extends AbstractEntity return getMucOptions().isPrivateAndNonAnonymous(); } - public synchronized MucOptions getMucOptions() { - if (this.mucOptions == null) { - this.mucOptions = new MucOptions(this); - } - return this.mucOptions; + public @NonNull MucOptions getMucOptions() { + return this.mucOptions.updateAndGet( + existing -> existing != null ? existing : new MucOptions(this) + ); } - public void resetMucOptions() { - this.mucOptions = null; + public @NonNull MucOptions resetMucOptions() { + return this.mucOptions.updateAndGet( + ignoredExisting -> new MucOptions(this) + ); } public void setContactJid(final Jid jid) { @@ -1344,7 +1346,7 @@ public class Conversation extends AbstractEntity public boolean storeInCache() { if ("cache".equals(getAttribute("storeMedia"))) return true; if ("shared".equals(getAttribute("storeMedia"))) return false; - if (mode == Conversation.MODE_MULTI && !mucOptions.isPrivateAndNonAnonymous()) return true; + if (mode == Conversation.MODE_MULTI && !getMucOptions().isPrivateAndNonAnonymous()) return true; return false; } diff --git a/src/main/java/eu/siacs/conversations/xmpp/manager/MultiUserChatManager.java b/src/main/java/eu/siacs/conversations/xmpp/manager/MultiUserChatManager.java index be523106094da92027fa306d89268d025099ba79..4fe8833f7cafa7fb917367a3a081c444d1c957fa 100644 --- a/src/main/java/eu/siacs/conversations/xmpp/manager/MultiUserChatManager.java +++ b/src/main/java/eu/siacs/conversations/xmpp/manager/MultiUserChatManager.java @@ -87,8 +87,7 @@ public class MultiUserChatManager extends AbstractManager { if (Config.MUC_LEAVE_BEFORE_JOIN) { unavailable(conversation); } - conversation.resetMucOptions(); - conversation.getMucOptions().setAutoPushConfiguration(autoPushConfiguration); + conversation.resetMucOptions().setAutoPushConfiguration(autoPushConfiguration); conversation.setHasMessagesLeftOnServer(false); final var disco = fetchDiscoInfo(conversation); diff --git a/src/test/java/eu/siacs/conversations/entities/ConversationGetMucOptionsRaceTest.java b/src/test/java/eu/siacs/conversations/entities/ConversationGetMucOptionsRaceTest.java new file mode 100644 index 0000000000000000000000000000000000000000..72d5977f50554147bb41fd59a4658f61d6283cc6 --- /dev/null +++ b/src/test/java/eu/siacs/conversations/entities/ConversationGetMucOptionsRaceTest.java @@ -0,0 +1,183 @@ +package eu.siacs.conversations.entities; + +import static net.bytebuddy.matcher.ElementMatchers.named; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicReference; + +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.annotation.Config; +import org.robolectric.annotation.ConscryptMode; + +import android.os.Build; +import eu.siacs.conversations.Conversations; +import eu.siacs.conversations.xmpp.Jid; +import junit.framework.Assert; +import net.bytebuddy.ByteBuddy; +import net.bytebuddy.asm.AsmVisitorWrapper; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.dynamic.loading.ClassLoadingStrategy; +import net.bytebuddy.implementation.Implementation; +import net.bytebuddy.jar.asm.MethodVisitor; +import net.bytebuddy.jar.asm.Opcodes; +import net.bytebuddy.jar.asm.Type; +import net.bytebuddy.pool.TypePool; + +@RunWith(RobolectricTestRunner.class) +@Config(sdk = Build.VERSION_CODES.TIRAMISU, application = Conversations.class) +@ConscryptMode(ConscryptMode.Mode.OFF) +public class ConversationGetMucOptionsRaceTest { + + static int fieldReadCount; + static volatile CountDownLatch remainingReads; + static volatile CountDownLatch resetDone; + + public static void gate() { + final var reads = remainingReads; + final var reset = resetDone; + if (reads == null || reset == null) return; + final boolean lastRead = reads.getCount() == 1; + reads.countDown(); + if (lastRead) { + try { + reset.await(); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + } + } + + static class GetMucOptionsInstrumentor extends MethodVisitor { + private int count = 0; + + GetMucOptionsInstrumentor(MethodVisitor delegate) { + super(Opcodes.ASM9, delegate); + } + + @Override + public void visitFieldInsn( + int opcode, String owner, String name, String descriptor + ) { + if (opcode == Opcodes.GETFIELD && "mucOptions".equals(name)) { + count++; + super.visitMethodInsn( + Opcodes.INVOKESTATIC, + Type.getInternalName( + ConversationGetMucOptionsRaceTest.class), + "gate", + "()V", + false + ); + } + super.visitFieldInsn(opcode, owner, name, descriptor); + } + + @Override + public void visitEnd() { + fieldReadCount = count; + super.visitEnd(); + } + } + + @SuppressWarnings("unchecked") + @BeforeClass + public static void instrumentConversation() throws Exception { + Class.forName("net.bytebuddy.agent.ByteBuddyAgent") + .getMethod("install") + .invoke(null); + + final var strategy = (ClassLoadingStrategy) + Class.forName( + "net.bytebuddy.dynamic.loading.ClassReloadingStrategy") + .getMethod("fromInstalledAgent") + .invoke(null); + + new ByteBuddy() + .redefine(Conversation.class) + .visit(new AsmVisitorWrapper.ForDeclaredMethods() + .method( + named("getMucOptions"), + new AsmVisitorWrapper.ForDeclaredMethods + .MethodVisitorWrapper() { + @Override + public MethodVisitor wrap( + TypeDescription instrumentedType, + MethodDescription instrumentedMethod, + MethodVisitor methodVisitor, + Implementation.Context implementationContext, + TypePool typePool, + int writerFlags, + int readerFlags + ) { + return new GetMucOptionsInstrumentor( + methodVisitor); + } + } + ) + ) + .make() + .load(Conversation.class.getClassLoader(), strategy); + } + + @Test + public void testGetMucOptionsNeverReturnsNull() throws Throwable { + final var account = mock(Account.class); + when(account.getJid()).thenReturn( + Jid.ofLocalAndDomain("testAccount", "example.org")); + + final var conversation = new Conversation( + "Test MUC", + account, + Jid.ofLocalAndDomain("testMuc", "example.org"), + Conversation.MODE_MULTI + ); + conversation.getMucOptions(); + + remainingReads = new CountDownLatch(fieldReadCount); + resetDone = new CountDownLatch(1); + + final var result = new AtomicReference(); + final var error = new AtomicReference(); + + Thread reader = new Thread(() -> { + try { + result.set(conversation.getMucOptions()); + } catch (Throwable t) { + error.set(t); + } + }); + + Thread resetter = new Thread(() -> { + try { + remainingReads.await(); + conversation.resetMucOptions(); + resetDone.countDown(); + } catch (Throwable t) { + error.set(t); + } + }); + + reader.start(); + resetter.start(); + + reader.join(10_000); + resetter.join(10_000); + + remainingReads = null; + resetDone = null; + + if (error.get() != null) throw error.get(); + + Assert.assertNotNull( + "getMucOptions() returned null" + + " — the field must not be re-read after the null check", + result.get() + ); + } +}