Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,10 @@ public void onCreate() {
// load and TermuxActivity handles reloads
mProperties = TermuxAppSharedProperties.getProperties();

mShellManager = TermuxShellManager.getShellManager();
// Use init() instead of getShellManager() so the singleton is lazily created if the
// OS auto-restarted the service after process death (TermuxApplication.onCreate did not
// run, leaving the static singleton null and causing an NPE in buildNotification()).
mShellManager = TermuxShellManager.init(getApplicationContext());
Comment on lines +125 to +128

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Verify that TermuxShellManager.init() is synchronized

# Search for the init method and surrounding lines to check for synchronized keyword
rg -A 5 "public static .*init\(" termux/termux-shared/src/main/java/com/termux/shared/termux/shell/TermuxShellManager.java

Repository: appdevforall/CodeOnTheGo

Length of output: 268


Add synchronization to TermuxShellManager.init() to prevent race conditions.

The init() method in TermuxShellManager is unsynchronized and exhibits a classic double-checked locking anti-pattern. If onCreate() is called concurrently (or from different lifecycle paths), multiple threads could pass the if (shellManager == null) check and create duplicate instances. Add synchronized to the method declaration or use volatile with a proper synchronized block:

public static synchronized TermuxShellManager init(`@NonNull` Context context) {
    if (shellManager == null)
        shellManager = new TermuxShellManager(context);
    return shellManager;
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@termux/termux-app/src/main/java/com/termux/app/TermuxService.java` around
lines 125 - 128, The TermuxShellManager.init() method lacks synchronization,
creating a race condition where multiple threads could bypass the null check and
create duplicate singleton instances. Add the synchronized keyword to the init()
method declaration to ensure thread-safe access to the singleton creation logic.
This will prevent multiple threads from concurrently executing the method and
creating competing instances of the TermuxShellManager.


runStartForeground();

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
package com.termux.app;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;

import com.termux.shared.termux.shell.TermuxShellManager;

import java.lang.reflect.Field;

import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.robolectric.Robolectric;
import org.robolectric.RobolectricTestRunner;
import org.robolectric.android.controller.ServiceController;

/**
* Repro for ADFA-4330: when the OS auto-restarts {@link TermuxService} after process death,
* {@code TermuxApplication.onCreate()} does NOT run, so the static
* {@link TermuxShellManager} singleton is still {@code null}.
*
* <p>On the pre-fix baseline, {@code TermuxService.onCreate()} assigned
* {@code mShellManager = TermuxShellManager.getShellManager()} (which returns the null
* singleton), then immediately called {@code runStartForeground() -> buildNotification() ->
* getTermuxSessionsSize()}, dereferencing the null {@code mShellManager} and throwing a
* {@link NullPointerException}.
*
* <p>The fix changes that assignment to {@code TermuxShellManager.init(applicationContext)},
* which lazily creates the singleton, so the service starts cleanly.
*
* <p>This test simulates the auto-restart by forcing the static singleton back to {@code null}
* before creating the service, then asserts the service comes up and
* {@link TermuxService#getTermuxSessionsSize()} returns 0 instead of NPE-ing.
*/
@RunWith(RobolectricTestRunner.class)
public class TermuxServiceShellManagerNpeTest {

/**
* Reset the static singleton to null to mimic a fresh process where
* TermuxApplication.onCreate() (which would normally call init()) never ran.
*/
@Before
public void clearShellManagerSingleton() throws Exception {
Field f = TermuxShellManager.class.getDeclaredField("shellManager");
f.setAccessible(true);
f.set(null, null);
}

/** Service onCreate() with a null shell-manager singleton must not NPE and must expose usable sessions. */
@Test
public void onCreateWithNullSingleton_doesNotNpe_andSessionsAreUsable() {
// Sanity: the auto-restart precondition — singleton is null going in.
assertEquals(null, TermuxShellManager.getShellManager());

// Drive the REAL service lifecycle. On stage this throws NullPointerException inside
// onCreate() -> runStartForeground() -> buildNotification() -> getTermuxSessionsSize().
ServiceController<TermuxService> controller =
Robolectric.buildService(TermuxService.class).create();
TermuxService service = controller.get();

// After a clean onCreate(), the shell manager must be wired up and queryable.
assertNotNull("mShellManager must be initialized after onCreate()",
TermuxShellManager.getShellManager());
assertEquals("A freshly created service manages zero sessions",
0, service.getTermuxSessionsSize());

controller.destroy();
}
}
Loading