From ffa84e008c712ceffa09d6b89a49882c88b3cca5 Mon Sep 17 00:00:00 2001 From: Hans Boehm Date: Tue, 11 Nov 2014 17:01:37 -0800 Subject: [PATCH] Reduce risk of memory corruption due to finalization. Many classes in graphics/java and elsewhere deallocate native memory in a finalizer on the assumption that instance methods can no longer be called once the finalizer has been called. This is incorrect if the object can be used, possibly indirectly, from another finalizer, possibly one in the application. This is the initial installment of a patch to cause such post-finalization uses to at least see a null pointer rather than causing memory corruption by accessing deallocated native memory. This should make it possible to identify and fix such finalization ordering issues. There are more graphics classes that need this treatment, and probably many more in other subsystems. This solution is < 100% effective if finalizers can be invoked concurrently. We currently promise that they aren't. (In my opinion, the real cause here is a language spec bug. But that ship has sailed.) Bug: 18178237 Change-Id: I844cf1e0fbb190407389c4f8e8f072752cca6198 --- graphics/java/android/graphics/Interpolator.java | 3 ++- graphics/java/android/graphics/MaskFilter.java | 1 + graphics/java/android/graphics/Matrix.java | 1 + graphics/java/android/graphics/NinePatch.java | 3 ++- graphics/java/android/graphics/Paint.java | 1 + graphics/java/android/graphics/Path.java | 3 ++- graphics/java/android/graphics/PathEffect.java | 1 + graphics/java/android/graphics/PathMeasure.java | 3 ++- graphics/java/android/graphics/Picture.java | 3 ++- graphics/java/android/graphics/Region.java | 3 ++- graphics/java/android/graphics/RegionIterator.java | 3 ++- graphics/java/android/graphics/Shader.java | 1 + graphics/java/android/graphics/Typeface.java | 1 + 13 files changed, 20 insertions(+), 7 deletions(-) diff --git a/graphics/java/android/graphics/Interpolator.java b/graphics/java/android/graphics/Interpolator.java index f695a9ea2cf4d..104546454fa9e 100644 --- a/graphics/java/android/graphics/Interpolator.java +++ b/graphics/java/android/graphics/Interpolator.java @@ -147,11 +147,12 @@ public class Interpolator { @Override protected void finalize() throws Throwable { nativeDestructor(native_instance); + native_instance = 0; // Other finalizers can still call us. } private int mValueCount; private int mFrameCount; - private final long native_instance; + private long native_instance; private static native long nativeConstructor(int valueCount, int frameCount); private static native void nativeDestructor(long native_instance); diff --git a/graphics/java/android/graphics/MaskFilter.java b/graphics/java/android/graphics/MaskFilter.java index 27a7dda493daf..d4743155729ea 100644 --- a/graphics/java/android/graphics/MaskFilter.java +++ b/graphics/java/android/graphics/MaskFilter.java @@ -25,6 +25,7 @@ public class MaskFilter { protected void finalize() throws Throwable { nativeDestructor(native_instance); + native_instance = 0; // Other finalizers can still call us. } private static native void nativeDestructor(long native_filter); diff --git a/graphics/java/android/graphics/Matrix.java b/graphics/java/android/graphics/Matrix.java index 90e5a4e487fab..1e8f11bbe3edd 100644 --- a/graphics/java/android/graphics/Matrix.java +++ b/graphics/java/android/graphics/Matrix.java @@ -827,6 +827,7 @@ public class Matrix { protected void finalize() throws Throwable { try { finalizer(native_instance); + native_instance = 0; // Other finalizers can still call us. } finally { super.finalize(); } diff --git a/graphics/java/android/graphics/NinePatch.java b/graphics/java/android/graphics/NinePatch.java index 6f42046e357d3..ec8020586b3fc 100644 --- a/graphics/java/android/graphics/NinePatch.java +++ b/graphics/java/android/graphics/NinePatch.java @@ -71,7 +71,7 @@ public class NinePatch { * * @hide */ - public final long mNativeChunk; + public long mNativeChunk; private Paint mPaint; private String mSrcName; @@ -121,6 +121,7 @@ public class NinePatch { if (mNativeChunk != 0) { // only attempt to destroy correctly initilized chunks nativeFinalize(mNativeChunk); + mNativeChunk = 0; } } finally { super.finalize(); diff --git a/graphics/java/android/graphics/Paint.java b/graphics/java/android/graphics/Paint.java index 652fe64c0a140..b8bb5a69e2a37 100644 --- a/graphics/java/android/graphics/Paint.java +++ b/graphics/java/android/graphics/Paint.java @@ -2219,6 +2219,7 @@ public class Paint { protected void finalize() throws Throwable { try { finalizer(mNativePaint); + mNativePaint = 0; } finally { super.finalize(); } diff --git a/graphics/java/android/graphics/Path.java b/graphics/java/android/graphics/Path.java index 0e9823d1661fd..42a3600d77e3c 100644 --- a/graphics/java/android/graphics/Path.java +++ b/graphics/java/android/graphics/Path.java @@ -27,7 +27,7 @@ public class Path { /** * @hide */ - public final long mNativePath; + public long mNativePath; /** * @hide @@ -746,6 +746,7 @@ public class Path { protected void finalize() throws Throwable { try { finalizer(mNativePath); + mNativePath = 0; // Other finalizers can still call us. } finally { super.finalize(); } diff --git a/graphics/java/android/graphics/PathEffect.java b/graphics/java/android/graphics/PathEffect.java index 617dfca6a966a..3292501e6324f 100644 --- a/graphics/java/android/graphics/PathEffect.java +++ b/graphics/java/android/graphics/PathEffect.java @@ -25,6 +25,7 @@ public class PathEffect { protected void finalize() throws Throwable { nativeDestructor(native_instance); + native_instance = 0; // Other finalizers can still call us. } private static native void nativeDestructor(long native_patheffect); diff --git a/graphics/java/android/graphics/PathMeasure.java b/graphics/java/android/graphics/PathMeasure.java index 7cc97653c097d..041615969d878 100644 --- a/graphics/java/android/graphics/PathMeasure.java +++ b/graphics/java/android/graphics/PathMeasure.java @@ -142,6 +142,7 @@ public class PathMeasure { protected void finalize() throws Throwable { native_destroy(native_instance); + native_instance = 0; // Other finalizers can still call us. } private static native long native_create(long native_path, boolean forceClosed); @@ -154,6 +155,6 @@ public class PathMeasure { private static native boolean native_nextContour(long native_instance); private static native void native_destroy(long native_instance); - /* package */private final long native_instance; + /* package */private long native_instance; } diff --git a/graphics/java/android/graphics/Picture.java b/graphics/java/android/graphics/Picture.java index fa9af2a4a4509..4dcb2f53eac5b 100644 --- a/graphics/java/android/graphics/Picture.java +++ b/graphics/java/android/graphics/Picture.java @@ -29,7 +29,7 @@ import java.io.OutputStream; */ public class Picture { private Canvas mRecordingCanvas; - private final long mNativePicture; + private long mNativePicture; private static final int WORKING_STREAM_STORAGE = 16 * 1024; @@ -60,6 +60,7 @@ public class Picture { protected void finalize() throws Throwable { try { nativeDestructor(mNativePicture); + mNativePicture = 0; } finally { super.finalize(); } diff --git a/graphics/java/android/graphics/Region.java b/graphics/java/android/graphics/Region.java index 727723d7f6752..de89ad07d8735 100644 --- a/graphics/java/android/graphics/Region.java +++ b/graphics/java/android/graphics/Region.java @@ -30,7 +30,7 @@ public class Region implements Parcelable { /** * @hide */ - public final long mNativeRegion; + public long mNativeRegion; // the native values for these must match up with the enum in SkRegion.h public enum Op { @@ -380,6 +380,7 @@ public class Region implements Parcelable { protected void finalize() throws Throwable { try { nativeDestructor(mNativeRegion); + mNativeRegion = 0; } finally { super.finalize(); } diff --git a/graphics/java/android/graphics/RegionIterator.java b/graphics/java/android/graphics/RegionIterator.java index 8401adba09c46..443b23c1b5fc7 100644 --- a/graphics/java/android/graphics/RegionIterator.java +++ b/graphics/java/android/graphics/RegionIterator.java @@ -43,12 +43,13 @@ public class RegionIterator { protected void finalize() throws Throwable { nativeDestructor(mNativeIter); + mNativeIter = 0; // Other finalizers can still call us. } private static native long nativeConstructor(long native_region); private static native void nativeDestructor(long native_iter); private static native boolean nativeNext(long native_iter, Rect r); - private final long mNativeIter; + private long mNativeIter; } diff --git a/graphics/java/android/graphics/Shader.java b/graphics/java/android/graphics/Shader.java index 6934955b86b62..c9c7a32bb8647 100644 --- a/graphics/java/android/graphics/Shader.java +++ b/graphics/java/android/graphics/Shader.java @@ -90,6 +90,7 @@ public class Shader { super.finalize(); } finally { nativeDestructor(native_instance); + native_instance = 0; // Other finalizers can still call us. } } diff --git a/graphics/java/android/graphics/Typeface.java b/graphics/java/android/graphics/Typeface.java index db42314447ffa..7eb5584a557a5 100644 --- a/graphics/java/android/graphics/Typeface.java +++ b/graphics/java/android/graphics/Typeface.java @@ -358,6 +358,7 @@ public class Typeface { protected void finalize() throws Throwable { try { nativeUnref(native_instance); + native_instance = 0; // Other finalizers can still call us. } finally { super.finalize(); }