Fix race condition in at_send_command_*

b/34883617

Change-Id: I946125e1c4db44e05afbc0e5d1fb22cc880f1d89
Signed-off-by: Weilun Du <wdu@google.com>

4 files changed, 41 insertions, 18 deletions

diff --git a/reference-ril/atchannel.c b/reference-ril/atchannel.c
index 5dc3e3c..0041836 100644
--- a/reference-ril/atchannel.c
+++ b/reference-ril/atchannel.c
@@ -61,12 +61,17 @@ void  AT_DUMP(const char*  prefix, const char*  buff, int  len)
 #endif
 
 /*
- * for current pending command
- * these are protected by s_commandmutex
+ * There is one reader thread |s_tid_reader| and potentially multiple writer
+ * threads. |s_commandmutex| and |s_commandcond| are used to maintain the
+ * condition that the writer thread will not read from |sp_response| until the
+ * reader thread has signaled itself is finished, etc. |s_writeMutex| is used to
+ * prevent multiple writer threads from calling at_send_command_full_nolock
+ * function at the same time.
  */
 
 static pthread_mutex_t s_commandmutex = PTHREAD_MUTEX_INITIALIZER;
 static pthread_cond_t s_commandcond = PTHREAD_COND_INITIALIZER;
+static pthread_mutex_t s_writeMutex = PTHREAD_MUTEX_INITIALIZER;
 
 static ATCommandType s_type;
 static const char *s_responsePrefix = NULL;
@@ -736,12 +741,16 @@ static int at_send_command_full (const char *command, ATCommandType type,
                     long long timeoutMsec, ATResponse **pp_outResponse)
 {
     int err;
+    bool inEmulator;
 
     if (0 != pthread_equal(s_tid_reader, pthread_self())) {
         /* cannot be called from reader thread */
         return AT_ERROR_INVALID_THREAD;
     }
-
+    inEmulator = isInEmulator();
+    if (inEmulator) {
+        pthread_mutex_lock(&s_writeMutex);
+    }
     pthread_mutex_lock(&s_commandmutex);
 
     err = at_send_command_full_nolock(command, type,
@@ -749,6 +758,9 @@ static int at_send_command_full (const char *command, ATCommandType type,
                     timeoutMsec, pp_outResponse);
 
     pthread_mutex_unlock(&s_commandmutex);
+    if (inEmulator) {
+        pthread_mutex_unlock(&s_writeMutex);
+    }
 
     if (err == AT_ERROR_TIMEOUT && s_onTimeout != NULL) {
         s_onTimeout();
@@ -888,12 +900,16 @@ int at_handshake()
 {
     int i;
     int err = 0;
+    bool inEmulator;
 
     if (0 != pthread_equal(s_tid_reader, pthread_self())) {
         /* cannot be called from reader thread */
         return AT_ERROR_INVALID_THREAD;
     }
-
+    inEmulator = isInEmulator();
+    if (inEmulator) {
+        pthread_mutex_lock(&s_writeMutex);
+    }
     pthread_mutex_lock(&s_commandmutex);
 
     for (i = 0 ; i < HANDSHAKE_RETRY_COUNT ; i++) {
@@ -914,6 +930,9 @@ int at_handshake()
     }
 
     pthread_mutex_unlock(&s_commandmutex);
+    if (inEmulator) {
+        pthread_mutex_unlock(&s_writeMutex);
+    }
 
     return err;
 }
diff --git a/reference-ril/misc.c b/reference-ril/misc.c
index e4b8d72..c0e9b6e 100644
--- a/reference-ril/misc.c
+++ b/reference-ril/misc.c
@@ -14,7 +14,9 @@
 ** See the License for the specific language governing permissions and
 ** limitations under the License.
 */
+#include <sys/system_properties.h>
 
+#include "misc.h"
 /** returns 1 if line starts with prefix, 0 if it does not */
 int strStartsWith(const char *line, const char *prefix)
 {
@@ -27,3 +29,12 @@ int strStartsWith(const char *line, const char *prefix)
     return *prefix == '\0';
 }
 
+// Returns true iff running this process in an emulator VM
+bool isInEmulator(void) {
+  static int inQemu = -1;
+  if (inQemu < 0) {
+      char propValue[PROP_VALUE_MAX];
+      inQemu = (__system_property_get("ro.kernel.qemu", propValue) != 0);
+  }
+  return inQemu == 1;
+}
diff --git a/reference-ril/misc.h b/reference-ril/misc.h
index 7044a07..9e65ab3 100644
--- a/reference-ril/misc.h
+++ b/reference-ril/misc.h
@@ -14,6 +14,9 @@
 ** See the License for the specific language governing permissions and
 ** limitations under the License.
 */
+#include <stdbool.h>
 
 /** returns 1 if line starts with prefix, 0 if it does not */
 int strStartsWith(const char *line, const char *prefix);
+/** Returns true iff running this process in an emulator VM */
+bool isInEmulator(void);
\ No newline at end of file
diff --git a/reference-ril/reference-ril.c b/reference-ril/reference-ril.c
index 997195a..d1c4455 100644
--- a/reference-ril/reference-ril.c
+++ b/reference-ril/reference-ril.c
@@ -17,7 +17,6 @@
 
 #include <telephony/ril_cdma_sms.h>
 #include <telephony/librilutils.h>
-#include <stdbool.h>
 #include <stdio.h>
 #include <assert.h>
 #include <string.h>
@@ -36,8 +35,8 @@
 #include <getopt.h>
 #include <sys/socket.h>
 #include <cutils/sockets.h>
-#include <termios.h>
 #include <sys/system_properties.h>
+#include <termios.h>
 #include <system/qemu_pipe.h>
 
 #include "ril.h"
@@ -241,15 +240,6 @@ static int s_repollCallsCount = 0;
 static int s_expectAnswer = 0;
 #endif /* WORKAROUND_ERRONEOUS_ANSWER */
 
-// Returns true iff running this process in an emulator VM
-static bool isInEmulator(void) {
-    static int inQemu = -1;
-    if (inQemu < 0) {
-        char propValue[PROP_VALUE_MAX];
-        inQemu = (__system_property_get("ro.kernel.qemu", propValue) != 0);
-    }
-    return inQemu == 1;
-}
 
 static int s_cell_info_rate_ms = INT_MAX;
 static int s_mcc = 0;
@@ -2127,9 +2117,9 @@ static void requestGetHardwareConfig(void *data, size_t datalen, RIL_Token t)
  * RIL_onRequestComplete() may be called from any thread, before or after
  * this function returns.
  *
- * Will always be called from the same thread, so returning here implies
- * that the radio is ready to process another command (whether or not
- * the previous command has completed).
+ * Because onRequest function could be called from multiple different thread,
+ * we must ensure that the underlying at_send_command_* function
+ * is atomic.
  */
 static void
 onRequest (int request, void *data, size_t datalen, RIL_Token t)