Android/frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

FileBridge: fix fd ownership mismanagement.

Browse Source 
This patch switches everything over to ParcelFileDescriptor, but the
important part of the change is changing FileBridge.getClientSocket to
return a ParcelFileDescriptor. Previously, it returned a raw
FileDescriptor that was closed by FileBridge, and the only non-test
caller of that function was taking it and constructing a
ParcelFileDescriptor from it, which would also attempt to close the fd,
leading to an fdsan abort.

Bug: http://b/162811367
Test: atest FileBridgeTest
Test: treehugger
Change-Id: I724ea7601bb072c98895f68abc08bb0e339d1db0
This commit is contained in:
Josh Gao
2020-09-21 14:32:42 -07:00
parent c26848e7bd
commit 4c392e8057
3 changed files with 53 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files

41
core/java/android/os/FileBridge.java
View File

@@ -16,7 +16,6 @@
package android.os;
import static android.system.OsConstants.AF_UNIX;
import static android.system.OsConstants.SOCK_STREAM;
import android.system.ErrnoException;
@@ -58,17 +57,19 @@ public class FileBridge extends Thread {
/** CMD_CLOSE */
private static final int CMD_CLOSE = 3;
private FileDescriptor mTarget;
private ParcelFileDescriptor mTarget;
private final FileDescriptor mServer = new FileDescriptor();
private final FileDescriptor mClient = new FileDescriptor();
private ParcelFileDescriptor mServer;
private ParcelFileDescriptor mClient;
private volatile boolean mClosed;
public FileBridge() {
try {
Os.socketpair(AF_UNIX, SOCK_STREAM, 0, mServer, mClient);
} catch (ErrnoException e) {
ParcelFileDescriptor[] fds = ParcelFileDescriptor.createSocketPair(SOCK_STREAM);
mServer = fds[0];
mClient = fds[1];
} catch (IOException e) {
throw new RuntimeException("Failed to create bridge");
}
}
@@ -80,15 +81,14 @@ public class FileBridge extends Thread {
public void forceClose() {
IoUtils.closeQuietly(mTarget);
IoUtils.closeQuietly(mServer);
IoUtils.closeQuietly(mClient);
mClosed = true;
}
public void setTargetFile(FileDescriptor target) {
public void setTargetFile(ParcelFileDescriptor target) {
mTarget = target;
}
public FileDescriptor getClientSocket() {
public ParcelFileDescriptor getClientSocket() {
return mClient;
}
@@ -96,32 +96,33 @@ public class FileBridge extends Thread {
public void run() {
final byte[] temp = new byte[8192];
try {
while (IoBridge.read(mServer, temp, 0, MSG_LENGTH) == MSG_LENGTH) {
while (IoBridge.read(mServer.getFileDescriptor(), temp, 0, MSG_LENGTH) == MSG_LENGTH) {
final int cmd = Memory.peekInt(temp, 0, ByteOrder.BIG_ENDIAN);
if (cmd == CMD_WRITE) {
// Shuttle data into local file
int len = Memory.peekInt(temp, 4, ByteOrder.BIG_ENDIAN);
while (len > 0) {
int n = IoBridge.read(mServer, temp, 0, Math.min(temp.length, len));
int n = IoBridge.read(mServer.getFileDescriptor(), temp, 0,
Math.min(temp.length, len));
if (n == -1) {
throw new IOException(
"Unexpected EOF; still expected " + len + " bytes");
}
IoBridge.write(mTarget, temp, 0, n);
IoBridge.write(mTarget.getFileDescriptor(), temp, 0, n);
len -= n;
}
} else if (cmd == CMD_FSYNC) {
// Sync and echo back to confirm
Os.fsync(mTarget);
IoBridge.write(mServer, temp, 0, MSG_LENGTH);
Os.fsync(mTarget.getFileDescriptor());
IoBridge.write(mServer.getFileDescriptor(), temp, 0, MSG_LENGTH);
} else if (cmd == CMD_CLOSE) {
// Close and echo back to confirm
Os.fsync(mTarget);
Os.close(mTarget);
Os.fsync(mTarget.getFileDescriptor());
mTarget.close();
mClosed = true;
IoBridge.write(mServer, temp, 0, MSG_LENGTH);
IoBridge.write(mServer.getFileDescriptor(), temp, 0, MSG_LENGTH);
break;
}
}
@@ -143,17 +144,11 @@ public class FileBridge extends Thread {
mClient = clientPfd.getFileDescriptor();
}
public FileBridgeOutputStream(FileDescriptor client) {
mClientPfd = null;
mClient = client;
}
@Override
public void close() throws IOException {
try {
writeCommandAndBlock(CMD_CLOSE, "close()");
} finally {
IoBridge.closeAndSignalBlockedThreads(mClient);
IoUtils.closeQuietly(mClientPfd);
}
}

12
core/tests/coretests/src/android/os/FileBridgeTest.java
View File

@@ -16,6 +16,9 @@
package android.os;
import static android.os.ParcelFileDescriptor.MODE_CREATE;
import static android.os.ParcelFileDescriptor.MODE_READ_WRITE;
import android.os.FileBridge.FileBridgeOutputStream;
import android.test.AndroidTestCase;
import android.test.MoreAsserts;
@@ -25,7 +28,6 @@ import libcore.io.Streams;
import java.io.ByteArrayOutputStream;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.Random;
@@ -33,7 +35,7 @@ import java.util.Random;
public class FileBridgeTest extends AndroidTestCase {
private File file;
private FileOutputStream fileOs;
private ParcelFileDescriptor outputFile;
private FileBridge bridge;
private FileBridgeOutputStream client;
@@ -44,17 +46,17 @@ public class FileBridgeTest extends AndroidTestCase {
file = getContext().getFileStreamPath("meow.dat");
file.delete();
fileOs = new FileOutputStream(file);
outputFile = ParcelFileDescriptor.open(file, MODE_CREATE | MODE_READ_WRITE);
bridge = new FileBridge();
bridge.setTargetFile(fileOs.getFD());
bridge.setTargetFile(outputFile);
bridge.start();
client = new FileBridgeOutputStream(bridge.getClientSocket());
}
@Override
protected void tearDown() throws Exception {
fileOs.close();
outputFile.close();
file.delete();
}

40
services/core/java/com/android/server/pm/PackageInstallerSession.java
View File

@@ -943,6 +943,23 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
}
}
private ParcelFileDescriptor openTargetInternal(String path, int flags, int mode)
throws IOException, ErrnoException {
// TODO: this should delegate to DCS so the system process avoids
// holding open FDs into containers.
final FileDescriptor fd = Os.open(path, flags, mode);
return new ParcelFileDescriptor(fd);
}
private ParcelFileDescriptor createRevocableFdInternal(RevocableFileDescriptor fd,
ParcelFileDescriptor pfd) throws IOException {
int releasedFdInt = pfd.detachFd();
FileDescriptor releasedFd = new FileDescriptor();
releasedFd.setInt$(releasedFdInt);
fd.init(mContext, releasedFd);
return fd.getRevocableFileDescriptor();
}
private ParcelFileDescriptor doWriteInternal(String name, long offsetBytes, long lengthBytes,
ParcelFileDescriptor incomingFd) throws IOException {
// Quick sanity check of state, and allocate a pipe for ourselves. We
@@ -975,21 +992,20 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
Binder.restoreCallingIdentity(identity);
}
// TODO: this should delegate to DCS so the system process avoids
// holding open FDs into containers.
final FileDescriptor targetFd = Os.open(target.getAbsolutePath(),
ParcelFileDescriptor targetPfd = openTargetInternal(target.getAbsolutePath(),
O_CREAT | O_WRONLY, 0644);
Os.chmod(target.getAbsolutePath(), 0644);
// If caller specified a total length, allocate it for them. Free up
// cache space to grow, if needed.
if (stageDir != null && lengthBytes > 0) {
mContext.getSystemService(StorageManager.class).allocateBytes(targetFd, lengthBytes,
mContext.getSystemService(StorageManager.class).allocateBytes(
targetPfd.getFileDescriptor(), lengthBytes,
PackageHelper.translateAllocateFlags(params.installFlags));
}
if (offsetBytes > 0) {
Os.lseek(targetFd, offsetBytes, OsConstants.SEEK_SET);
Os.lseek(targetPfd.getFileDescriptor(), offsetBytes, OsConstants.SEEK_SET);
}
if (incomingFd != null) {
@@ -999,8 +1015,9 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
// inserted above to hold the session active.
try {
final Int64Ref last = new Int64Ref(0);
FileUtils.copy(incomingFd.getFileDescriptor(), targetFd, lengthBytes, null,
Runnable::run, (long progress) -> {
FileUtils.copy(incomingFd.getFileDescriptor(), targetPfd.getFileDescriptor(),
lengthBytes, null, Runnable::run,
(long progress) -> {
if (params.sizeBytes > 0) {
final long delta = progress - last.value;
last.value = progress;
@@ -1011,7 +1028,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
}
});
} finally {
IoUtils.closeQuietly(targetFd);
IoUtils.closeQuietly(targetPfd);
IoUtils.closeQuietly(incomingFd);
// We're done here, so remove the "bridge" that was holding
@@ -1027,12 +1044,11 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
}
return null;
} else if (PackageInstaller.ENABLE_REVOCABLE_FD) {
fd.init(mContext, targetFd);
return fd.getRevocableFileDescriptor();
return createRevocableFdInternal(fd, targetPfd);
} else {
bridge.setTargetFile(targetFd);
bridge.setTargetFile(targetPfd);
bridge.start();
return new ParcelFileDescriptor(bridge.getClientSocket());
return bridge.getClientSocket();
}
} catch (ErrnoException e) {