From d7b17c469bc5c55cccde857a637267e5a00173a1 Mon Sep 17 00:00:00 2001
From: "John W. Bruce" <juce@google.com>
Date: Wed, 25 Sep 2019 18:02:56 -0700
Subject: [PATCH] Usage Table Failure Creates Broken CryptoSession

(This is a merge of http://go/wvgerrit/86383)

When Key Sessions were added to CryptoSession, the initialization of the
initial Key Session was placed at the end of the initialization of the
owning CryptoSession. That's all well and good except the block right
before that assumed that it was safe to abort initialization early in
order to swallow errors when setting up usage tables. As a result, if
anything caused usage table initialization to fail, it would leave the
CryptoSession without a Key Session, resulting in an inevitable segfault
further down the line.

There's no reason the Key Session can't be initialized first. This
change moves initialziation order around to avoid the bug.

Bug: 141021960
Test: CE CDM Unit Tests
Test: Android Unit Tests
Change-Id: Ic78005c831d2a24d7d6de22df54462b2bd7085f0
---
 .../cdm/core/src/crypto_session.cpp           | 26 ++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/libwvdrmengine/cdm/core/src/crypto_session.cpp b/libwvdrmengine/cdm/core/src/crypto_session.cpp
index 9f21fb63..2e82e246 100644
--- a/libwvdrmengine/cdm/core/src/crypto_session.cpp
+++ b/libwvdrmengine/cdm/core/src/crypto_session.cpp
@@ -674,6 +674,7 @@ CdmResponseType CryptoSession::Open(SecurityLevel requested_security_level) {
     return LOAD_SYSTEM_ID_ERROR;
   }
 
+  // Set up request ID
   uint64_t request_id_base;
   OEMCryptoResult random_sts;
   WithOecReadLock("Open() calling OEMCrypto_GetRandom", [&] {
@@ -688,6 +689,22 @@ CdmResponseType CryptoSession::Open(SecurityLevel requested_security_level) {
                 HexEncode(reinterpret_cast<uint8_t*>(&request_id_index),
                           sizeof(request_id_index));
 
+  // Initialize key session
+  WithOecSessionLock("Open() calling key_session_.reset()", [&] {
+    key_session_.reset(new ContentKeySession(oec_session_id_, metrics_));
+  });
+
+  // Set up Usage Table support
+  //
+  // This MUST be the last thing in the function because it contains the
+  // successful return paths of the function. It will attempt to initialize
+  // usage tables. However, whether they are employed is determined by
+  // information in the license, which will not be received until later. In case
+  // usage tables are not even needed, initialization errors here are ignored by
+  // returning successfully.
+  //
+  // TODO(b/141350978): Refactor this code so that it does not have this
+  // problem.
   if (!GetApiVersion(&api_version_)) {
     LOGE("Failed to get API version");
     return USAGE_SUPPORT_GET_API_FAILED;
@@ -735,9 +752,12 @@ CdmResponseType CryptoSession::Open(SecurityLevel requested_security_level) {
     metrics_->oemcrypto_usage_table_support_.SetError(result);
   }
 
-  WithOecSessionLock("Open() calling key_session_.reset()", [&] {
-    key_session_.reset(new ContentKeySession(oec_session_id_, metrics_));
-  });
+  // Do not add logic after this point as it may never get exercised. In the
+  // event of errors initializing the usage tables, the code will return early,
+  // never reaching this point. See the comment above the "Set up Usage Table
+  // support" section for details.
+  //
+  // TODO(b/139973602): Refactor this code.
 
   return NO_ERROR;
 }