Skip to content

Commit

Permalink
AImageReader CFI crash mitigations
Browse files Browse the repository at this point in the history
Revert "gpu/android: Remove setup for disabling AImageReader."
This reverts commit dcd5a39.

Revert "Remove flags to enable/disable AImageReader."
This reverts commit 463fa0f.

Restore GPU bug blacklist for AImageReader on ARM and Qualcomm CPUs

Restore the AImageReader blacklist for ARM/Qualcomm chipsets which causes
crashes on Android 9 and 10 (at different code locations).

See discussions at:
* bromite/bromite#445
* bromite/bromite#814
* bromite/bromite#1005

License: GPL-3.0-only - https://spdx.org/licenses/GPL-3.0-only.html

Change-Id: I0db87f4107c94f68e846f06f365b8eefd0076599
  • Loading branch information
csagan5 authored and chirayudesai committed Oct 11, 2023
1 parent 3ae3736 commit 08b8f72
Show file tree
Hide file tree
Showing 12 changed files with 66 additions and 5 deletions.
8 changes: 7 additions & 1 deletion base/android/android_image_reader_compat.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,21 @@
namespace base {
namespace android {

bool AndroidImageReader::disable_support_ = false;

AndroidImageReader& AndroidImageReader::GetInstance() {
// C++11 static local variable initialization is
// thread-safe.
static AndroidImageReader instance;
return instance;
}

void AndroidImageReader::DisableSupport() {
disable_support_ = true;
}

bool AndroidImageReader::IsSupported() {
return is_supported_;
return !disable_support_ && is_supported_;
}

AndroidImageReader::AndroidImageReader() : is_supported_(LoadFunctions()) {}
Expand Down
4 changes: 4 additions & 0 deletions base/android/android_image_reader_compat.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ class BASE_EXPORT AndroidImageReader {
AndroidImageReader(const AndroidImageReader&) = delete;
AndroidImageReader& operator=(const AndroidImageReader&) = delete;

// Disable image reader support.
static void DisableSupport();

// Check if the image reader usage is supported. This function returns TRUE
// if android version is >=OREO, image reader support is not disabled and all
// the required functions are loaded.
Expand Down Expand Up @@ -61,6 +64,7 @@ class BASE_EXPORT AndroidImageReader {
jobject ANativeWindow_toSurface(JNIEnv* env, ANativeWindow* window);

private:
static bool disable_support_;
friend class base::NoDestructor<AndroidImageReader>;

AndroidImageReader();
Expand Down
6 changes: 3 additions & 3 deletions chrome/browser/flag-metadata.json
Original file line number Diff line number Diff line change
Expand Up @@ -2957,9 +2957,9 @@
"expiry_milestone": 125
},
{
"name": "enable-image-reader",
"owners": [ "vikassoni", "liberato" ],
"expiry_milestone": 125
"name": "enable-image-reader", // Bromite: do not expire
"owners": [ "vikassoni", "liberato" ], // flag
"expiry_milestone": -1
},
{
"name": "enable-immersive-fullscreen-toolbar",
Expand Down
16 changes: 16 additions & 0 deletions gpu/config/gpu_driver_bug_list.json
Original file line number Diff line number Diff line change
Expand Up @@ -3688,6 +3688,22 @@
"no_downscaled_overlay_promotion"
]
},
{
"id":335,
"cr_bugs": [1051705],
"description": "Disable AImageReader on ARM GPUs",
"os": {
"type": "android",
"version": {
"op": "<",
"value": "10"
}
},
"gl_vendor": "ARM.*|Qualcomm.*",
"features": [
"disable_aimagereader"
]
},
{
"id": 381,
"cr_bugs": [
Expand Down
5 changes: 5 additions & 0 deletions gpu/config/gpu_finch_features.cc
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,11 @@ BASE_FEATURE(kUseGles2ForOopR,
#endif
);


// Use android AImageReader when playing videos with MediaPlayer.
const base::Feature kAImageReaderMediaPlayer{"AImageReaderMediaPlayer",
base::FEATURE_ENABLED_BY_DEFAULT};

#if BUILDFLAG(IS_ANDROID)
// Use android SurfaceControl API for managing display compositor's buffer queue
// and using overlays on Android. Also used by webview to disable surface
Expand Down
1 change: 1 addition & 0 deletions gpu/config/gpu_finch_features.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ namespace features {
GPU_EXPORT BASE_DECLARE_FEATURE(kUseGles2ForOopR);

// All features in alphabetical order. The features should be documented
GPU_EXPORT extern const base::Feature kAImageReaderMediaPlayer;
// alongside the definition of their values in the .cc file.
#if BUILDFLAG(IS_ANDROID)
GPU_EXPORT BASE_DECLARE_FEATURE(kAndroidSurfaceControl);
Expand Down
8 changes: 8 additions & 0 deletions gpu/config/gpu_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ GpuFeatureStatus GetAndroidSurfaceControlFeatureStatus(
#if !BUILDFLAG(IS_ANDROID)
return kGpuFeatureStatusDisabled;
#else
if (blocklisted_features.count(GPU_FEATURE_TYPE_ANDROID_SURFACE_CONTROL))
return kGpuFeatureStatusBlocklisted;

if (!gpu_preferences.enable_android_surface_control)
return kGpuFeatureStatusDisabled;

Expand Down Expand Up @@ -347,6 +350,11 @@ void AdjustGpuFeatureStatusToWorkarounds(GpuFeatureInfo* gpu_feature_info) {
gpu_feature_info->status_values[GPU_FEATURE_TYPE_CANVAS_OOP_RASTERIZATION] =
kGpuFeatureStatusBlocklisted;
}

if (gpu_feature_info->IsWorkaroundEnabled(DISABLE_AIMAGEREADER)) {
gpu_feature_info->status_values[GPU_FEATURE_TYPE_ANDROID_SURFACE_CONTROL] =
kGpuFeatureStatusBlocklisted;
}
}

// Estimates roughly user total disk space by counting in the drives where
Expand Down
1 change: 1 addition & 0 deletions gpu/config/gpu_workaround_list.txt
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ decode_encode_srgb_for_generatemipmap
depth_stencil_renderbuffer_resize_emulation
disable_2d_canvas_auto_flush
disable_accelerated_av1_decode
disable_aimagereader
disable_accelerated_av1_encode
disable_accelerated_h264_decode
disable_accelerated_h264_encode
Expand Down
5 changes: 5 additions & 0 deletions gpu/ipc/service/gpu_init.cc
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,11 @@ bool GpuInit::InitializeAndStartSandbox(base::CommandLine* command_line,
}
#endif // BUILDFLAG(IS_WIN)

// Disable AImageReader if the workaround is enabled.
if (gpu_feature_info_.IsWorkaroundEnabled(DISABLE_AIMAGEREADER)) {
base::android::AndroidImageReader::DisableSupport();
}

if (gpu_feature_info_.status_values[GPU_FEATURE_TYPE_VULKAN] !=
kGpuFeatureStatusEnabled ||
!InitializeVulkan()) {
Expand Down
11 changes: 10 additions & 1 deletion gpu/ipc/service/stream_texture_android.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include <string.h>

#include "base/android/android_image_reader_compat.h"
#include "base/android/scoped_hardware_buffer_fence_sync.h"
#include "base/feature_list.h"
#include "base/functional/bind.h"
Expand Down Expand Up @@ -50,7 +51,15 @@ std::unique_ptr<ui::ScopedMakeCurrent> MakeCurrent(
}

TextureOwner::Mode GetTextureOwnerMode() {
return features::IsAImageReaderEnabled()
const bool a_image_reader_supported =
base::android::AndroidImageReader::GetInstance().IsSupported();

// TODO(vikassoni) : Currently we have 2 different flags to enable/disable
// AImageReader - one for MCVD and other for MediaPlayer here. Merge those 2
// flags into a single flag. Keeping the 2 flags separate for now since finch
// experiment using this flag is in progress.
return a_image_reader_supported && features::IsAImageReaderEnabled() &&
base::FeatureList::IsEnabled(features::kAImageReaderMediaPlayer)
? TextureOwner::Mode::kAImageReaderInsecure
: TextureOwner::Mode::kSurfaceTextureInsecure;
}
Expand Down
5 changes: 5 additions & 0 deletions media/base/media_switches.cc
Original file line number Diff line number Diff line change
Expand Up @@ -985,6 +985,11 @@ BASE_FEATURE(kHardwareSecureDecryptionExperiment,
// Allows automatically disabling hardware secure Content Decryption Module
// (CDM) after failures or crashes to fallback to software secure CDMs. If this
// feature is disabled, the fallback will never happen and users could be stuck
// Enables the Android Image Reader path for Video decoding(for AVDA and MCVD)
BASE_FEATURE(kAImageReaderVideoOutput,
"AImageReaderVideoOutput",
base::FEATURE_ENABLED_BY_DEFAULT);

// in playback failures.
BASE_FEATURE(kHardwareSecureDecryptionFallback,
"HardwareSecureDecryptionFallback",
Expand Down
1 change: 1 addition & 0 deletions media/base/media_switches.h
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,7 @@ MEDIA_EXPORT BASE_DECLARE_FEATURE(kVideoBlitColorAccuracy);
MEDIA_EXPORT BASE_DECLARE_FEATURE(kVideoToolboxVideoDecoder);
#endif // BUILDFLAG(IS_APPLE)
MEDIA_EXPORT BASE_DECLARE_FEATURE(kWebRTCColorAccuracy);
MEDIA_EXPORT BASE_DECLARE_FEATURE(kAImageReaderVideoOutput);
MEDIA_EXPORT BASE_DECLARE_FEATURE(kVp9kSVCHWDecoding);
MEDIA_EXPORT BASE_DECLARE_FEATURE(kWebContentsCaptureHiDpi);
MEDIA_EXPORT BASE_DECLARE_FEATURE(kWebrtcMediaCapabilitiesParameters);
Expand Down

0 comments on commit 08b8f72

Please sign in to comment.