Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit d2183654 authored by Jeff Brown's avatar Jeff Brown
Browse files

Fix ownership of CursorWindows across processes. 

Bug: 5332296

Ensure that there is always an owner for each CursorWindow
and that references to each window are acquired/released
appropriately at all times.

Added synchronization to CursorToBulkCursorAdaptor to
prevent the underlying Cursor and CursorWindow from being
remotely accessed in ways that might violate invariants,
resulting in leaks or other problems.

Ensured that CursorToBulkCursorAdaptor promptly releases
its references to the Cursor and CursorWindow when closed
so they don't stick around longer than they should, even
if the remote end hangs onto the IBulkCursor for some reason.

CursorWindow respects Parcelable.FLAG_WRITE_RETURN_VALUE
as an indication that one reference to the CursorWindow is
being released.  Correspondingly, CursorToBulkCursorAdaptor
acquires a reference to the CursorWindow before returning
it to the caller.  This change also prevents races from
resulting in the transfer of an invalid CursorWindow over
the wire.

Ensured that BulkCursorToCursorAdaptor promptly releases
its reference to the IBulkCursor when closed and throws
on attempts to access the cursor while closed.

Modified ContentProviderNative to handle both parts of
the wrapping and unwrapping of Cursors into IBulkCursors.
This makes it a lot easier to ensure that the right
things happen on both ends.  Also, it turns out that
the only caller of IContentProvider.bulkQuery was
ContentProviderNative itself so there was no need
to support bulkQuery on ContentProviderProxy and it was
just getting in the way.

Implement CloseGuard on CursorWindow.

Change-Id: Ib3c8305d3cc62322f38a06698d404a2989bb6ef9
parent 1d8e7d64

core/java/android/content/ContentProvider.java

+3 −20
Original line number Diff line number Diff line
@@ -22,10 +22,6 @@ import android.content.pm.ProviderInfo; 
import android.content.res.AssetFileDescriptor;

import android.content.res.Configuration;

import android.database.Cursor;

import android.database.CursorToBulkCursorAdaptor;

import android.database.CursorWindow;

import android.database.IBulkCursor;

import android.database.IContentObserver;

import android.database.SQLException;

import android.net.Uri;

import android.os.AsyncTask;
@@ -168,22 +164,9 @@ public abstract class ContentProvider implements ComponentCallbacks2 { 
            return ContentProvider.this;

        }



        /**

         * Remote version of a query, which returns an IBulkCursor. The bulk

         * cursor should be wrapped with BulkCursorToCursorAdaptor before use.

         */

        public IBulkCursor bulkQuery(Uri uri, String[] projection,

                String selection, String[] selectionArgs, String sortOrder,

                IContentObserver observer, CursorWindow window) {

            enforceReadPermission(uri);

            Cursor cursor = ContentProvider.this.query(uri, projection,

                    selection, selectionArgs, sortOrder);

            if (cursor == null) {

                return null;

            }

            return new CursorToBulkCursorAdaptor(cursor, observer,

                    ContentProvider.this.getClass().getName(),

                    hasWritePermission(uri), window);

        @Override

        public String getProviderName() {

            return getContentProvider().getClass().getName();

        }



        public Cursor query(Uri uri, String[] projection,

core/java/android/content/ContentProviderNative.java

+77 −101
Original line number Diff line number Diff line
@@ -20,6 +20,7 @@ import android.content.res.AssetFileDescriptor; 
import android.database.BulkCursorNative;

import android.database.BulkCursorToCursorAdaptor;

import android.database.Cursor;

import android.database.CursorToBulkCursorAdaptor;

import android.database.CursorWindow;

import android.database.DatabaseUtils;

import android.database.IBulkCursor;
@@ -65,6 +66,13 @@ abstract public class ContentProviderNative extends Binder implements IContentPr 
        return new ContentProviderProxy(obj);

    }



    /**

     * Gets the name of the content provider.

     * Should probably be part of the {@link IContentProvider} interface.

     * @return The content provider name.

     */

    public abstract String getProviderName();



    @Override

    public boolean onTransact(int code, Parcel data, Parcel reply, int flags)

            throws RemoteException {
@@ -98,33 +106,23 @@ abstract public class ContentProviderNative extends Binder implements IContentPr 
                    }



                    String sortOrder = data.readString();

                    IContentObserver observer = IContentObserver.Stub.

                        asInterface(data.readStrongBinder());

                    IContentObserver observer = IContentObserver.Stub.asInterface(

                            data.readStrongBinder());

                    CursorWindow window = CursorWindow.CREATOR.createFromParcel(data);



                    // Flag for whether caller wants the number of

                    // rows in the cursor and the position of the

                    // "_id" column index (or -1 if non-existent)

                    // Only to be returned if binder != null.

                    boolean wantsCursorMetadata = data.readInt() != 0;



                    IBulkCursor bulkCursor = bulkQuery(url, projection, selection,

                            selectionArgs, sortOrder, observer, window);

                    if (bulkCursor != null) {

                        final IBinder binder = bulkCursor.asBinder();

                        if (wantsCursorMetadata) {

                            final int count = bulkCursor.count();

                    Cursor cursor = query(url, projection, selection, selectionArgs, sortOrder);

                    if (cursor != null) {

                        CursorToBulkCursorAdaptor adaptor = new CursorToBulkCursorAdaptor(

                                cursor, observer, getProviderName(), window);

                        final IBinder binder = adaptor.asBinder();

                        final int count = adaptor.count();

                        final int index = BulkCursorToCursorAdaptor.findRowIdColumnIndex(

                                    bulkCursor.getColumnNames());

                                adaptor.getColumnNames());



                        reply.writeNoException();

                        reply.writeStrongBinder(binder);

                        reply.writeInt(count);

                        reply.writeInt(index);

                        } else {

                            reply.writeNoException();

                            reply.writeStrongBinder(binder);

                        }

                    } else {

                        reply.writeNoException();

                        reply.writeStrongBinder(null);
@@ -324,12 +322,11 @@ final class ContentProviderProxy implements IContentProvider 
        return mRemote;

    }



    // Like bulkQuery() but sets up provided 'adaptor' if not null.

    private IBulkCursor bulkQueryInternal(

        Uri url, String[] projection,

        String selection, String[] selectionArgs, String sortOrder,

        IContentObserver observer, CursorWindow window,

        BulkCursorToCursorAdaptor adaptor) throws RemoteException {

    public Cursor query(Uri url, String[] projection, String selection,

            String[] selectionArgs, String sortOrder) throws RemoteException {

        CursorWindow window = new CursorWindow(false /* window will be used remotely */);

        try {

            BulkCursorToCursorAdaptor adaptor = new BulkCursorToCursorAdaptor();

            Parcel data = Parcel.obtain();

            Parcel reply = Parcel.obtain();

            try {
@@ -355,61 +352,40 @@ final class ContentProviderProxy implements IContentProvider 
                    data.writeString(selectionArgs[i]);

                }

                data.writeString(sortOrder);

            data.writeStrongBinder(observer.asBinder());

                data.writeStrongBinder(adaptor.getObserver().asBinder());

                window.writeToParcel(data, 0);



            // Flag for whether or not we want the number of rows in the

            // cursor and the position of the "_id" column index (or -1 if

            // non-existent).  Only to be returned if binder != null.

            final boolean wantsCursorMetadata = (adaptor != null);

            data.writeInt(wantsCursorMetadata ? 1 : 0);



                mRemote.transact(IContentProvider.QUERY_TRANSACTION, data, reply, 0);



                DatabaseUtils.readExceptionFromParcel(reply);



            IBulkCursor bulkCursor = null;

            IBinder bulkCursorBinder = reply.readStrongBinder();

            if (bulkCursorBinder != null) {

                bulkCursor = BulkCursorNative.asInterface(bulkCursorBinder);



                if (wantsCursorMetadata) {

                IBulkCursor bulkCursor = BulkCursorNative.asInterface(reply.readStrongBinder());

                if (bulkCursor != null) {

                    int rowCount = reply.readInt();

                    int idColumnPosition = reply.readInt();

                    if (bulkCursor != null) {

                        adaptor.set(bulkCursor, rowCount, idColumnPosition);

                    }

                }

                    adaptor.initialize(bulkCursor, rowCount, idColumnPosition);

                } else {

                    adaptor.close();

                    adaptor = null;

                }

            return bulkCursor;

                return adaptor;

            } catch (RemoteException ex) {

                adaptor.close();

                throw ex;

            } catch (RuntimeException ex) {

                adaptor.close();

                throw ex;

            } finally {

                data.recycle();

                reply.recycle();

            }

        } finally {

            // We close the window now because the cursor adaptor does not

            // take ownership of the window until the first call to onMove.

            // The adaptor will obtain a fresh reference to the window when

            // it is filled.

            window.close();

        }



    public IBulkCursor bulkQuery(Uri url, String[] projection,

            String selection, String[] selectionArgs, String sortOrder, IContentObserver observer,

            CursorWindow window) throws RemoteException {

        return bulkQueryInternal(

            url, projection, selection, selectionArgs, sortOrder,

            observer, window,

            null /* BulkCursorToCursorAdaptor */);

    }



    public Cursor query(Uri url, String[] projection, String selection,

            String[] selectionArgs, String sortOrder) throws RemoteException {

        //TODO make a pool of windows so we can reuse memory dealers

        CursorWindow window = new CursorWindow(false /* window will be used remotely */);

        BulkCursorToCursorAdaptor adaptor = new BulkCursorToCursorAdaptor();

        IBulkCursor bulkCursor = bulkQueryInternal(

            url, projection, selection, selectionArgs, sortOrder,

            adaptor.getObserver(), window,

            adaptor);

        if (bulkCursor == null) {

            return null;

        }

        return adaptor;

    }



    public String getType(Uri url) throws RemoteException

core/java/android/content/IContentProvider.java

+0 −10
Original line number Diff line number Diff line
@@ -18,9 +18,6 @@ package android.content; 


import android.content.res.AssetFileDescriptor;

import android.database.Cursor;

import android.database.CursorWindow;

import android.database.IBulkCursor;

import android.database.IContentObserver;

import android.net.Uri;

import android.os.Bundle;

import android.os.IBinder;
@@ -36,13 +33,6 @@ import java.util.ArrayList; 
 * @hide

 */

public interface IContentProvider extends IInterface {

    /**

     * @hide - hide this because return type IBulkCursor and parameter

     * IContentObserver are system private classes.

     */

    public IBulkCursor bulkQuery(Uri url, String[] projection,

            String selection, String[] selectionArgs, String sortOrder, IContentObserver observer,

            CursorWindow window) throws RemoteException;

    public Cursor query(Uri url, String[] projection, String selection,

            String[] selectionArgs, String sortOrder) throws RemoteException;

    public String getType(Uri url) throws RemoteException;

core/java/android/database/AbstractCursor.java

+4 −6
Original line number Diff line number Diff line
@@ -78,13 +78,11 @@ public abstract class AbstractCursor implements CrossProcessCursor { 
    }



    public void deactivate() {

        deactivateInternal();

        onDeactivateOrClose();

    }



    /**

     * @hide

     */

    public void deactivateInternal() {

    /** @hide */

    protected void onDeactivateOrClose() {

        if (mSelfObserver != null) {

            mContentResolver.unregisterContentObserver(mSelfObserver);

            mSelfObserverRegistered = false;
@@ -108,7 +106,7 @@ public abstract class AbstractCursor implements CrossProcessCursor { 
    public void close() {

        mClosed = true;

        mContentObservable.unregisterAll();

        deactivateInternal();

        onDeactivateOrClose();

    }



    /**

core/java/android/database/AbstractWindowedCursor.java

+26 −0
Original line number Diff line number Diff line
@@ -19,6 +19,11 @@ package android.database; 
/**

 * A base class for Cursors that store their data in {@link CursorWindow}s.

 * <p>

 * The cursor owns the cursor window it uses.  When the cursor is closed,

 * its window is also closed.  Likewise, when the window used by the cursor is

 * changed, its old window is closed.  This policy of strict ownership ensures

 * that cursor windows are not leaked.

 * </p><p>

 * Subclasses are responsible for filling the cursor window with data during

 * {@link #onMove(int, int)}, allocating a new cursor window if necessary.

 * During {@link #requery()}, the existing cursor window should be cleared and
@@ -180,4 +185,25 @@ public abstract class AbstractWindowedCursor extends AbstractCursor { 
            mWindow = null;

        }

    }



    /**

     * If there is a window, clear it.

     * Otherwise, creates a local window.

     * @hide

     */

    protected void clearOrCreateLocalWindow() {

        if (mWindow == null) {

            // If there isn't a window set already it will only be accessed locally

            mWindow = new CursorWindow(true /* the window is local only */);

        } else {

            mWindow.clear();

        }

    }



    /** @hide */

    @Override

    protected void onDeactivateOrClose() {

        super.onDeactivateOrClose();

        closeWindow();

    }

}