From bbe06e7b598e5249e40c06b6bf2c563ad94e00f5 Mon Sep 17 00:00:00 2001 From: Li Li Date: Wed, 28 Jul 2021 11:27:26 -0700 Subject: [PATCH] Freezer: fix exception when parsing /proc/locks The original code is based on /proc/locks manpage. Unfortunately, that manpage is never correctly updated to include the new field for blocked locks. The new code detects the extra field. As StringTokenizer is too heavy, ProcFileReader is used for better performance. Bug: 194756340 Test: FrameworksUtilTests com.android.internal.util.ProcFileReaderTest Test: FrameworksCoreTests com.android.internal.os.ProcLocksReaderTest Test: No /proc/locks parsing exception with blocked locks Change-Id: I4c9763f9d3091f7d84d2e4b672d7e5cb78b33f59 --- core/java/android/os/Process.java | 42 --------- .../android/internal/os/ProcLocksReader.java | 89 ++++++++++++++++++ .../android/internal/util/ProcFileReader.java | 43 +++++++-- .../internal/os/ProcLocksReaderTest.java | 90 +++++++++++++++++++ .../internal/util/ProcFileReaderTest.java | 40 +++++++++ .../android/server/am/CachedAppOptimizer.java | 7 +- 6 files changed, 261 insertions(+), 50 deletions(-) create mode 100644 core/java/com/android/internal/os/ProcLocksReader.java create mode 100644 core/tests/coretests/src/com/android/internal/os/ProcLocksReaderTest.java diff --git a/core/java/android/os/Process.java b/core/java/android/os/Process.java index 6bca336dae917..9f37c4877199d 100644 --- a/core/java/android/os/Process.java +++ b/core/java/android/os/Process.java @@ -34,12 +34,9 @@ import dalvik.system.VMRuntime; import libcore.io.IoUtils; -import java.io.BufferedReader; import java.io.FileDescriptor; -import java.io.FileReader; import java.io.IOException; import java.util.Map; -import java.util.StringTokenizer; import java.util.concurrent.TimeoutException; /** @@ -1472,43 +1469,4 @@ public class Process { } private static native int nativePidFdOpen(int pid, int flags) throws ErrnoException; - - /** - * Checks if a process corresponding to a specific pid owns any file locks. - * @param pid The process ID for which we want to know the existence of file locks. - * @return true If the process holds any file locks, false otherwise. - * @throws IOException if /proc/locks can't be accessed. - * - * @hide - */ - public static boolean hasFileLocks(int pid) throws Exception { - BufferedReader br = null; - - try { - br = new BufferedReader(new FileReader("/proc/locks")); - String line; - - while ((line = br.readLine()) != null) { - StringTokenizer st = new StringTokenizer(line); - - for (int i = 0; i < 5 && st.hasMoreTokens(); i++) { - String str = st.nextToken(); - try { - if (i == 4 && Integer.parseInt(str) == pid) { - return true; - } - } catch (NumberFormatException nfe) { - throw new Exception("Exception parsing /proc/locks at \" " - + line + " \", token #" + i); - } - } - } - - return false; - } finally { - if (br != null) { - br.close(); - } - } - } } diff --git a/core/java/com/android/internal/os/ProcLocksReader.java b/core/java/com/android/internal/os/ProcLocksReader.java new file mode 100644 index 0000000000000..bd3115fc5d4c7 --- /dev/null +++ b/core/java/com/android/internal/os/ProcLocksReader.java @@ -0,0 +1,89 @@ +/* + * Copyright (C) 2021 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.internal.os; + +import com.android.internal.util.ProcFileReader; + +import libcore.io.IoUtils; + +import java.io.FileInputStream; +import java.io.IOException; + +/** + * Reads and parses {@code locks} files in the {@code proc} filesystem. + * A typical example of /proc/locks + * + * 1: POSIX ADVISORY READ 18403 fd:09:9070 1073741826 1073742335 + * 2: POSIX ADVISORY WRITE 18292 fd:09:34062 0 EOF + * 2: -> POSIX ADVISORY WRITE 18291 fd:09:34062 0 EOF + * 2: -> POSIX ADVISORY WRITE 18293 fd:09:34062 0 EOF + * 3: POSIX ADVISORY READ 3888 fd:09:13992 128 128 + * 4: POSIX ADVISORY READ 3888 fd:09:14230 1073741826 1073742335 + */ +public class ProcLocksReader { + private final String mPath; + + public ProcLocksReader() { + mPath = "/proc/locks"; + } + + public ProcLocksReader(String path) { + mPath = path; + } + + /** + * Checks if a process corresponding to a specific pid owns any file locks. + * @param pid The process ID for which we want to know the existence of file locks. + * @return true If the process holds any file locks, false otherwise. + * @throws IOException if /proc/locks can't be accessed. + */ + public boolean hasFileLocks(int pid) throws Exception { + ProcFileReader reader = null; + long last = -1; + long id; // ordinal position of the lock in the list + int owner; // the PID of the process that owns the lock + + try { + reader = new ProcFileReader(new FileInputStream(mPath)); + + while (reader.hasMoreData()) { + id = reader.nextLong(true); // lock id + if (id == last) { + reader.finishLine(); // blocked lock + continue; + } + + reader.nextIgnored(); // lock type: POSIX? + reader.nextIgnored(); // lock type: MANDATORY? + reader.nextIgnored(); // lock type: RW? + + owner = reader.nextInt(); // pid + if (owner == pid) { + return true; + } + reader.finishLine(); + last = id; + } + } catch (IOException e) { + // TODO: let ProcFileReader log the failed line + throw new Exception("Exception parsing /proc/locks"); + } finally { + IoUtils.closeQuietly(reader); + } + return false; + } +} diff --git a/core/java/com/android/internal/util/ProcFileReader.java b/core/java/com/android/internal/util/ProcFileReader.java index ead58c7de6117..0dd8ad89df614 100644 --- a/core/java/com/android/internal/util/ProcFileReader.java +++ b/core/java/com/android/internal/util/ProcFileReader.java @@ -28,8 +28,8 @@ import java.nio.charset.StandardCharsets; * requires each line boundary to be explicitly acknowledged using * {@link #finishLine()}. Assumes {@link StandardCharsets#US_ASCII} encoding. *

- * Currently doesn't support formats based on {@code \0}, tabs, or repeated - * delimiters. + * Currently doesn't support formats based on {@code \0}, tabs. + * Consecutive spaces are treated as a single delimiter. */ public class ProcFileReader implements Closeable { private final InputStream mStream; @@ -75,6 +75,11 @@ public class ProcFileReader implements Closeable { private void consumeBuf(int count) throws IOException { // TODO: consider moving to read pointer, but for now traceview says // these copies aren't a bottleneck. + + // skip all consecutive delimiters. + while (count < mTail && mBuffer[count] == ' ') { + count++; + } System.arraycopy(mBuffer, count, mBuffer, 0, mTail - count); mTail -= count; if (mTail == 0) { @@ -159,11 +164,18 @@ public class ProcFileReader implements Closeable { * Parse and return next token as base-10 encoded {@code long}. */ public long nextLong() throws IOException { + return nextLong(false); + } + + /** + * Parse and return next token as base-10 encoded {@code long}. + */ + public long nextLong(boolean stopAtInvalid) throws IOException { final int tokenIndex = nextTokenIndex(); if (tokenIndex == -1) { throw new ProtocolException("Missing required long"); } else { - return parseAndConsumeLong(tokenIndex); + return parseAndConsumeLong(tokenIndex, stopAtInvalid); } } @@ -176,7 +188,7 @@ public class ProcFileReader implements Closeable { if (tokenIndex == -1) { return def; } else { - return parseAndConsumeLong(tokenIndex); + return parseAndConsumeLong(tokenIndex, false); } } @@ -186,7 +198,10 @@ public class ProcFileReader implements Closeable { return s; } - private long parseAndConsumeLong(int tokenIndex) throws IOException { + /** + * If stopAtInvalid is true, don't throw IOException but return whatever parsed so far. + */ + private long parseAndConsumeLong(int tokenIndex, boolean stopAtInvalid) throws IOException { final boolean negative = mBuffer[0] == '-'; // TODO: refactor into something like IntegralToString @@ -194,7 +209,11 @@ public class ProcFileReader implements Closeable { for (int i = negative ? 1 : 0; i < tokenIndex; i++) { final int digit = mBuffer[i] - '0'; if (digit < 0 || digit > 9) { - throw invalidLong(tokenIndex); + if (stopAtInvalid) { + break; + } else { + throw invalidLong(tokenIndex); + } } // always parse as negative number and apply sign later; this @@ -226,6 +245,18 @@ public class ProcFileReader implements Closeable { return (int) value; } + /** + * Bypass the next token. + */ + public void nextIgnored() throws IOException { + final int tokenIndex = nextTokenIndex(); + if (tokenIndex == -1) { + throw new ProtocolException("Missing required token"); + } else { + consumeBuf(tokenIndex + 1); + } + } + @Override public void close() throws IOException { mStream.close(); diff --git a/core/tests/coretests/src/com/android/internal/os/ProcLocksReaderTest.java b/core/tests/coretests/src/com/android/internal/os/ProcLocksReaderTest.java new file mode 100644 index 0000000000000..d800c2c3c66e3 --- /dev/null +++ b/core/tests/coretests/src/com/android/internal/os/ProcLocksReaderTest.java @@ -0,0 +1,90 @@ +/* + * Copyright (C) 2021 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.internal.os; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import android.content.Context; +import android.os.FileUtils; + +import androidx.test.InstrumentationRegistry; +import androidx.test.filters.SmallTest; +import androidx.test.runner.AndroidJUnit4; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import java.io.File; +import java.nio.file.Files; + +@SmallTest +@RunWith(AndroidJUnit4.class) +public class ProcLocksReaderTest { + private File mProcDirectory; + + @Before + public void setUp() { + Context context = InstrumentationRegistry.getContext(); + mProcDirectory = context.getDir("proc", Context.MODE_PRIVATE); + } + + @After + public void tearDown() throws Exception { + FileUtils.deleteContents(mProcDirectory); + } + + @Test + public void testRunSimpleLocks() throws Exception { + String simpleLocks = + "1: POSIX ADVISORY READ 18403 fd:09:9070 1073741826 1073742335\n" + + "2: POSIX ADVISORY WRITE 18292 fd:09:34062 0 EOF\n"; + assertFalse(runHasFileLocks(simpleLocks, 18402)); + assertFalse(runHasFileLocks(simpleLocks, 18404)); + assertTrue(runHasFileLocks(simpleLocks, 18403)); + assertTrue(runHasFileLocks(simpleLocks, 18292)); + } + + @Test + public void testRunBlockedLocks() throws Exception { + String blockedLocks = + "1: POSIX ADVISORY READ 18403 fd:09:9070 1073741826 1073742335\n" + + "2: POSIX ADVISORY WRITE 18292 fd:09:34062 0 EOF\n" + + "2: -> POSIX ADVISORY WRITE 18291 fd:09:34062 0 EOF\n" + + "2: -> POSIX ADVISORY WRITE 18293 fd:09:34062 0 EOF\n" + + "3: POSIX ADVISORY READ 3888 fd:09:13992 128 128\n" + + "4: POSIX ADVISORY READ 3888 fd:09:14230 1073741826 1073742335\n"; + assertFalse(runHasFileLocks(blockedLocks, 18402)); + assertFalse(runHasFileLocks(blockedLocks, 18404)); + assertTrue(runHasFileLocks(blockedLocks, 18403)); + assertTrue(runHasFileLocks(blockedLocks, 18292)); + + assertFalse(runHasFileLocks(blockedLocks, 18291)); + assertFalse(runHasFileLocks(blockedLocks, 18293)); + assertTrue(runHasFileLocks(blockedLocks, 3888)); + } + + private boolean runHasFileLocks(String fileContents, int pid) throws Exception { + File tempFile = File.createTempFile("locks", null, mProcDirectory); + Files.write(tempFile.toPath(), fileContents.getBytes()); + boolean result = new ProcLocksReader(tempFile.toString()).hasFileLocks(pid); + Files.delete(tempFile.toPath()); + return result; + } +} diff --git a/core/tests/utiltests/src/com/android/internal/util/ProcFileReaderTest.java b/core/tests/utiltests/src/com/android/internal/util/ProcFileReaderTest.java index b6da1954ba796..0532628ba16d9 100644 --- a/core/tests/utiltests/src/com/android/internal/util/ProcFileReaderTest.java +++ b/core/tests/utiltests/src/com/android/internal/util/ProcFileReaderTest.java @@ -166,6 +166,46 @@ public class ProcFileReaderTest extends AndroidTestCase { assertEquals(-1L, reader.nextOptionalLong(-1L)); } + public void testInvalidLongs() throws Exception { + final ProcFileReader reader = buildReader("12: 34\n56 78@#\n"); + + assertEquals(12L, reader.nextLong(true)); + assertEquals(34L, reader.nextLong(true)); + reader.finishLine(); + assertTrue(reader.hasMoreData()); + + assertEquals(56L, reader.nextLong(true)); + assertEquals(78L, reader.nextLong(true)); + reader.finishLine(); + assertFalse(reader.hasMoreData()); + } + + public void testConsecutiveDelimiters() throws Exception { + final ProcFileReader reader = buildReader("1 2 3 4 5\n"); + + assertEquals(1L, reader.nextLong()); + assertEquals(2L, reader.nextLong()); + assertEquals(3L, reader.nextLong()); + assertEquals(4L, reader.nextLong()); + assertEquals(5L, reader.nextLong()); + reader.finishLine(); + assertFalse(reader.hasMoreData()); + } + + public void testIgnore() throws Exception { + final ProcFileReader reader = buildReader("a b c\n"); + + assertEquals("a", reader.nextString()); + assertTrue(reader.hasMoreData()); + + reader.nextIgnored(); + assertTrue(reader.hasMoreData()); + + assertEquals("c", reader.nextString()); + reader.finishLine(); + assertFalse(reader.hasMoreData()); + } + private static ProcFileReader buildReader(String string) throws IOException { return buildReader(string, 2048); } diff --git a/services/core/java/com/android/server/am/CachedAppOptimizer.java b/services/core/java/com/android/server/am/CachedAppOptimizer.java index 9dbb70757cf76..7c336d768006b 100644 --- a/services/core/java/com/android/server/am/CachedAppOptimizer.java +++ b/services/core/java/com/android/server/am/CachedAppOptimizer.java @@ -41,6 +41,7 @@ import android.util.Slog; import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; +import com.android.internal.os.ProcLocksReader; import com.android.internal.util.FrameworkStatsLog; import com.android.server.ServiceThread; @@ -319,6 +320,7 @@ public final class CachedAppOptimizer { private int mPersistentCompactionCount; private int mBfgsCompactionCount; private final ProcessDependencies mProcessDependencies; + private final ProcLocksReader mProcLocksReader; public CachedAppOptimizer(ActivityManagerService am) { this(am, null, new DefaultProcessDependencies()); @@ -335,6 +337,7 @@ public final class CachedAppOptimizer { mProcessDependencies = processDependencies; mTestCallback = callback; mSettingsObserver = new SettingsContentObserver(); + mProcLocksReader = new ProcLocksReader(); } /** @@ -1312,7 +1315,7 @@ public final class CachedAppOptimizer { try { // pre-check for locks to avoid unnecessary freeze/unfreeze operations - if (Process.hasFileLocks(pid)) { + if (mProcLocksReader.hasFileLocks(pid)) { if (DEBUG_FREEZER) { Slog.d(TAG_AM, name + " (" + pid + ") holds file locks, not freezing"); } @@ -1399,7 +1402,7 @@ public final class CachedAppOptimizer { try { // post-check to prevent races - if (Process.hasFileLocks(pid)) { + if (mProcLocksReader.hasFileLocks(pid)) { if (DEBUG_FREEZER) { Slog.d(TAG_AM, name + " (" + pid + ") holds file locks, reverting freeze"); }