Merge "graphics: fix a potential use after free"
diff --git a/audio/2.0/default/Stream.cpp b/audio/2.0/default/Stream.cpp
index c16a956..29946fe 100644
--- a/audio/2.0/default/Stream.cpp
+++ b/audio/2.0/default/Stream.cpp
@@ -189,7 +189,9 @@
}
Return<AudioDevice> Stream::getDevice() {
- return AudioDevice(mStream->get_device(mStream));
+ int device;
+ Result retval = getParam(AudioParameter::keyRouting, &device);
+ return retval == Result::OK ? static_cast<AudioDevice>(device) : AudioDevice::NONE;
}
Return<Result> Stream::setDevice(const DeviceAddress& address) {
diff --git a/audio/2.0/vts/functional/AudioPrimaryHidlHalTest.cpp b/audio/2.0/vts/functional/AudioPrimaryHidlHalTest.cpp
index cc20456..a3f0d27 100644
--- a/audio/2.0/vts/functional/AudioPrimaryHidlHalTest.cpp
+++ b/audio/2.0/vts/functional/AudioPrimaryHidlHalTest.cpp
@@ -32,6 +32,7 @@
#include <android/hardware/audio/2.0/IDevice.h>
#include <android/hardware/audio/2.0/IDevicesFactory.h>
+#include <android/hardware/audio/2.0/IPrimaryDevice.h>
#include <android/hardware/audio/2.0/types.h>
#include <android/hardware/audio/common/2.0/types.h>
@@ -50,6 +51,8 @@
using ::android::hardware::hidl_vec;
using ::android::hardware::audio::V2_0::DeviceAddress;
using ::android::hardware::audio::V2_0::IDevice;
+using ::android::hardware::audio::V2_0::IPrimaryDevice;
+using TtyMode = ::android::hardware::audio::V2_0::IPrimaryDevice::TtyMode;
using ::android::hardware::audio::V2_0::IDevicesFactory;
using ::android::hardware::audio::V2_0::IStream;
using ::android::hardware::audio::V2_0::IStreamIn;
@@ -63,6 +66,7 @@
using ::android::hardware::audio::common::V2_0::AudioHandleConsts;
using ::android::hardware::audio::common::V2_0::AudioInputFlag;
using ::android::hardware::audio::common::V2_0::AudioIoHandle;
+using ::android::hardware::audio::common::V2_0::AudioMode;
using ::android::hardware::audio::common::V2_0::AudioOffloadInfo;
using ::android::hardware::audio::common::V2_0::AudioOutputFlag;
using ::android::hardware::audio::common::V2_0::AudioSource;
@@ -154,19 +158,23 @@
ASSERT_NO_FATAL_FAILURE(AudioHidlTest::SetUp()); // setup base
if (device == nullptr) {
- environment->registerTearDown([]{ device.clear(); });
IDevicesFactory::Result result;
+ sp<IDevice> baseDevice;
ASSERT_OK(devicesFactory->openDevice(IDevicesFactory::Device::PRIMARY,
- returnIn(result, device)));
+ returnIn(result, baseDevice)));
+ ASSERT_TRUE(baseDevice != nullptr);
+
+ environment->registerTearDown([]{ device.clear(); });
+ device = IPrimaryDevice::castFrom(baseDevice);
+ ASSERT_TRUE(device != nullptr);
}
- ASSERT_TRUE(device != nullptr);
}
protected:
// Cache the device opening to speed up each test by ~0.5s
- static sp<IDevice> device;
+ static sp<IPrimaryDevice> device;
};
-sp<IDevice> AudioPrimaryHidlTest::device;
+sp<IPrimaryDevice> AudioPrimaryHidlTest::device;
TEST_F(AudioPrimaryHidlTest, OpenPrimaryDevice) {
doc::test("Test the openDevice (called in SetUp)");
@@ -234,42 +242,64 @@
////////////////////////// {set,get}{Master,Mic}Mute /////////////////////////
//////////////////////////////////////////////////////////////////////////////
-class BoolAccessorPrimaryHidlTest : public AudioPrimaryHidlTest {
+template <class Property>
+class AccessorPrimaryHidlTest : public AudioPrimaryHidlTest {
protected:
+
+ /** Test a property getter and setter. */
template <class Getter, class Setter>
- void testBoolAccessors(const string& propertyName, const vector<bool>& valuesToTest,
- Setter setter, Getter getter) {
- for (bool setState : valuesToTest) {
- SCOPED_TRACE("Test " + propertyName + " state: " + to_string(setState));
- ASSERT_OK((device.get()->*setter)(setState));
- bool getState;
- ASSERT_OK((device.get()->*getter)(returnIn(res, getState)));
+ void testAccessors(const string& propertyName, const vector<Property>& valuesToTest,
+ Setter setter, Getter getter) {
+
+ Property initialValue; // Save initial value to restore it at the end of the test
+ ASSERT_OK((device.get()->*getter)(returnIn(res, initialValue)));
+ ASSERT_OK(res);
+
+ for (Property setValue : valuesToTest) {
+ SCOPED_TRACE("Test " + propertyName + " getter and setter for " + testing::PrintToString(setValue));
+ ASSERT_OK((device.get()->*setter)(setValue));
+ Property getValue;
+ // Make sure the getter returns the same value just set
+ ASSERT_OK((device.get()->*getter)(returnIn(res, getValue)));
ASSERT_OK(res);
- ASSERT_EQ(setState, getState);
+ EXPECT_EQ(setValue, getValue);
}
+
+ ASSERT_OK((device.get()->*setter)(initialValue)); // restore initial value
+ }
+
+ /** Test the getter and setter of an optional feature. */
+ template <class Getter, class Setter>
+ void testOptionalAccessors(const string& propertyName, const vector<Property>& valuesToTest,
+ Setter setter, Getter getter) {
+ doc::test("Test the optional " + propertyName + " getters and setter");
+ {
+ SCOPED_TRACE("Test feature support by calling the getter");
+ Property initialValue;
+ ASSERT_OK((device.get()->*getter)(returnIn(res, initialValue)));
+ if (res == Result::NOT_SUPPORTED) {
+ doc::partialTest(propertyName + " getter is not supported");
+ return;
+ }
+ ASSERT_OK(res); // If it is supported it must succeed
+ }
+ // The feature is supported, test it
+ testAccessors(propertyName, valuesToTest, setter, getter);
}
};
+using BoolAccessorPrimaryHidlTest = AccessorPrimaryHidlTest<bool>;
+
TEST_F(BoolAccessorPrimaryHidlTest, MicMuteTest) {
doc::test("Check that the mic can be muted and unmuted");
- testBoolAccessors("mic mute", {true, false, true}, &IDevice::setMicMute, &IDevice::getMicMute);
+ testAccessors("mic mute", {true, false, true}, &IDevice::setMicMute, &IDevice::getMicMute);
// TODO: check that the mic is really muted (all sample are 0)
}
TEST_F(BoolAccessorPrimaryHidlTest, MasterMuteTest) {
doc::test("If master mute is supported, try to mute and unmute the master output");
- {
- SCOPED_TRACE("Check for master mute support");
- auto ret = device->setMasterMute(false);
- ASSERT_TRUE(ret.isOk());
- if (ret == Result::NOT_SUPPORTED) {
- doc::partialTest("Master mute is not supported");
- return;
- }
- }
- // NOTE: this code has never been tested on a platform supporting MasterMute
- testBoolAccessors("master mute", {true, false, true},
- &IDevice::setMasterMute, &IDevice::getMasterMute);
+ testOptionalAccessors("master mute", {true, false, true},
+ &IDevice::setMasterMute, &IDevice::getMasterMute);
// TODO: check that the master volume is really muted
}
@@ -618,8 +648,8 @@
stream->getAudioProperties(returnIn(sampleRateHz, mask, format));
// FIXME: the qcom hal it does not currently negotiate the sampleRate & channel mask
- // EXPECT_EQ(expectedConfig.sampleRateHz, sampleRateHz);
- // EXPECT_EQ(expectedConfig.channelMask, mask);
+ EXPECT_EQ(expectedConfig.sampleRateHz, sampleRateHz);
+ EXPECT_EQ(expectedConfig.channelMask, mask);
EXPECT_EQ(expectedConfig.format, format);
}
@@ -631,11 +661,11 @@
auto sampleRate = extract(stream->getSampleRate());
// FIXME: the qcom hal it does not currently negotiate the sampleRate
- // ASSERT_EQ(audioConfig.sampleRateHz, sampleRate);
+ ASSERT_EQ(audioConfig.sampleRateHz, sampleRate);
auto channelMask = extract(stream->getChannelMask());
// FIXME: the qcom hal it does not currently negotiate the channelMask
- // ASSERT_EQ(audioConfig.channelMask, channelMask);
+ ASSERT_EQ(audioConfig.channelMask, channelMask);
auto frameSize = extract(stream->getFrameSize());
ASSERT_GE(frameSize, 0U);
@@ -659,15 +689,10 @@
testGetAudioProperties(stream, audioConfig);
- // FIXME: Stream wrapper does not implement getDevice properly.
- // It needs to call getProperty({"routing"}).
- // The current implementation segfault with the default hal
- /*
- * auto ret = stream->getDevice();
- * ASSERT_TRUE(ret.isOk());
- * AudioDevice device = ret;
- * ASSERT_EQ(AudioDevice::OUT_ALL, device);
- */
+ auto ret = stream->getDevice();
+ ASSERT_TRUE(ret.isOk());
+ AudioDevice device = ret;
+ ASSERT_EQ(AudioDevice::OUT_DEFAULT, device);
}
TEST_P(InputStreamTest, GettersTest) {
@@ -695,6 +720,68 @@
}
//////////////////////////////////////////////////////////////////////////////
+/////////////////////////////// PrimaryDevice ////////////////////////////////
+//////////////////////////////////////////////////////////////////////////////
+
+TEST_F(AudioPrimaryHidlTest, setVoiceVolume) {
+ doc::test("Make sure setVoiceVolume only succeed if volume is in [0,1]");
+ for (float volume : {0.0, 0.01, 0.5, 0.09, 1.0}) {
+ SCOPED_TRACE("volume=" + to_string(volume));
+ ASSERT_OK(device->setVoiceVolume(volume));
+ }
+ for (float volume : (float[]){-INFINITY,-1.0, -0.0,
+ 1.0 + std::numeric_limits<float>::epsilon(), 2.0, INFINITY,
+ NAN}) {
+ SCOPED_TRACE("volume=" + to_string(volume));
+ // FIXME: NAN should never be accepted
+ // FIXME: Missing api doc. What should the impl do if the volume is outside [0,1] ?
+ ASSERT_INVALID_ARGUMENTS(device->setVoiceVolume(volume));
+ }
+}
+
+TEST_F(AudioPrimaryHidlTest, setMode) {
+ doc::test("Make sure setMode always succeeds if mode is valid");
+ for (AudioMode mode : {AudioMode::IN_CALL, AudioMode::IN_COMMUNICATION,
+ AudioMode::RINGTONE, AudioMode::CURRENT,
+ AudioMode::NORMAL /* Make sure to leave the test in normal mode */ }) {
+ SCOPED_TRACE("mode=" + toString(mode));
+ ASSERT_OK(device->setMode(mode));
+ }
+
+ // FIXME: Missing api doc. What should the impl do if the mode is invalid ?
+ ASSERT_INVALID_ARGUMENTS(device->setMode(AudioMode::INVALID));
+}
+
+
+TEST_F(BoolAccessorPrimaryHidlTest, BtScoNrecEnabled) {
+ doc::test("Query and set the BT SCO NR&EC state");
+ testOptionalAccessors("BtScoNrecEnabled", {true, false, true},
+ &IPrimaryDevice::setBtScoNrecEnabled,
+ &IPrimaryDevice::getBtScoNrecEnabled);
+}
+
+TEST_F(BoolAccessorPrimaryHidlTest, setGetBtScoWidebandEnabled) {
+ doc::test("Query and set the SCO whideband state");
+ testOptionalAccessors("BtScoWideband", {true, false, true},
+ &IPrimaryDevice::setBtScoWidebandEnabled,
+ &IPrimaryDevice::getBtScoWidebandEnabled);
+}
+
+using TtyModeAccessorPrimaryHidlTest = AccessorPrimaryHidlTest<TtyMode>;
+TEST_F(TtyModeAccessorPrimaryHidlTest, setGetTtyMode) {
+ doc::test("Query and set the TTY mode state");
+ testOptionalAccessors("TTY mode", {TtyMode::OFF, TtyMode::HCO, TtyMode::VCO, TtyMode::FULL},
+ &IPrimaryDevice::setTtyMode, &IPrimaryDevice::getTtyMode);
+}
+
+TEST_F(BoolAccessorPrimaryHidlTest, setGetHac) {
+ doc::test("Query and set the HAC state");
+ testAccessors("HAC", {true, false, true},
+ &IPrimaryDevice::setHacEnabled,
+ &IPrimaryDevice::getHacEnabled);
+}
+
+//////////////////////////////////////////////////////////////////////////////
//////////////////// Clean caches on global tear down ////////////////////////
//////////////////////////////////////////////////////////////////////////////
diff --git a/audio/2.0/vts/functional/utility/AssertOk.h b/audio/2.0/vts/functional/utility/AssertOk.h
index 5397436..16488ae 100644
--- a/audio/2.0/vts/functional/utility/AssertOk.h
+++ b/audio/2.0/vts/functional/utility/AssertOk.h
@@ -31,8 +31,20 @@
assertOk(result);
}
+inline void assertInvalidArguments(::android::hardware::audio::V2_0::Result result) {
+ ASSERT_EQ(decltype(result)::INVALID_ARGUMENTS, result);
+}
+
+inline void assertInvalidArguments(
+ ::android::hardware::Return<::android::hardware::audio::V2_0::Result> ret) {
+ ASSERT_TRUE(ret.isOk());
+ ::android::hardware::audio::V2_0::Result result = ret;
+ assertInvalidArguments(result);
+}
}
// Test anything provided is and contains only OK
#define ASSERT_OK(ret) ASSERT_NO_FATAL_FAILURE(detail::assertOk(ret))
#define EXPECT_OK(ret) EXPECT_NO_FATAL_FAILURE(detail::assertOk(ret))
+
+#define ASSERT_INVALID_ARGUMENTS(ret) ASSERT_NO_FATAL_FAILURE(detail::assertInvalidArguments(ret))
diff --git a/bluetooth/1.0/default/vendor_interface.cc b/bluetooth/1.0/default/vendor_interface.cc
index 2576eca..a6507dd 100644
--- a/bluetooth/1.0/default/vendor_interface.cc
+++ b/bluetooth/1.0/default/vendor_interface.cc
@@ -273,9 +273,6 @@
if (lib_interface_ != nullptr) {
bt_vendor_lpm_mode_t mode = BT_VND_LPM_DISABLE;
lib_interface_->op(BT_VND_OP_LPM_SET_MODE, &mode);
-
- int power_state = BT_VND_PWR_OFF;
- lib_interface_->op(BT_VND_OP_POWER_CTRL, &power_state);
}
fd_watcher_.StopWatchingFileDescriptors();
@@ -287,6 +284,9 @@
if (lib_interface_ != nullptr) {
lib_interface_->op(BT_VND_OP_USERIAL_CLOSE, nullptr);
+
+ int power_state = BT_VND_PWR_OFF;
+ lib_interface_->op(BT_VND_OP_POWER_CTRL, &power_state);
}
if (lib_handle_ != nullptr) {
diff --git a/configstore/1.0/ISurfaceFlingerConfigs.hal b/configstore/1.0/ISurfaceFlingerConfigs.hal
index 4403a90..318590d 100644
--- a/configstore/1.0/ISurfaceFlingerConfigs.hal
+++ b/configstore/1.0/ISurfaceFlingerConfigs.hal
@@ -16,6 +16,29 @@
package android.hardware.configstore@1.0;
interface ISurfaceFlingerConfigs {
+ /*
+ * The following two methods define (respectively):
+ *
+ * - The phase offset between hardware vsync and when apps are woken up by the
+ * Choreographer callback
+ * - The phase offset between hardware vsync and when SurfaceFlinger wakes up
+ * to consume input
+ *
+ * Their values may be tuned to trade off between display pipeline latency (both
+ * overall latency and the lengths of the app --> SF and SF --> display phases)
+ * and frame delivery jitter (which typically manifests as "jank" or "jerkiness"
+ * while interacting with the device). The default values must produce a
+ * relatively low amount of jitter at the expense of roughly two frames of
+ * app --> display latency, and unless significant testing is performed to avoid
+ * increased display jitter (both manual investigation using systrace [1] and
+ * automated testing using dumpsys gfxinfo [2] are recommended), they should not
+ * be modified.
+ *
+ * [1] https://developer.android.com/studio/profile/systrace.html
+ * [2] https://developer.android.com/training/testing/performance.html
+ */
vsyncEventPhaseOffsetNs() generates (OptionalInt64 value);
+ vsyncSfEventPhaseOffsetNs() generates (OptionalInt64 value);
+
useTripleFramebuffer() generates (OptionalBool value);
};
diff --git a/configstore/1.0/default/SurfaceFlingerConfigs.cpp b/configstore/1.0/default/SurfaceFlingerConfigs.cpp
index 5d62b15..f73ecb4 100644
--- a/configstore/1.0/default/SurfaceFlingerConfigs.cpp
+++ b/configstore/1.0/default/SurfaceFlingerConfigs.cpp
@@ -19,6 +19,16 @@
return Void();
}
+Return<void> SurfaceFlingerConfigs::vsyncSfEventPhaseOffsetNs(vsyncEventPhaseOffsetNs_cb _hidl_cb) {
+#ifdef SF_VSYNC_EVENT_PHASE_OFFSET_NS
+ _hidl_cb({true, SF_VSYNC_EVENT_PHASE_OFFSET_NS});
+ LOG(INFO) << "sfvsync event phase offset ns = " << SF_VSYNC_EVENT_PHASE_OFFSET_NS;
+#else
+ _hidl_cb({false, 0});
+#endif
+ return Void();
+}
+
Return<void> SurfaceFlingerConfigs::useTripleFramebuffer(useTripleFramebuffer_cb _hidl_cb) {
bool value = false;
#ifdef USE_TRIPLE_FRAMEBUFFER
diff --git a/configstore/1.0/default/SurfaceFlingerConfigs.h b/configstore/1.0/default/SurfaceFlingerConfigs.h
index c9652fc..bbb61d6 100644
--- a/configstore/1.0/default/SurfaceFlingerConfigs.h
+++ b/configstore/1.0/default/SurfaceFlingerConfigs.h
@@ -25,6 +25,7 @@
struct SurfaceFlingerConfigs : public ISurfaceFlingerConfigs {
// Methods from ::android::hardware::configstore::V1_0::ISurfaceFlingerConfigs follow.
Return<void> vsyncEventPhaseOffsetNs(vsyncEventPhaseOffsetNs_cb _hidl_cb) override;
+ Return<void> vsyncSfEventPhaseOffsetNs(vsyncEventPhaseOffsetNs_cb _hidl_cb) override;
Return<void> useTripleFramebuffer(useTripleFramebuffer_cb _hidl_cb) override;
// Methods from ::android::hidl::base::V1_0::IBase follow.
diff --git a/configstore/1.0/default/surfaceflinger.mk b/configstore/1.0/default/surfaceflinger.mk
index 5a946f4..49314d7 100644
--- a/configstore/1.0/default/surfaceflinger.mk
+++ b/configstore/1.0/default/surfaceflinger.mk
@@ -5,6 +5,10 @@
LOCAL_CFLAGS += -DVSYNC_EVENT_PHASE_OFFSET_NS=$(VSYNC_EVENT_PHASE_OFFSET_NS)
endif
+ifneq ($(SF_VSYNC_EVENT_PHASE_OFFSET_NS),)
+ LOCAL_CFLAGS += -DSF_VSYNC_EVENT_PHASE_OFFSET_NS=$(SF_VSYNC_EVENT_PHASE_OFFSET_NS)
+endif
+
ifeq ($(NUM_FRAMEBUFFER_SURFACE_BUFFERS),3)
LOCAL_CFLAGS += -DUSE_TRIPLE_FRAMEBUFFER
endif
diff --git a/graphics/composer/2.1/default/ComposerClient.cpp b/graphics/composer/2.1/default/ComposerClient.cpp
index cd2f049..a2d5d4b 100644
--- a/graphics/composer/2.1/default/ComposerClient.cpp
+++ b/graphics/composer/2.1/default/ComposerClient.cpp
@@ -230,30 +230,69 @@
ComposerClient::~ComposerClient()
{
- ALOGD("client destroyed");
+ // We want to call hwc2_close here (and move hwc2_open to the
+ // constructor), with the assumption that hwc2_close would
+ //
+ // - clean up all resources owned by the client
+ // - make sure all displays are blank (since there is no layer)
+ //
+ // But since SF used to crash at this point, different hwcomposer2
+ // implementations behave differently on hwc2_close. Our only portable
+ // choice really is to abort(). But that is not an option anymore
+ // because we might also have VTS or VR as clients that can come and go.
+ //
+ // Below we manually clean all resources (layers and virtual
+ // displays), and perform a presentDisplay afterwards.
+ ALOGW("destroying composer client");
mHal.enableCallback(false);
- mHal.removeClient();
- // no need to grab the mutex as any in-flight hwbinder call should keep
- // the client alive
+ // no need to grab the mutex as any in-flight hwbinder call would have
+ // kept the client alive
for (const auto& dpy : mDisplayData) {
- if (!dpy.second.Layers.empty()) {
- ALOGW("client destroyed with valid layers");
- }
+ ALOGW("destroying client resources for display %" PRIu64, dpy.first);
+
for (const auto& ly : dpy.second.Layers) {
mHal.destroyLayer(dpy.first, ly.first);
}
if (dpy.second.IsVirtual) {
- ALOGW("client destroyed with valid virtual display");
mHal.destroyVirtualDisplay(dpy.first);
+ } else {
+ ALOGW("performing a final presentDisplay");
+
+ std::vector<Layer> changedLayers;
+ std::vector<IComposerClient::Composition> compositionTypes;
+ uint32_t displayRequestMask = 0;
+ std::vector<Layer> requestedLayers;
+ std::vector<uint32_t> requestMasks;
+ mHal.validateDisplay(dpy.first, &changedLayers, &compositionTypes,
+ &displayRequestMask, &requestedLayers, &requestMasks);
+
+ mHal.acceptDisplayChanges(dpy.first);
+
+ int32_t presentFence = -1;
+ std::vector<Layer> releasedLayers;
+ std::vector<int32_t> releaseFences;
+ mHal.presentDisplay(dpy.first, &presentFence, &releasedLayers, &releaseFences);
+ if (presentFence >= 0) {
+ close(presentFence);
+ }
+ for (auto fence : releaseFences) {
+ if (fence >= 0) {
+ close(fence);
+ }
+ }
}
}
mDisplayData.clear();
sHandleImporter.cleanup();
+
+ mHal.removeClient();
+
+ ALOGW("removed composer client");
}
void ComposerClient::initialize()