From 4a3d7ed45d98ad2fe900221755845b87f26b554a Mon Sep 17 00:00:00 2001 From: Mike Lockwood Date: Fri, 6 Mar 2015 09:18:09 -0800 Subject: [PATCH] MIDI Manager: Add explicit close mechanism for input and output ports Relying on errors from closing the file descriptor is not reliable and was resulting in file descriptor leaks in device servers. Change-Id: Ib5cc22dba493eae6608a12cc6d4178d8390da77b --- .../android/media/midi/IMidiDeviceServer.aidl | 5 +- media/java/android/media/midi/MidiDevice.java | 18 +-- .../android/media/midi/MidiDeviceServer.java | 118 ++++++++++++------ .../android/media/midi/MidiInputPort.java | 40 +++++- .../android/media/midi/MidiOutputPort.java | 38 +++++- 5 files changed, 172 insertions(+), 47 deletions(-) diff --git a/media/java/android/media/midi/IMidiDeviceServer.aidl b/media/java/android/media/midi/IMidiDeviceServer.aidl index 71914ad5892d7..3331aae649bf9 100644 --- a/media/java/android/media/midi/IMidiDeviceServer.aidl +++ b/media/java/android/media/midi/IMidiDeviceServer.aidl @@ -21,6 +21,7 @@ import android.os.ParcelFileDescriptor; /** @hide */ interface IMidiDeviceServer { - ParcelFileDescriptor openInputPort(int portNumber); - ParcelFileDescriptor openOutputPort(int portNumber); + ParcelFileDescriptor openInputPort(IBinder token, int portNumber); + ParcelFileDescriptor openOutputPort(IBinder token, int portNumber); + void closePort(IBinder token); } diff --git a/media/java/android/media/midi/MidiDevice.java b/media/java/android/media/midi/MidiDevice.java index a273915039ae5..1a39485b5e47b 100644 --- a/media/java/android/media/midi/MidiDevice.java +++ b/media/java/android/media/midi/MidiDevice.java @@ -18,6 +18,8 @@ package android.media.midi; import android.content.Context; import android.content.ServiceConnection; +import android.os.Binder; +import android.os.IBinder; import android.os.ParcelFileDescriptor; import android.os.RemoteException; import android.util.Log; @@ -36,13 +38,13 @@ public final class MidiDevice implements Closeable { private static final String TAG = "MidiDevice"; private final MidiDeviceInfo mDeviceInfo; - private final IMidiDeviceServer mServer; + private final IMidiDeviceServer mDeviceServer; private Context mContext; private ServiceConnection mServiceConnection; /* package */ MidiDevice(MidiDeviceInfo deviceInfo, IMidiDeviceServer server) { mDeviceInfo = deviceInfo; - mServer = server; + mDeviceServer = server; mContext = null; mServiceConnection = null; } @@ -50,7 +52,7 @@ public final class MidiDevice implements Closeable { /* package */ MidiDevice(MidiDeviceInfo deviceInfo, IMidiDeviceServer server, Context context, ServiceConnection serviceConnection) { mDeviceInfo = deviceInfo; - mServer = server; + mDeviceServer = server; mContext = context; mServiceConnection = serviceConnection; } @@ -72,11 +74,12 @@ public final class MidiDevice implements Closeable { */ public MidiInputPort openInputPort(int portNumber) { try { - ParcelFileDescriptor pfd = mServer.openInputPort(portNumber); + IBinder token = new Binder(); + ParcelFileDescriptor pfd = mDeviceServer.openInputPort(token, portNumber); if (pfd == null) { return null; } - return new MidiInputPort(pfd, portNumber); + return new MidiInputPort(mDeviceServer, token, pfd, portNumber); } catch (RemoteException e) { Log.e(TAG, "RemoteException in openInputPort"); return null; @@ -91,11 +94,12 @@ public final class MidiDevice implements Closeable { */ public MidiOutputPort openOutputPort(int portNumber) { try { - ParcelFileDescriptor pfd = mServer.openOutputPort(portNumber); + IBinder token = new Binder(); + ParcelFileDescriptor pfd = mDeviceServer.openOutputPort(token, portNumber); if (pfd == null) { return null; } - return new MidiOutputPort(pfd, portNumber); + return new MidiOutputPort(mDeviceServer, token, pfd, portNumber); } catch (RemoteException e) { Log.e(TAG, "RemoteException in openOutputPort"); return null; diff --git a/media/java/android/media/midi/MidiDeviceServer.java b/media/java/android/media/midi/MidiDeviceServer.java index 87fb3c5eccf38..4d59c6395faf5 100644 --- a/media/java/android/media/midi/MidiDeviceServer.java +++ b/media/java/android/media/midi/MidiDeviceServer.java @@ -28,6 +28,7 @@ import libcore.io.IoUtils; import java.io.Closeable; import java.io.IOException; +import java.util.HashMap; /** * Internal class used for providing an implementation for a MIDI device. @@ -53,11 +54,68 @@ public final class MidiDeviceServer implements Closeable { // MidiOutputPorts for clients connected to our input ports private final MidiOutputPort[] mInputPortOutputPorts; + abstract private class PortClient implements IBinder.DeathRecipient { + final IBinder mToken; + + PortClient(IBinder token) { + mToken = token; + + try { + token.linkToDeath(this, 0); + } catch (RemoteException e) { + close(); + } + } + + abstract void close(); + + @Override + public void binderDied() { + close(); + } + } + + private class InputPortClient extends PortClient { + private final MidiOutputPort mOutputPort; + + InputPortClient(IBinder token, MidiOutputPort outputPort) { + super(token); + mOutputPort = outputPort; + } + + @Override + void close() { + mToken.unlinkToDeath(this, 0); + synchronized (mInputPortOutputPorts) { + mInputPortOutputPorts[mOutputPort.getPortNumber()] = null; + } + IoUtils.closeQuietly(mOutputPort); + } + } + + private class OutputPortClient extends PortClient { + private final MidiInputPort mInputPort; + + OutputPortClient(IBinder token, MidiInputPort inputPort) { + super(token); + mInputPort = inputPort; + } + + @Override + void close() { + mToken.unlinkToDeath(this, 0); + mOutputPortDispatchers[mInputPort.getPortNumber()].getSender().disconnect(mInputPort); + IoUtils.closeQuietly(mInputPort); + } + } + + private final HashMap mPortClients = new HashMap(); + // Binder interface stub for receiving connection requests from clients private final IMidiDeviceServer mServer = new IMidiDeviceServer.Stub() { @Override - public ParcelFileDescriptor openInputPort(int portNumber) { + public ParcelFileDescriptor openInputPort(IBinder token, int portNumber) { if (mDeviceInfo.isPrivate()) { if (Binder.getCallingUid() != Process.myUid()) { throw new SecurityException("Can't access private device from different UID"); @@ -78,25 +136,13 @@ public final class MidiDeviceServer implements Closeable { try { ParcelFileDescriptor[] pair = ParcelFileDescriptor.createSocketPair( OsConstants.SOCK_SEQPACKET); - final MidiOutputPort outputPort = new MidiOutputPort(pair[0], portNumber); + MidiOutputPort outputPort = new MidiOutputPort(pair[0], portNumber); mInputPortOutputPorts[portNumber] = outputPort; - final int portNumberF = portNumber; - final MidiReceiver inputPortReceviver = mInputPortReceivers[portNumber]; - - outputPort.connect(new MidiReceiver() { - @Override - public void receive(byte[] msg, int offset, int count, long timestamp) - throws IOException { - try { - inputPortReceviver.receive(msg, offset, count, timestamp); - } catch (IOException e) { - IoUtils.closeQuietly(mInputPortOutputPorts[portNumberF]); - mInputPortOutputPorts[portNumberF] = null; - // FIXME also flush the receiver - } - } - }); - + outputPort.connect(mInputPortReceivers[portNumber]); + InputPortClient client = new InputPortClient(token, outputPort); + synchronized (mPortClients) { + mPortClients.put(token, client); + } return pair[1]; } catch (IOException e) { Log.e(TAG, "unable to create ParcelFileDescriptors in openInputPort"); @@ -106,7 +152,7 @@ public final class MidiDeviceServer implements Closeable { } @Override - public ParcelFileDescriptor openOutputPort(int portNumber) { + public ParcelFileDescriptor openOutputPort(IBinder token, int portNumber) { if (mDeviceInfo.isPrivate()) { if (Binder.getCallingUid() != Process.myUid()) { throw new SecurityException("Can't access private device from different UID"); @@ -121,28 +167,28 @@ public final class MidiDeviceServer implements Closeable { try { ParcelFileDescriptor[] pair = ParcelFileDescriptor.createSocketPair( OsConstants.SOCK_SEQPACKET); - final MidiInputPort inputPort = new MidiInputPort(pair[0], portNumber); - final MidiSender sender = mOutputPortDispatchers[portNumber].getSender(); - sender.connect(new MidiReceiver() { - @Override - public void receive(byte[] msg, int offset, int count, long timestamp) - throws IOException { - try { - inputPort.receive(msg, offset, count, timestamp); - } catch (IOException e) { - IoUtils.closeQuietly(inputPort); - sender.disconnect(this); - // FIXME also flush the receiver? - } - } - }); - + MidiInputPort inputPort = new MidiInputPort(pair[0], portNumber); + mOutputPortDispatchers[portNumber].getSender().connect(inputPort); + OutputPortClient client = new OutputPortClient(token, inputPort); + synchronized (mPortClients) { + mPortClients.put(token, client); + } return pair[1]; } catch (IOException e) { Log.e(TAG, "unable to create ParcelFileDescriptors in openOutputPort"); return null; } } + + @Override + public void closePort(IBinder token) { + synchronized (mPortClients) { + PortClient client = mPortClients.remove(token); + if (client != null) { + client.close(); + } + } + } }; /* package */ MidiDeviceServer(IMidiManager midiManager, MidiReceiver[] inputPortReceivers, diff --git a/media/java/android/media/midi/MidiInputPort.java b/media/java/android/media/midi/MidiInputPort.java index fb70e8927b093..5d944cb30da70 100644 --- a/media/java/android/media/midi/MidiInputPort.java +++ b/media/java/android/media/midi/MidiInputPort.java @@ -16,7 +16,12 @@ package android.media.midi; +import android.os.IBinder; import android.os.ParcelFileDescriptor; +import android.os.RemoteException; +import android.util.Log; + +import dalvik.system.CloseGuard; import libcore.io.IoUtils; @@ -31,16 +36,29 @@ import java.io.IOException; * @hide */ public class MidiInputPort extends MidiReceiver implements Closeable { + private static final String TAG = "MidiInputPort"; + private final IMidiDeviceServer mDeviceServer; + private final IBinder mToken; private final int mPortNumber; private final FileOutputStream mOutputStream; + private final CloseGuard mGuard = CloseGuard.get(); + // buffer to use for sending data out our output stream private final byte[] mBuffer = new byte[MidiPortImpl.MAX_PACKET_SIZE]; - /* package */ MidiInputPort(ParcelFileDescriptor pfd, int portNumber) { + /* package */ MidiInputPort(IMidiDeviceServer server, IBinder token, + ParcelFileDescriptor pfd, int portNumber) { + mDeviceServer = server; + mToken = token; mPortNumber = portNumber; mOutputStream = new ParcelFileDescriptor.AutoCloseOutputStream(pfd); + mGuard.open("close"); + } + + /* package */ MidiInputPort(ParcelFileDescriptor pfd, int portNumber) { + this(null, null, pfd, portNumber); } /** @@ -79,6 +97,26 @@ public class MidiInputPort extends MidiReceiver implements Closeable { @Override public void close() throws IOException { + mGuard.close(); mOutputStream.close(); + if (mDeviceServer != null) { + try { + mDeviceServer.closePort(mToken); + } catch (RemoteException e) { + Log.e(TAG, "RemoteException in MidiInputPort.close()"); + } + } + } + + @Override + protected void finalize() throws Throwable { + try { + if (mGuard != null) { + mGuard.warnIfOpen(); + } + close(); + } finally { + super.finalize(); + } } } diff --git a/media/java/android/media/midi/MidiOutputPort.java b/media/java/android/media/midi/MidiOutputPort.java index 8b99e9079ba63..d46b2028fbb1e 100644 --- a/media/java/android/media/midi/MidiOutputPort.java +++ b/media/java/android/media/midi/MidiOutputPort.java @@ -16,9 +16,13 @@ package android.media.midi; +import android.os.IBinder; import android.os.ParcelFileDescriptor; +import android.os.RemoteException; import android.util.Log; +import dalvik.system.CloseGuard; + import libcore.io.IoUtils; import java.io.Closeable; @@ -34,10 +38,14 @@ import java.io.IOException; public class MidiOutputPort extends MidiSender implements Closeable { private static final String TAG = "MidiOutputPort"; + private final IMidiDeviceServer mDeviceServer; + private final IBinder mToken; private final int mPortNumber; private final FileInputStream mInputStream; private final MidiDispatcher mDispatcher = new MidiDispatcher(); + private final CloseGuard mGuard = CloseGuard.get(); + // This thread reads MIDI events from a socket and distributes them to the list of // MidiReceivers attached to this device. private final Thread mThread = new Thread() { @@ -70,10 +78,18 @@ public class MidiOutputPort extends MidiSender implements Closeable { } }; - /* package */ MidiOutputPort(ParcelFileDescriptor pfd, int portNumber) { + /* package */ MidiOutputPort(IMidiDeviceServer server, IBinder token, + ParcelFileDescriptor pfd, int portNumber) { + mDeviceServer = server; + mToken = token; mPortNumber = portNumber; mInputStream = new ParcelFileDescriptor.AutoCloseInputStream(pfd); mThread.start(); + mGuard.open("close"); + } + + /* package */ MidiOutputPort(ParcelFileDescriptor pfd, int portNumber) { + this(null, null, pfd, portNumber); } /** @@ -97,6 +113,26 @@ public class MidiOutputPort extends MidiSender implements Closeable { @Override public void close() throws IOException { + mGuard.close(); mInputStream.close(); + if (mDeviceServer != null) { + try { + mDeviceServer.closePort(mToken); + } catch (RemoteException e) { + Log.e(TAG, "RemoteException in MidiOutputPort.close()"); + } + } + } + + @Override + protected void finalize() throws Throwable { + try { + if (mGuard != null) { + mGuard.warnIfOpen(); + } + close(); + } finally { + super.finalize(); + } } }