Merge changes I49057737,I68e3096d

* changes:
  Frameworks: Clean up SystemProperties
  Frameworks: Add warning to SystemProperties.get

core/java/android/os/SystemProperties.java

@@ -16,6 +16,8 @@
 
 package android.os;
 
+import android.annotation.NonNull;
+import android.annotation.Nullable;
 import android.util.Log;
 import android.util.MutableInt;
 
@@ -43,17 +45,12 @@ public class SystemProperties {
 
     public static final int PROP_VALUE_MAX = 91;
 
+    @GuardedBy("sChangeCallbacks")
     private static final ArrayList<Runnable> sChangeCallbacks = new ArrayList<Runnable>();
 
     @GuardedBy("sRoReads")
-    private static final HashMap<String, MutableInt> sRoReads;
+    private static final HashMap<String, MutableInt> sRoReads =
-    static {
+            TRACK_KEY_ACCESS ? new HashMap<>() : null;
-        if (TRACK_KEY_ACCESS) {
-            sRoReads = new HashMap<>();
-        } else {
-            sRoReads = null;
-        }
-    }
 
     private static void onKeyAccess(String key) {
         if (!TRACK_KEY_ACCESS) return;
@@ -85,77 +82,102 @@ public class SystemProperties {
     private static native void native_report_sysprop_change();
 
     /**
-     * Get the value for the given key.
+     * Get the String value for the given {@code key}.
-     * @return an empty string if the key isn't found
+     *
+     * <b>WARNING:</b> Do not use this method if the value may not be a valid UTF string! This
+     * method will crash in native code.
+     *
+     * @param key the key to lookup
+     * @return an empty string if the {@code key} isn't found
      */
-    public static String get(String key) {
+    @NonNull
+    public static String get(@NonNull String key) {
         if (TRACK_KEY_ACCESS) onKeyAccess(key);
         return native_get(key);
     }
 
     /**
-     * Get the value for the given key.
+     * Get the String value for the given {@code key}.
-     * @return if the key isn't found, return def if it isn't null, or an empty string otherwise
+     *
+     * <b>WARNING:</b> Do not use this method if the value may not be a valid UTF string! This
+     * method will crash in native code.
+     *
+     * @param key the key to lookup
+     * @param def the default value in case the property is not set or empty
+     * @return if the {@code key} isn't found, return {@code def} if it isn't null, or an empty
+     * string otherwise
      */
-    public static String get(String key, String def) {
+    @NonNull
+    public static String get(@NonNull String key, @Nullable String def) {
         if (TRACK_KEY_ACCESS) onKeyAccess(key);
         return native_get(key, def);
     }
 
     /**
-     * Get the value for the given key, and return as an integer.
+     * Get the value for the given {@code key}, and return as an integer.
+     *
      * @param key the key to lookup
      * @param def a default value to return
      * @return the key parsed as an integer, or def if the key isn't found or
      *         cannot be parsed
      */
-    public static int getInt(String key, int def) {
+    public static int getInt(@NonNull String key, int def) {
         if (TRACK_KEY_ACCESS) onKeyAccess(key);
         return native_get_int(key, def);
     }
 
     /**
-     * Get the value for the given key, and return as a long.
+     * Get the value for the given {@code key}, and return as a long.
+     *
      * @param key the key to lookup
      * @param def a default value to return
      * @return the key parsed as a long, or def if the key isn't found or
      *         cannot be parsed
      */
-    public static long getLong(String key, long def) {
+    public static long getLong(@NonNull String key, long def) {
         if (TRACK_KEY_ACCESS) onKeyAccess(key);
         return native_get_long(key, def);
     }
 
     /**
-     * Get the value for the given key, returned as a boolean.
+     * Get the value for the given {@code key}, returned as a boolean.
      * Values 'n', 'no', '0', 'false' or 'off' are considered false.
      * Values 'y', 'yes', '1', 'true' or 'on' are considered true.
      * (case sensitive).
      * If the key does not exist, or has any other value, then the default
      * result is returned.
+     *
      * @param key the key to lookup
      * @param def a default value to return
      * @return the key parsed as a boolean, or def if the key isn't found or is
      *         not able to be parsed as a boolean.
      */
-    public static boolean getBoolean(String key, boolean def) {
+    public static boolean getBoolean(@NonNull String key, boolean def) {
         if (TRACK_KEY_ACCESS) onKeyAccess(key);
         return native_get_boolean(key, def);
     }
 
     /**
-     * Set the value for the given key.
+     * Set the value for the given {@code key} to {@code val}.
-     * @throws IllegalArgumentException if the value exceeds 92 characters
+     *
+     * @throws IllegalArgumentException if the {@code val} exceeds 91 characters
      */
-    public static void set(String key, String val) {
+    public static void set(@NonNull String key, @Nullable String val) {
         if (val != null && val.length() > PROP_VALUE_MAX) {
-            throw newValueTooLargeException(key, val);
+            throw new IllegalArgumentException("value of system property '" + key
+                    + "' is longer than " + PROP_VALUE_MAX + " characters: " + val);
         }
         if (TRACK_KEY_ACCESS) onKeyAccess(key);
         native_set(key, val);
     }
 
-    public static void addChangeCallback(Runnable callback) {
+    /**
+     * Add a callback that will be run whenever any system property changes.
+     *
+     * @param callback The {@link Runnable} that should be executed when a system property
+     * changes.
+     */
+    public static void addChangeCallback(@NonNull Runnable callback) {
         synchronized (sChangeCallbacks) {
             if (sChangeCallbacks.size() == 0) {
                 native_add_change_callback();
@@ -164,7 +186,8 @@ public class SystemProperties {
         }
     }
 
-    static void callChangeCallbacks() {
+    @SuppressWarnings("unused")  // Called from native code.
+    private static void callChangeCallbacks() {
         synchronized (sChangeCallbacks) {
             //Log.i("foo", "Calling " + sChangeCallbacks.size() + " change callbacks!");
             if (sChangeCallbacks.size() == 0) {
@@ -177,11 +200,6 @@ public class SystemProperties {
         }
     }
 
-    private static IllegalArgumentException newValueTooLargeException(String key, String value) {
-        return new IllegalArgumentException("value of system property '" + key + "' is longer than "
-                + PROP_VALUE_MAX + " characters: " + value);
-    }
-
     /*
      * Notifies listeners that a system property has changed
      */

core/jni/android_os_SystemProperties.cpp

@@ -17,188 +17,109 @@
 
 #define LOG_TAG "SysPropJNI"
 
+#include "android-base/logging.h"
+#include "android-base/properties.h"
 #include "cutils/properties.h"
 #include "utils/misc.h"
 #include <utils/Log.h>
 #include "jni.h"
 #include "core_jni_helpers.h"
 #include <nativehelper/JNIHelp.h>
+#include <nativehelper/ScopedPrimitiveArray.h>
+#include <nativehelper/ScopedUtfChars.h>
 
 namespace android
 {
 
-static jstring SystemProperties_getSS(JNIEnv *env, jobject clazz,
+namespace {
-                                      jstring keyJ, jstring defJ)
-{
-    int len;
-    const char* key;
-    char buf[PROPERTY_VALUE_MAX];
-    jstring rvJ = NULL;
 
-    if (keyJ == NULL) {
+template <typename T, typename Handler>
-        jniThrowNullPointerException(env, "key must not be null.");
+T ConvertKeyAndForward(JNIEnv *env, jstring keyJ, T defJ, Handler handler) {
-        goto error;
+    std::string key;
-    }
+    {
-
+        // Scope the String access. If the handler can throw an exception,
-    key = env->GetStringUTFChars(keyJ, NULL);
+        // releasing the string characters late would trigger an abort.
-
+        ScopedUtfChars key_utf(env, keyJ);
-    len = property_get(key, buf, "");
+        if (key_utf.c_str() == nullptr) {
-    if ((len <= 0) && (defJ != NULL)) {
+            return defJ;
-        rvJ = defJ;
-    } else if (len >= 0) {
-        rvJ = env->NewStringUTF(buf);
-    } else {
-        rvJ = env->NewStringUTF("");
-    }
-
-    env->ReleaseStringUTFChars(keyJ, key);
-
-error:
-    return rvJ;
-}
-
-static jstring SystemProperties_getS(JNIEnv *env, jobject clazz,
-                                      jstring keyJ)
-{
-    return SystemProperties_getSS(env, clazz, keyJ, NULL);
-}
-
-static jint SystemProperties_get_int(JNIEnv *env, jobject clazz,
-                                      jstring keyJ, jint defJ)
-{
-    int len;
-    const char* key;
-    char buf[PROPERTY_VALUE_MAX];
-    char* end;
-    jint result = defJ;
-
-    if (keyJ == NULL) {
-        jniThrowNullPointerException(env, "key must not be null.");
-        goto error;
-    }
-
-    key = env->GetStringUTFChars(keyJ, NULL);
-
-    len = property_get(key, buf, "");
-    if (len > 0) {
-        result = strtol(buf, &end, 0);
-        if (end == buf) {
-            result = defJ;
         }
+        key = key_utf.c_str();  // This will make a copy, but we can't avoid
+                                // with the existing interface in
+                                // android::base.
     }
-
+    return handler(key, defJ);
-    env->ReleaseStringUTFChars(keyJ, key);
-
-error:
-    return result;
 }
 
-static jlong SystemProperties_get_long(JNIEnv *env, jobject clazz,
+jstring SystemProperties_getSS(JNIEnv *env, jclass clazz, jstring keyJ,
-                                      jstring keyJ, jlong defJ)
+                               jstring defJ)
 {
-    int len;
+    // Using ConvertKeyAndForward is sub-optimal for copying the key string,
-    const char* key;
+    // but improves reuse and reasoning over code.
-    char buf[PROPERTY_VALUE_MAX];
+    auto handler = [&](const std::string& key, jstring defJ) {
-    char* end;
+        std::string prop_val = android::base::GetProperty(key, "");
-    jlong result = defJ;
+        if (!prop_val.empty()) {
-
+            return env->NewStringUTF(prop_val.c_str());
-    if (keyJ == NULL) {
+        };
-        jniThrowNullPointerException(env, "key must not be null.");
+        if (defJ != nullptr) {
-        goto error;
+            return defJ;
-    }
-
-    key = env->GetStringUTFChars(keyJ, NULL);
-
-    len = property_get(key, buf, "");
-    if (len > 0) {
-        result = strtoll(buf, &end, 0);
-        if (end == buf) {
-            result = defJ;
         }
-    }
+        // This function is specified to never return null (or have an
-
+        // exception pending).
-    env->ReleaseStringUTFChars(keyJ, key);
+        return env->NewStringUTF("");
-
+    };
-error:
+    return ConvertKeyAndForward(env, keyJ, defJ, handler);
-    return result;
 }
 
-static jboolean SystemProperties_get_boolean(JNIEnv *env, jobject clazz,
+jstring SystemProperties_getS(JNIEnv *env, jclass clazz, jstring keyJ)
-                                      jstring keyJ, jboolean defJ)
 {
-    int len;
+    return SystemProperties_getSS(env, clazz, keyJ, nullptr);
-    const char* key;
+}
-    char buf[PROPERTY_VALUE_MAX];
-    jboolean result = defJ;
 
-    if (keyJ == NULL) {
+template <typename T>
-        jniThrowNullPointerException(env, "key must not be null.");
+T SystemProperties_get_integral(JNIEnv *env, jclass, jstring keyJ,
-        goto error;
+                                       T defJ)
-    }
+{
+    auto handler = [](const std::string& key, T defV) {
+        return android::base::GetIntProperty<T>(key, defV);
+    };
+    return ConvertKeyAndForward(env, keyJ, defJ, handler);
+}
 
-    key = env->GetStringUTFChars(keyJ, NULL);
+jboolean SystemProperties_get_boolean(JNIEnv *env, jclass, jstring keyJ,
+                                      jboolean defJ)
+{
+    auto handler = [](const std::string& key, jboolean defV) -> jboolean {
+        bool result = android::base::GetBoolProperty(key, defV);
+        return result ? JNI_TRUE : JNI_FALSE;
+    };
+    return ConvertKeyAndForward(env, keyJ, defJ, handler);
+}
 
-    len = property_get(key, buf, "");
+void SystemProperties_set(JNIEnv *env, jobject clazz, jstring keyJ,
-    if (len == 1) {
+                          jstring valJ)
-        char ch = buf[0];
+{
-        if (ch == '0' || ch == 'n')
+    auto handler = [&](const std::string& key, bool) {
-            result = false;
+        std::string val;
-        else if (ch == '1' || ch == 'y')
+        if (valJ != nullptr) {
-            result = true;
+            ScopedUtfChars key_utf(env, valJ);
-    } else if (len > 1) {
+            val = key_utf.c_str();
-         if (!strcmp(buf, "no") || !strcmp(buf, "false") || !strcmp(buf, "off")) {
-            result = false;
-        } else if (!strcmp(buf, "yes") || !strcmp(buf, "true") || !strcmp(buf, "on")) {
-            result = true;
         }
-    }
+        return android::base::SetProperty(key, val);
-
+    };
-    env->ReleaseStringUTFChars(keyJ, key);
+    if (!ConvertKeyAndForward(env, keyJ, true, handler)) {
-
+        // Must have been a failure in SetProperty.
-error:
-    return result;
-}
-
-static void SystemProperties_set(JNIEnv *env, jobject clazz,
-                                      jstring keyJ, jstring valJ)
-{
-    int err;
-    const char* key;
-    const char* val;
-
-    if (keyJ == NULL) {
-        jniThrowNullPointerException(env, "key must not be null.");
-        return ;
-    }
-    key = env->GetStringUTFChars(keyJ, NULL);
-
-    if (valJ == NULL) {
-        val = "";       /* NULL pointer not allowed here */
-    } else {
-        val = env->GetStringUTFChars(valJ, NULL);
-    }
-
-    err = property_set(key, val);
-
-    env->ReleaseStringUTFChars(keyJ, key);
-
-    if (valJ != NULL) {
-        env->ReleaseStringUTFChars(valJ, val);
-    }
-
-    if (err < 0) {
         jniThrowException(env, "java/lang/RuntimeException",
                           "failed to set system property");
     }
 }
 
-static JavaVM* sVM = NULL;
+JavaVM* sVM = nullptr;
-static jclass sClazz = NULL;
+jclass sClazz = nullptr;
-static jmethodID sCallChangeCallbacks;
+jmethodID sCallChangeCallbacks;
 
-static void do_report_sysprop_change() {
+void do_report_sysprop_change() {
     //ALOGI("Java SystemProperties: VM=%p, Clazz=%p", sVM, sClazz);
-    if (sVM != NULL && sClazz != NULL) {
+    if (sVM != nullptr && sClazz != nullptr) {
         JNIEnv* env;
         if (sVM->GetEnv((void **)&env, JNI_VERSION_1_4) >= 0) {
             //ALOGI("Java SystemProperties: calling %p", sCallChangeCallbacks);
@@ -207,47 +128,49 @@ static void do_report_sysprop_change() {
     }
 }
 
-static void SystemProperties_add_change_callback(JNIEnv *env, jobject clazz)
+void SystemProperties_add_change_callback(JNIEnv *env, jobject clazz)
 {
     // This is called with the Java lock held.
-    if (sVM == NULL) {
+    if (sVM == nullptr) {
         env->GetJavaVM(&sVM);
     }
-    if (sClazz == NULL) {
+    if (sClazz == nullptr) {
         sClazz = (jclass) env->NewGlobalRef(clazz);
         sCallChangeCallbacks = env->GetStaticMethodID(sClazz, "callChangeCallbacks", "()V");
         add_sysprop_change_callback(do_report_sysprop_change, -10000);
     }
 }
 
-static void SystemProperties_report_sysprop_change(JNIEnv /**env*/, jobject /*clazz*/)
+void SystemProperties_report_sysprop_change(JNIEnv /**env*/, jobject /*clazz*/)
 {
     report_sysprop_change();
 }
 
-static const JNINativeMethod method_table[] = {
+}  // namespace
-    { "native_get", "(Ljava/lang/String;)Ljava/lang/String;",
-      (void*) SystemProperties_getS },
-    { "native_get", "(Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;",
-      (void*) SystemProperties_getSS },
-    { "native_get_int", "(Ljava/lang/String;I)I",
-      (void*) SystemProperties_get_int },
-    { "native_get_long", "(Ljava/lang/String;J)J",
-      (void*) SystemProperties_get_long },
-    { "native_get_boolean", "(Ljava/lang/String;Z)Z",
-      (void*) SystemProperties_get_boolean },
-    { "native_set", "(Ljava/lang/String;Ljava/lang/String;)V",
-      (void*) SystemProperties_set },
-    { "native_add_change_callback", "()V",
-      (void*) SystemProperties_add_change_callback },
-    { "native_report_sysprop_change", "()V",
-      (void*) SystemProperties_report_sysprop_change },
-};
 
 int register_android_os_SystemProperties(JNIEnv *env)
 {
-    return RegisterMethodsOrDie(env, "android/os/SystemProperties", method_table,
+    const JNINativeMethod method_table[] = {
-                                NELEM(method_table));
+        { "native_get", "(Ljava/lang/String;)Ljava/lang/String;",
+          (void*) SystemProperties_getS },
+        { "native_get",
+          "(Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;",
+          (void*) SystemProperties_getSS },
+        { "native_get_int", "(Ljava/lang/String;I)I",
+          (void*) SystemProperties_get_integral<jint> },
+        { "native_get_long", "(Ljava/lang/String;J)J",
+          (void*) SystemProperties_get_integral<jlong> },
+        { "native_get_boolean", "(Ljava/lang/String;Z)Z",
+          (void*) SystemProperties_get_boolean },
+        { "native_set", "(Ljava/lang/String;Ljava/lang/String;)V",
+          (void*) SystemProperties_set },
+        { "native_add_change_callback", "()V",
+          (void*) SystemProperties_add_change_callback },
+        { "native_report_sysprop_change", "()V",
+          (void*) SystemProperties_report_sysprop_change },
+    };
+    return RegisterMethodsOrDie(env, "android/os/SystemProperties",
+                                method_table, NELEM(method_table));
 }
 
 };

core/tests/systemproperties/run_core_systemproperties_test.sh

@@ -15,7 +15,7 @@ fi
 
 if [[ $rebuild == true ]]; then
   make -j4 FrameworksCoreSystemPropertiesTests
-  TESTAPP=${ANDROID_PRODUCT_OUT}/data/app/FrameworksCoreSystemPropertiesTests.apk
+  TESTAPP=${ANDROID_PRODUCT_OUT}/data/app/FrameworksCoreSystemPropertiesTests/FrameworksCoreSystemPropertiesTests.apk
   COMMAND="adb install -r $TESTAPP"
   echo $COMMAND
   $COMMAND

core/tests/systemproperties/src/android/os/SystemPropertiesTest.java

@@ -51,6 +51,11 @@ public class SystemPropertiesTest extends TestCase {
         value = SystemProperties.get(KEY, "default");
         assertEquals("default", value);
 
+        // null default value is the same as "".
+        SystemProperties.set(KEY, null);
+        value = SystemProperties.get(KEY, "default");
+        assertEquals("default", value);
+
         SystemProperties.set(KEY, "SA");
         value = SystemProperties.get(KEY, "default");
         assertEquals("SA", value);
@@ -62,7 +67,78 @@ public class SystemPropertiesTest extends TestCase {
         value = SystemProperties.get(KEY, "default");
         assertEquals("default", value);
 
+        // null value is the same as "".
+        SystemProperties.set(KEY, "SA");
+        SystemProperties.set(KEY, null);
+        value = SystemProperties.get(KEY, "default");
+        assertEquals("default", value);
+
         value = SystemProperties.get(KEY);
         assertEquals("", value);
     }
+
+    private static void testInt(String setVal, int defValue, int expected) {
+      SystemProperties.set(KEY, setVal);
+      int value = SystemProperties.getInt(KEY, defValue);
+      assertEquals(expected, value);
+    }
+
+    private static void testLong(String setVal, long defValue, long expected) {
+      SystemProperties.set(KEY, setVal);
+      long value = SystemProperties.getLong(KEY, defValue);
+      assertEquals(expected, value);
+    }
+
+    @SmallTest
+    public void testIntegralProperties() throws Exception {
+        testInt("", 123, 123);
+        testInt("", 0, 0);
+        testInt("", -123, -123);
+
+        testInt("123", 124, 123);
+        testInt("0", 124, 0);
+        testInt("-123", 124, -123);
+
+        testLong("", 3147483647L, 3147483647L);
+        testLong("", 0, 0);
+        testLong("", -3147483647L, -3147483647L);
+
+        testLong("3147483647", 124, 3147483647L);
+        testLong("0", 124, 0);
+        testLong("-3147483647", 124, -3147483647L);
+    }
+
+    @SmallTest
+    @SuppressWarnings("null")
+    public void testNullKey() throws Exception {
+        try {
+            SystemProperties.get(null);
+            fail("Expected NullPointerException");
+        } catch (NullPointerException npe) {
+        }
+
+        try {
+            SystemProperties.get(null, "default");
+            fail("Expected NullPointerException");
+        } catch (NullPointerException npe) {
+        }
+
+        try {
+            SystemProperties.set(null, "value");
+            fail("Expected NullPointerException");
+        } catch (NullPointerException npe) {
+        }
+
+        try {
+            SystemProperties.getInt(null, 0);
+            fail("Expected NullPointerException");
+        } catch (NullPointerException npe) {
+        }
+
+        try {
+            SystemProperties.getLong(null, 0);
+            fail("Expected NullPointerException");
+        } catch (NullPointerException npe) {
+        }
+    }
 }