From dd8a061d28ecaf286bfba3bdbd2a495c45cfb4a4 Mon Sep 17 00:00:00 2001
From: Dan Williams <dcbw@redhat.com>
Date: Thu, 14 Oct 2010 10:22:59 -0500
Subject: [PATCH] serial: make QCDM frame parsing more robust

Ensure that valid HDLC frames that are not valid QCDM frames
are correctly rejected, and that their data is correctly
discarded.

The core bug was that Sierra CnS frames have leading and trailing
HDLC frame terminator bytes (0x7E), and the code was incorrectly
treating the leading terminator as the end of a frame, not the
beginning.  Thus it would consider the outstanding serial request
finished without actually parsing the response packet.

Now, we make sure we don't tell the serial receive code that
we have a full QCDM frame until we actually do have one, which is
at least 3 bytes + 0x7E.
---
 src/mm-qcdm-serial-port.c | 105 +++++++++++++++++++++++---------------
 1 file changed, 65 insertions(+), 40 deletions(-)

diff --git a/src/mm-qcdm-serial-port.c b/src/mm-qcdm-serial-port.c
index 72463fbf..7f4302c5 100644
--- a/src/mm-qcdm-serial-port.c
+++ b/src/mm-qcdm-serial-port.c
@@ -37,22 +37,38 @@ typedef struct {
 /*****************************************************************************/
 
 static gboolean
-parse_response (MMSerialPort *port, GByteArray *response, GError **error)
+find_qcdm_start (GByteArray *response, gsize *start)
 {
-    int i;
+    int i, last = -1;
 
-    /* Look for the QCDM packet termination character; if we found it, treat
-     * the buffer as a qcdm command.
+    /* Look for 3 bytes and a QCDM frame marker, ie enough data for a valid
+     * frame.  There will usually be three cases here; (1) a QCDM frame
+     * starting with data and terminated by 0x7E, and (2) a QCDM frame starting
+     * with 0x7E and ending with 0x7E, and (3) a non-QCDM frame that still
+     * uses HDLC framing (like Sierra CnS) that starts and ends with 0x7E.
      */
     for (i = 0; i < response->len; i++) {
-        if (response->data[i] == 0x7E)
-            return TRUE;
-    }
+        if (response->data[i] == 0x7E) {
+            if (i > last + 3) {
+                /* Got a full QCDM frame; 3 non-0x7E bytes and a terminator */
+                if (start)
+                    *start = last + 1;
+                return TRUE;
+            }
 
-    /* Otherwise, need more data from the device */
+            /* Save position of the last QCDM frame marker */
+            last = i;
+        }
+    }
     return FALSE;
 }
 
+static gboolean
+parse_response (MMSerialPort *port, GByteArray *response, GError **error)
+{
+    return find_qcdm_start (response, NULL);
+}
+
 static gsize
 handle_response (MMSerialPort *port,
                  GByteArray *response,
@@ -64,41 +80,49 @@ handle_response (MMSerialPort *port,
     GByteArray *unescaped = NULL;
     GError *dm_error = NULL;
     gsize used = 0;
+    gsize start = 0;
+    gboolean success = FALSE, more = FALSE;
+    gsize unescaped_len = 0;
 
-    /* Ignore empty frames */
-    if (response->len > 0 && response->data[0] == 0x7E)
-        return 1;
+    if (error)
+        goto callback;
 
-    if (!error) {
-        gboolean more = FALSE, success;
-        gsize unescaped_len = 0;
-
-        /* FIXME: don't munge around with byte array internals */
-        unescaped = g_byte_array_sized_new (1024);
-        success = dm_decapsulate_buffer ((const char *) response->data,
-                                         response->len,
-                                         (char *) unescaped->data,
-                                         1024,
-                                         &unescaped_len,
-                                         &used,
-                                         &more);
-        if (!success) {
-            g_set_error_literal (&dm_error,
-                                 MM_MODEM_ERROR, MM_MODEM_ERROR_GENERAL,
-                                 "Failed to unescape QCDM packet.");
-            g_byte_array_free (unescaped, TRUE);
-            unescaped = NULL;
-        } else if (more) {
-            /* Need more data; we shouldn't have gotten here since the parse
-             * function checks for the end-of-frame marker, but whatever.
-             */
-            return 0;
-        } else {
-            /* Successfully decapsulated the DM command */
-            unescaped->len = (guint) unescaped_len;
-        }
+    /* Get the offset into the buffer of where the QCDM frame starts */
+    if (!find_qcdm_start (response, &start)) {
+        g_set_error_literal (&dm_error,
+                             MM_MODEM_ERROR, MM_MODEM_ERROR_GENERAL,
+                             "Failed to parse QCDM packet.");
+        /* Discard the unparsable data */
+        used = response->len;
+        goto callback;
     }
 
+    /* FIXME: don't munge around with byte array internals */
+    unescaped = g_byte_array_sized_new (1024);
+    success = dm_decapsulate_buffer ((const char *) (response->data + start),
+                                     response->len - start,
+                                     (char *) unescaped->data,
+                                     1024,
+                                     &unescaped_len,
+                                     &used,
+                                     &more);
+    if (!success) {
+        g_set_error_literal (&dm_error,
+                             MM_MODEM_ERROR, MM_MODEM_ERROR_GENERAL,
+                             "Failed to unescape QCDM packet.");
+        g_byte_array_free (unescaped, TRUE);
+        unescaped = NULL;
+    } else if (more) {
+        /* Need more data; we shouldn't have gotten here since the parse
+         * function checks for the end-of-frame marker, but whatever.
+         */
+        return 0;
+    } else {
+        /* Successfully decapsulated the DM command */
+        unescaped->len = (guint) unescaped_len;
+    }
+
+callback:
     response_callback (MM_QCDM_SERIAL_PORT (port),
                        unescaped,
                        dm_error ? dm_error : error,
@@ -106,8 +130,9 @@ handle_response (MMSerialPort *port,
 
     if (unescaped)
         g_byte_array_free (unescaped, TRUE);
+    g_clear_error (&dm_error);
 
-    return used;
+    return start + used;
 }
 
 /*****************************************************************************/