Resource leak: BufferedReader not closed in LogFragment.loadAsyncTask.doInBackground

[CyberSecurity-NCC]

Summary

The doInBackground method of the private loadAsyncTask class in LogFragment.java opens a BufferedReader that wraps an InputStreamReader connected to a Process input stream, but never closes it. This results in a file descriptor leak every time the AsyncTask is executed.

Impact

• File descriptor exhaustion: Each execution of this AsyncTask leaks the file descriptor associated with the pipe connected to the logcat process. Common user interactions – such as opening the fragment, manually refreshing the log, or switching between log levels – can trigger repeated executions. Once the per-process file descriptor limit is reached, the app may crash with IOException: Too many open files, fail to launch new processes, or suffer from OutOfMemoryError.
• Zombie processes: Because the Process object is not explicitly destroyed, the leaking stream also keeps a reference to the native process, potentially leaving logcat instances running in the background and compounding the resource drain.

Code Analysis

File: grocy-android/app/src/main/java/xyz/zedler/patrick/grocy/fragment/LogFragment.java
Method: loadAsyncTask.doInBackground(Void...) (approximately line 125)

protected final String doInBackground(Void... params) {
  StringBuilder log = new StringBuilder();
  try {
    Process process = Runtime.getRuntime().exec(logcatCommand);
    BufferedReader bufferedReader = new BufferedReader(
        new InputStreamReader(process.getInputStream())
    );
    String line;
    while ((line = bufferedReader.readLine()) != null) {
      log.append(line).append('\n');
    }
    if (log.length() > 0) log.deleteCharAt(log.length() - 1);
  } catch (IOException ignored) {
  }
  return log.toString();
}

The BufferedReader is opened but never closed – there is no finally block, no explicit close() call, and no try‑with‑resources statement. Because the reference to the stream is discarded immediately after the loop, the file descriptor and the underlying native resources remain open until the garbage collector performs finalization, which is unpredictable.

Suggested Fix

Enclose the BufferedReader in a try‑with‑resources block to guarantee that the stream (and the associated native pipe) is closed in both normal and exceptional paths. Additionally, destroy the Process after reading to avoid accumulating zombie processes.

protected final String doInBackground(Void... params) {
  StringBuilder log = new StringBuilder();
  try {
    Process process = Runtime.getRuntime().exec(logcatCommand);
    try (BufferedReader bufferedReader = new BufferedReader(
             new InputStreamReader(process.getInputStream()))) {
      String line;
      while ((line = bufferedReader.readLine()) != null) {
        log.append(line).append('\n');
      }
    } // BufferedReader is auto-closed here
    if (log.length() > 0) log.deleteCharAt(log.length() - 1);
    process.destroy();
  } catch (IOException ignored) {
  }
  return log.toString();
}

Additional Notes

There are some other resource leak issues:

1. InputStream not closed in RecipeEditViewModel.pasteFromClipboard()

Code Analysis

File: app/src/main/java/xyz/zedler/patrick/grocy/viewmodel/RecipeEditViewModel.java
Method: public void pasteFromClipboard() (around line 356)

try {
    InputStream imageStream = getApplication().getContentResolver().openInputStream(item.getUri());
    Bitmap bitmap = BitmapFactory.decodeStream(imageStream);
    scaleAndUploadBitmap(null, bitmap);
} catch (FileNotFoundException e) {
    e.printStackTrace();
    showMessage(R.string.error_clipboard_no_image);
}

The Android documentation
for BitmapFactory.decodeStream(InputStream) clearly states that the method
does not close the stream. The caller retains ownership and must close it.
Because imageStream is never closed – neither in a finally block nor with
try-with-resources – every invocation of pasteFromClipboard() leaks one input stream.

Suggested Fix

Wrap the InputStream in a try-with-resources block so that the stream (and its
underlying native resources) is always released, even if decoding or upload throws.

try (InputStream imageStream = getApplication().getContentResolver().openInputStream(item.getUri())) {
    Bitmap bitmap = BitmapFactory.decodeStream(imageStream);
    scaleAndUploadBitmap(null, bitmap);
} catch (FileNotFoundException e) {
    e.printStackTrace();
    showMessage(R.string.error_clipboard_no_image);
}

If openInputStream() may also throw other exceptions (e.g., SecurityException,
IOException), consider catching a broader exception or adding a catch for Exception
to make sure the stream is always closed on all error paths.

2. Potential ExecutorService leak in scaleAndUploadBitmap when scaling/converting throws exception

Code Analysis

File: app/src/main/java/.../MasterProductCatOptionalViewModel.java

public void scaleAndUploadBitmap(@Nullable String filePath, @Nullable Bitmap image) {
    // ...
    ExecutorService executor = Executors.newSingleThreadExecutor();
    executor.execute(() -> {
        Bitmap scaledBitmap = filePath != null
                ? PictureUtil.scaleBitmap(filePath)
                : PictureUtil.scaleBitmap(image);
        byte[] imageArray = PictureUtil.convertBitmapToByteArray(scaledBitmap);
        new Handler(Looper.getMainLooper()).post(() -> {
            uploadPicture(imageArray);
            executor.shutdown();
        });
    });
}

The executor.shutdown() call is only reached after both the scaling and conversion steps succeed. If scaleBitmap or convertBitmapToByteArray throws an unhandled exception, the Handler.post block is never executed, and the executor—together with its thread—is abandoned.

Suggested Fix

Wrap the task logic in a try-finally block to guarantee shutdown regardless of success or failure. Handle exceptions gracefully on the main thread to maintain proper UI state:

executor.execute(() -> {
    try {
        Bitmap scaledBitmap = filePath != null
                ? PictureUtil.scaleBitmap(filePath)
                : PictureUtil.scaleBitmap(image);
        byte[] imageArray = PictureUtil.convertBitmapToByteArray(scaledBitmap);
        new Handler(Looper.getMainLooper()).post(() -> {
            uploadPicture(imageArray);
        });
    } catch (Exception e) {
        new Handler(Looper.getMainLooper()).post(() -> {
            // Notify the user, reset loading state, etc.
            showErrorMessage();
            isLoadingLive.setValue(false);
        });
    } finally {
        executor.shutdown();
    }
});

As a further improvement, consider reusing a shared single-thread executor (e.g., a static or injected ExecutorService) instead of creating a new executor for every image upload. This would simplify lifecycle management and reduce thread-creation overhead.

3. ExecutorService leak in RecipeEditViewModel.scaleAndUploadBitmap on failure paths

Code Analysis

File: app/src/main/java/xyz/zedler/patrick/grocy/viewmodel/RecipeEditViewModel.java
Method: scaleAndUploadBitmap(@Nullable String filePath, @Nullable Bitmap image) (around line 378)

ExecutorService executor = Executors.newSingleThreadExecutor();
executor.execute(() -> {
    Bitmap scaledBitmap = filePath != null
        ? PictureUtil.scaleBitmap(filePath)
        : PictureUtil.scaleBitmap(image);
    byte[] imageArray = PictureUtil.convertBitmapToByteArray(scaledBitmap);
    new Handler(Looper.getMainLooper()).post(() -> {
        uploadPicture(imageArray);
        executor.shutdown();
    });
});

If PictureUtil.scaleBitmap or convertBitmapToByteArray throws a runtime exception (e.g. OutOfMemoryError, NullPointerException, or any other unexpected error), the code in the post callback is never executed. Consequently executor.shutdown() is never called, and the executor service remains active in a broken state.

Suggested Fix

Wrap the scaling and conversion logic in a try-finally block so that executor.shutdown() is invoked unconditionally once the task completes. Moving the shutdown out of the main-thread handler also matches the expectation that executor lifecycle management happens on the worker thread.

ExecutorService executor = Executors.newSingleThreadExecutor();
executor.execute(() -> {
    try {
        Bitmap scaledBitmap = filePath != null
            ? PictureUtil.scaleBitmap(filePath)
            : PictureUtil.scaleBitmap(image);
        byte[] imageArray = PictureUtil.convertBitmapToByteArray(scaledBitmap);
        new Handler(Looper.getMainLooper()).post(() -> uploadPicture(imageArray));
    } finally {
        executor.shutdown();
    }
});

This guarantees the executor is always shut down after the background work finishes, regardless of whether an exception occurs.

Context & Acknowledgement

This issue was identified during our academic research on Java resource management. We have manually reviewed this finding to ensure its validity.
Thank you for maintaining this open-source project! We hope this report helps.