From 2fcda2f9f41aecc439ebae7cf5c995ba1c5e4177 Mon Sep 17 00:00:00 2001 From: Frank Li Date: Fri, 21 May 2021 01:49:50 +0000 Subject: [PATCH] Improve the net-capabilities scheme for backward compatibility - Create a new XML tag for the new format. The new format should persist the arrays of values without assuming they necessarily have different bits. - Upon reading, if the new tag is present, use that. If the new tag is not present but the old tag is present, then limit the contents to the capabilities/transports that existed in R. - Catch all exceptions and ignore the requests if they can't be re-read from disk. This is a measure to avoid any mistake where the device couldn't boot when JS tries to restore state. Bug: 183071974 Test: atest JobStoreTest Original-Change: https://android-review.googlesource.com/1679762 Merged-In: I56cde50d2adab81134c8be4f6996d68018f4212a Change-Id: I56cde50d2adab81134c8be4f6996d68018f4212a --- .../java/com/android/server/job/JobStore.java | 133 +++++++++++++++--- .../com/android/server/job/JobStoreTest.java | 16 +++ 2 files changed, 126 insertions(+), 23 deletions(-) diff --git a/apex/jobscheduler/service/java/com/android/server/job/JobStore.java b/apex/jobscheduler/service/java/com/android/server/job/JobStore.java index f7415962cc7e..7a2840709d15 100644 --- a/apex/jobscheduler/service/java/com/android/server/job/JobStore.java +++ b/apex/jobscheduler/service/java/com/android/server/job/JobStore.java @@ -16,6 +16,9 @@ package com.android.server.job; +import static android.net.NetworkCapabilities.NET_CAPABILITY_TEMPORARILY_NOT_METERED; +import static android.net.NetworkCapabilities.TRANSPORT_TEST; + import static com.android.server.job.JobSchedulerService.sElapsedRealtimeClock; import static com.android.server.job.JobSchedulerService.sSystemClock; @@ -30,6 +33,7 @@ import android.os.PersistableBundle; import android.os.Process; import android.os.SystemClock; import android.os.UserHandle; +import android.text.TextUtils; import android.text.format.DateUtils; import android.util.ArraySet; import android.util.AtomicFile; @@ -63,6 +67,7 @@ import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.List; import java.util.Set; +import java.util.StringJoiner; import java.util.function.Consumer; import java.util.function.Predicate; @@ -386,6 +391,36 @@ public final class JobStore { return true; } + /** + * Returns a single string representation of the contents of the specified intArray. + * If the intArray is [1, 2, 4] as the input, the return result will be the string "1,2,4". + */ + @VisibleForTesting + static String intArrayToString(int[] values) { + final StringJoiner sj = new StringJoiner(","); + for (final int value : values) { + sj.add(String.valueOf(value)); + } + return sj.toString(); + } + + + /** + * Converts a string containing a comma-separated list of decimal representations + * of ints into an array of int. If the string is not correctly formatted, + * or if any value doesn't fit into an int, NumberFormatException is thrown. + */ + @VisibleForTesting + static int[] stringToIntArray(String str) { + if (TextUtils.isEmpty(str)) return new int[0]; + final String[] arr = str.split(","); + final int[] values = new int[arr.length]; + for (int i = 0; i < arr.length; i++) { + values[i] = Integer.parseInt(arr[i]); + } + return values; + } + /** * Runnable that writes {@link #mJobSet} out to xml. * NOTE: This Runnable locks on mLock @@ -549,15 +584,12 @@ public final class JobStore { out.startTag(null, XML_TAG_PARAMS_CONSTRAINTS); if (jobStatus.hasConnectivityConstraint()) { final NetworkRequest network = jobStatus.getJob().getRequiredNetwork(); - // STOPSHIP b/183071974: improve the scheme for backward compatibility and - // mainline cleanliness. - out.attribute(null, "net-capabilities", Long.toString( - BitUtils.packBits(network.getCapabilities()))); - out.attribute(null, "net-unwanted-capabilities", Long.toString( - BitUtils.packBits(network.getForbiddenCapabilities()))); - - out.attribute(null, "net-transport-types", Long.toString( - BitUtils.packBits(network.getTransportTypes()))); + out.attribute(null, "net-capabilities-csv", intArrayToString( + network.getCapabilities())); + out.attribute(null, "net-forbidden-capabilities-csv", intArrayToString( + network.getForbiddenCapabilities())); + out.attribute(null, "net-transport-types-csv", intArrayToString( + network.getTransportTypes())); } if (jobStatus.hasIdleConstraint()) { out.attribute(null, "idle", Boolean.toString(true)); @@ -831,7 +863,14 @@ public final class JobStore { } catch (NumberFormatException e) { Slog.d(TAG, "Error reading constraints, skipping."); return null; + } catch (XmlPullParserException e) { + Slog.d(TAG, "Error Parser Exception.", e); + return null; + } catch (IOException e) { + Slog.d(TAG, "Error I/O Exception.", e); + return null; } + parser.next(); // Consume // Read out execution parameters tag. @@ -973,31 +1012,79 @@ public final class JobStore { return new JobInfo.Builder(jobId, cname); } - private void buildConstraintsFromXml(JobInfo.Builder jobBuilder, XmlPullParser parser) { + /** + * In S, there has been a change in format to make the code more robust and more + * maintainable. + * If the capabities are bits 4, 14, 15, the format in R, it is a long string as + * netCapabilitiesLong = '49168' from the old XML file attribute "net-capabilities". + * The format in S is the int array string as netCapabilitiesIntArray = '4,14,15' + * from the new XML file attribute "net-capabilities-array". + * For backward compatibility, when reading old XML the old format is still supported in + * reading, but in order to avoid issues with OEM-defined flags, the accepted capabilities + * are limited to that(maxNetCapabilityInR & maxTransportInR) defined in R. + */ + private void buildConstraintsFromXml(JobInfo.Builder jobBuilder, XmlPullParser parser) + throws XmlPullParserException, IOException { String val; + String netCapabilitiesLong = null; + String netForbiddenCapabilitiesLong = null; + String netTransportTypesLong = null; - final String netCapabilities = parser.getAttributeValue(null, "net-capabilities"); - final String netforbiddenCapabilities = parser.getAttributeValue( - null, "net-unwanted-capabilities"); - final String netTransportTypes = parser.getAttributeValue(null, "net-transport-types"); - if (netCapabilities != null && netTransportTypes != null) { + final String netCapabilitiesIntArray = parser.getAttributeValue( + null, "net-capabilities-csv"); + final String netForbiddenCapabilitiesIntArray = parser.getAttributeValue( + null, "net-forbidden-capabilities-csv"); + final String netTransportTypesIntArray = parser.getAttributeValue( + null, "net-transport-types-csv"); + if (netCapabilitiesIntArray == null || netTransportTypesIntArray == null) { + netCapabilitiesLong = parser.getAttributeValue(null, "net-capabilities"); + netForbiddenCapabilitiesLong = parser.getAttributeValue( + null, "net-unwanted-capabilities"); + netTransportTypesLong = parser.getAttributeValue(null, "net-transport-types"); + } + + if ((netCapabilitiesIntArray != null) && (netTransportTypesIntArray != null)) { final NetworkRequest.Builder builder = new NetworkRequest.Builder() .clearCapabilities(); - final long forbiddenCapabilities = netforbiddenCapabilities != null - ? Long.parseLong(netforbiddenCapabilities) - : BitUtils.packBits(builder.build().getForbiddenCapabilities()); - // We're okay throwing NFE here; caught by caller - for (int capability : BitUtils.unpackBits(Long.parseLong(netCapabilities))) { + + for (int capability : stringToIntArray(netCapabilitiesIntArray)) { builder.addCapability(capability); } - for (int forbiddenCapability : BitUtils.unpackBits( - Long.parseLong(netforbiddenCapabilities))) { + + for (int forbiddenCapability : stringToIntArray(netForbiddenCapabilitiesIntArray)) { builder.addForbiddenCapability(forbiddenCapability); } - for (int transport : BitUtils.unpackBits(Long.parseLong(netTransportTypes))) { + + for (int transport : stringToIntArray(netTransportTypesIntArray)) { builder.addTransportType(transport); } jobBuilder.setRequiredNetwork(builder.build()); + } else if (netCapabilitiesLong != null && netTransportTypesLong != null) { + final NetworkRequest.Builder builder = new NetworkRequest.Builder() + .clearCapabilities(); + final int maxNetCapabilityInR = NET_CAPABILITY_TEMPORARILY_NOT_METERED; + // We're okay throwing NFE here; caught by caller + for (int capability : BitUtils.unpackBits(Long.parseLong( + netCapabilitiesLong))) { + if (capability <= maxNetCapabilityInR) { + builder.addCapability(capability); + } + } + for (int forbiddenCapability : BitUtils.unpackBits(Long.parseLong( + netForbiddenCapabilitiesLong))) { + if (forbiddenCapability <= maxNetCapabilityInR) { + builder.addForbiddenCapability(forbiddenCapability); + } + } + + final int maxTransportInR = TRANSPORT_TEST; + for (int transport : BitUtils.unpackBits(Long.parseLong( + netTransportTypesLong))) { + if (transport <= maxTransportInR) { + builder.addTransportType(transport); + } + } + jobBuilder.setRequiredNetwork(builder.build()); } else { // Read legacy values val = parser.getAttributeValue(null, "connectivity"); diff --git a/services/tests/servicestests/src/com/android/server/job/JobStoreTest.java b/services/tests/servicestests/src/com/android/server/job/JobStoreTest.java index 2efebbf044c9..8eb3cf3ca418 100644 --- a/services/tests/servicestests/src/com/android/server/job/JobStoreTest.java +++ b/services/tests/servicestests/src/com/android/server/job/JobStoreTest.java @@ -4,7 +4,9 @@ import static android.net.NetworkCapabilities.NET_CAPABILITY_IMS; import static android.net.NetworkCapabilities.NET_CAPABILITY_OEM_PAID; import static android.net.NetworkCapabilities.TRANSPORT_WIFI; +import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.anyString; @@ -101,6 +103,20 @@ public class JobStoreTest { mTaskStoreUnderTest.waitForWriteToCompleteForTesting(5_000L)); } + @Test + public void testStringToIntArrayAndIntArrayToString() { + final int[] netCapabilitiesIntArray = { 1, 3, 5, 7, 9 }; + final String netCapabilitiesStr = "1,3,5,7,9"; + final String netCapabilitiesStrWithErrorInt = "1,3,a,7,9"; + final String emptyString = ""; + final String str1 = JobStore.intArrayToString(netCapabilitiesIntArray); + assertArrayEquals(netCapabilitiesIntArray, JobStore.stringToIntArray(str1)); + assertEquals(0, JobStore.stringToIntArray(emptyString).length); + assertThrows(NumberFormatException.class, + () -> JobStore.stringToIntArray(netCapabilitiesStrWithErrorInt)); + assertEquals(netCapabilitiesStr, JobStore.intArrayToString(netCapabilitiesIntArray)); + } + @Test public void testMaybeWriteStatusToDisk() throws Exception { int taskId = 5;