From c21b5a019c1da00b6d861cd2859e3c349a44b3a7 Mon Sep 17 00:00:00 2001 From: Jeff Brown Date: Mon, 7 Jan 2013 17:15:12 -0800 Subject: [PATCH] Fix cursor window leak when query execution fails. Ensure that the Cursor object is closed if a query on a content provider fails due to an error or is canceled during execution. There are several places in the code where similar problems can occur. To further reduce the likelihood of leaks, close the cursor window immediately when a query fails. Bug: 7278577 Change-Id: I8c686c259de80a162b9086628a817d57f09fdd13 --- .../java/android/content/AsyncTaskLoader.java | 2 +- .../content/ContentProviderNative.java | 20 +++++++++---- .../java/android/content/ContentResolver.java | 12 ++++++-- core/java/android/content/CursorLoader.java | 11 +++++-- .../database/CursorToBulkCursorAdaptor.java | 5 ++++ .../android/database/sqlite/SQLiteCursor.java | 29 ++++++++++++------- 6 files changed, 56 insertions(+), 23 deletions(-) diff --git a/core/java/android/content/AsyncTaskLoader.java b/core/java/android/content/AsyncTaskLoader.java index f9025d90c0019..188c786881564 100644 --- a/core/java/android/content/AsyncTaskLoader.java +++ b/core/java/android/content/AsyncTaskLoader.java @@ -78,7 +78,7 @@ public abstract class AsyncTaskLoader extends Loader { // So we treat this case as an unhandled exception. throw ex; } - if (DEBUG) Slog.v(TAG, this + " <<< doInBackground (was canceled)"); + if (DEBUG) Slog.v(TAG, this + " <<< doInBackground (was canceled)", ex); return null; } } diff --git a/core/java/android/content/ContentProviderNative.java b/core/java/android/content/ContentProviderNative.java index 550a1c984f539..384ba5796714b 100644 --- a/core/java/android/content/ContentProviderNative.java +++ b/core/java/android/content/ContentProviderNative.java @@ -113,13 +113,21 @@ abstract public class ContentProviderNative extends Binder implements IContentPr Cursor cursor = query(url, projection, selection, selectionArgs, sortOrder, cancellationSignal); if (cursor != null) { - CursorToBulkCursorAdaptor adaptor = new CursorToBulkCursorAdaptor( - cursor, observer, getProviderName()); - BulkCursorDescriptor d = adaptor.getBulkCursorDescriptor(); + try { + CursorToBulkCursorAdaptor adaptor = new CursorToBulkCursorAdaptor( + cursor, observer, getProviderName()); + BulkCursorDescriptor d = adaptor.getBulkCursorDescriptor(); + cursor = null; - reply.writeNoException(); - reply.writeInt(1); - d.writeToParcel(reply, Parcelable.PARCELABLE_WRITE_RETURN_VALUE); + reply.writeNoException(); + reply.writeInt(1); + d.writeToParcel(reply, Parcelable.PARCELABLE_WRITE_RETURN_VALUE); + } finally { + // Close cursor if an exception was thrown while constructing the adaptor. + if (cursor != null) { + cursor.close(); + } + } } else { reply.writeNoException(); reply.writeInt(0); diff --git a/core/java/android/content/ContentResolver.java b/core/java/android/content/ContentResolver.java index e34c827d290e4..86bf8c204d29c 100644 --- a/core/java/android/content/ContentResolver.java +++ b/core/java/android/content/ContentResolver.java @@ -381,6 +381,7 @@ public abstract class ContentResolver { return null; } IContentProvider stableProvider = null; + Cursor qCursor = null; try { long startTime = SystemClock.uptimeMillis(); @@ -390,7 +391,6 @@ public abstract class ContentResolver { remoteCancellationSignal = unstableProvider.createCancellationSignal(); cancellationSignal.setRemote(remoteCancellationSignal); } - Cursor qCursor; try { qCursor = unstableProvider.query(uri, projection, selection, selectionArgs, sortOrder, remoteCancellationSignal); @@ -409,20 +409,26 @@ public abstract class ContentResolver { if (qCursor == null) { return null; } - // force query execution + + // Force query execution. Might fail and throw a runtime exception here. qCursor.getCount(); long durationMillis = SystemClock.uptimeMillis() - startTime; maybeLogQueryToEventLog(durationMillis, uri, projection, selection, sortOrder); - // Wrap the cursor object into CursorWrapperInner object + + // Wrap the cursor object into CursorWrapperInner object. CursorWrapperInner wrapper = new CursorWrapperInner(qCursor, stableProvider != null ? stableProvider : acquireProvider(uri)); stableProvider = null; + qCursor = null; return wrapper; } catch (RemoteException e) { // Arbitrary and not worth documenting, as Activity // Manager will kill this process shortly anyway. return null; } finally { + if (qCursor != null) { + qCursor.close(); + } if (unstableProvider != null) { releaseUnstableProvider(unstableProvider); } diff --git a/core/java/android/content/CursorLoader.java b/core/java/android/content/CursorLoader.java index 9f7a1041cf4e0..4e89dec9f02c7 100644 --- a/core/java/android/content/CursorLoader.java +++ b/core/java/android/content/CursorLoader.java @@ -65,9 +65,14 @@ public class CursorLoader extends AsyncTaskLoader { Cursor cursor = getContext().getContentResolver().query(mUri, mProjection, mSelection, mSelectionArgs, mSortOrder, mCancellationSignal); if (cursor != null) { - // Ensure the cursor window is filled - cursor.getCount(); - registerContentObserver(cursor, mObserver); + try { + // Ensure the cursor window is filled. + cursor.getCount(); + registerContentObserver(cursor, mObserver); + } catch (RuntimeException ex) { + cursor.close(); + throw ex; + } } return cursor; } finally { diff --git a/core/java/android/database/CursorToBulkCursorAdaptor.java b/core/java/android/database/CursorToBulkCursorAdaptor.java index 525be96c14980..82a61d4f66a63 100644 --- a/core/java/android/database/CursorToBulkCursorAdaptor.java +++ b/core/java/android/database/CursorToBulkCursorAdaptor.java @@ -132,6 +132,11 @@ public final class CursorToBulkCursorAdaptor extends BulkCursorNative } } + /** + * Returns an object that contains sufficient metadata to reconstruct + * the cursor remotely. May throw if an error occurs when executing the query + * and obtaining the row count. + */ public BulkCursorDescriptor getBulkCursorDescriptor() { synchronized (mLock) { throwIfCursorIsClosed(); diff --git a/core/java/android/database/sqlite/SQLiteCursor.java b/core/java/android/database/sqlite/SQLiteCursor.java index b29897eed97bf..5a1a8e22be88e 100644 --- a/core/java/android/database/sqlite/SQLiteCursor.java +++ b/core/java/android/database/sqlite/SQLiteCursor.java @@ -138,17 +138,26 @@ public class SQLiteCursor extends AbstractWindowedCursor { private void fillWindow(int requiredPos) { clearOrCreateWindow(getDatabase().getPath()); - if (mCount == NO_COUNT) { - int startPos = DatabaseUtils.cursorPickFillWindowStartPosition(requiredPos, 0); - mCount = mQuery.fillWindow(mWindow, startPos, requiredPos, true); - mCursorWindowCapacity = mWindow.getNumRows(); - if (Log.isLoggable(TAG, Log.DEBUG)) { - Log.d(TAG, "received count(*) from native_fill_window: " + mCount); + try { + if (mCount == NO_COUNT) { + int startPos = DatabaseUtils.cursorPickFillWindowStartPosition(requiredPos, 0); + mCount = mQuery.fillWindow(mWindow, startPos, requiredPos, true); + mCursorWindowCapacity = mWindow.getNumRows(); + if (Log.isLoggable(TAG, Log.DEBUG)) { + Log.d(TAG, "received count(*) from native_fill_window: " + mCount); + } + } else { + int startPos = DatabaseUtils.cursorPickFillWindowStartPosition(requiredPos, + mCursorWindowCapacity); + mQuery.fillWindow(mWindow, startPos, requiredPos, false); } - } else { - int startPos = DatabaseUtils.cursorPickFillWindowStartPosition(requiredPos, - mCursorWindowCapacity); - mQuery.fillWindow(mWindow, startPos, requiredPos, false); + } catch (RuntimeException ex) { + // Close the cursor window if the query failed and therefore will + // not produce any results. This helps to avoid accidentally leaking + // the cursor window if the client does not correctly handle exceptions + // and fails to close the cursor. + closeWindow(); + throw ex; } }