facebook-api: properly handle optional Thrift fields and scoping

The plugin is required to read Thrift data for the presence states of
contacts. The data which is being read has some optional fields, which
are rarely not supplied. This has led to this bug being undiscovered
for quite some time.

Not only was the plugin not properly accounting for optional fields,
but also did not account for field scoping. This is not really an issue
until a Thrift list is being read, which will cause the identifier to
grow with each field read, rather than reset. The field identifier is
only relevant to its local scope, nothing more. More importantly, the
identifier must be reset with each iteration of a list.

4 files changed, 132 insertions, 85 deletions

diff --git a/facebook/facebook-api.c b/facebook/facebook-api.c
index 3461e2a..6b30bcd 100644
--- a/facebook/facebook-api.c
+++ b/facebook/facebook-api.c
@@ -808,66 +808,66 @@ fb_api_cb_mqtt_open(FbMqtt *mqtt, gpointer data)
     thft = fb_thrift_new(NULL, 0);
 
     /* Write the client identifier */
-    fb_thrift_write_field(thft, FB_THRIFT_TYPE_STRING, 1);
+    fb_thrift_write_field(thft, FB_THRIFT_TYPE_STRING, 1, 0);
     fb_thrift_write_str(thft, priv->cid);
 
-    fb_thrift_write_field(thft, FB_THRIFT_TYPE_STRUCT, 4);
+    fb_thrift_write_field(thft, FB_THRIFT_TYPE_STRUCT, 4, 1);
 
     /* Write the user identifier */
-    fb_thrift_write_field(thft, FB_THRIFT_TYPE_I64, 5);
+    fb_thrift_write_field(thft, FB_THRIFT_TYPE_I64, 1, 0);
     fb_thrift_write_i64(thft, priv->uid);
 
     /* Write the information string */
-    fb_thrift_write_field(thft, FB_THRIFT_TYPE_STRING, 6);
+    fb_thrift_write_field(thft, FB_THRIFT_TYPE_STRING, 2, 1);
     fb_thrift_write_str(thft, "");
 
     /* Write the UNKNOWN ("cp"?) */
-    fb_thrift_write_field(thft, FB_THRIFT_TYPE_I64, 7);
+    fb_thrift_write_field(thft, FB_THRIFT_TYPE_I64, 3, 2);
     fb_thrift_write_i64(thft, 23);
 
     /* Write the UNKNOWN ("ecp"?) */
-    fb_thrift_write_field(thft, FB_THRIFT_TYPE_I64, 8);
+    fb_thrift_write_field(thft, FB_THRIFT_TYPE_I64, 4, 3);
     fb_thrift_write_i64(thft, 26);
 
     /* Write the UNKNOWN */
-    fb_thrift_write_field(thft, FB_THRIFT_TYPE_I32, 9);
+    fb_thrift_write_field(thft, FB_THRIFT_TYPE_I32, 5, 4);
     fb_thrift_write_i32(thft, 1);
 
     /* Write the UNKNOWN ("no_auto_fg"?) */
-    fb_thrift_write_field(thft, FB_THRIFT_TYPE_BOOL, 10);
+    fb_thrift_write_field(thft, FB_THRIFT_TYPE_BOOL, 6, 5);
     fb_thrift_write_bool(thft, TRUE);
 
     /* Write the visibility state */
-    fb_thrift_write_field(thft, FB_THRIFT_TYPE_BOOL, 11);
+    fb_thrift_write_field(thft, FB_THRIFT_TYPE_BOOL, 7, 6);
     fb_thrift_write_bool(thft, !priv->invisible);
 
     /* Write the device identifier */
-    fb_thrift_write_field(thft, FB_THRIFT_TYPE_STRING, 12);
+    fb_thrift_write_field(thft, FB_THRIFT_TYPE_STRING, 8, 7);
     fb_thrift_write_str(thft, priv->did);
 
     /* Write the UNKNOWN ("fg"?) */
-    fb_thrift_write_field(thft, FB_THRIFT_TYPE_BOOL, 13);
+    fb_thrift_write_field(thft, FB_THRIFT_TYPE_BOOL, 9, 8);
     fb_thrift_write_bool(thft, TRUE);
 
     /* Write the UNKNOWN ("nwt"?) */
-    fb_thrift_write_field(thft, FB_THRIFT_TYPE_I32, 14);
+    fb_thrift_write_field(thft, FB_THRIFT_TYPE_I32, 10, 9);
     fb_thrift_write_i32(thft, 1);
 
     /* Write the UNKNOWN ("nwst"?) */
-    fb_thrift_write_field(thft, FB_THRIFT_TYPE_I32, 15);
+    fb_thrift_write_field(thft, FB_THRIFT_TYPE_I32, 11, 10);
     fb_thrift_write_i32(thft, 0);
 
     /* Write the MQTT identifier */
-    fb_thrift_write_field(thft, FB_THRIFT_TYPE_I64, 16);
+    fb_thrift_write_field(thft, FB_THRIFT_TYPE_I64, 12, 11);
     fb_thrift_write_i64(thft, priv->mid);
 
     /* Write the UNKNOWN */
-    fb_thrift_write_field(thft, FB_THRIFT_TYPE_LIST, 18);
+    fb_thrift_write_field(thft, FB_THRIFT_TYPE_LIST, 14, 12);
     fb_thrift_write_list(thft, FB_THRIFT_TYPE_I32, 0);
     fb_thrift_write_stop(thft);
 
     /* Write the token */
-    fb_thrift_write_field(thft, FB_THRIFT_TYPE_STRING, 19);
+    fb_thrift_write_field(thft, FB_THRIFT_TYPE_STRING, 15, 14);
     fb_thrift_write_str(thft, priv->token);
 
     /* Write the STOP for the struct */
@@ -1340,6 +1340,7 @@ fb_api_cb_publish_ms(FbApi *api, GByteArray *pload)
     JsonNode *root;
     JsonNode *node;
 
+    /* Read identifier string (for Facebook employees) */
     thft = fb_thrift_new(pload, 0);
     fb_thrift_read_str(thft, NULL);
     size = fb_thrift_get_pos(thft);
@@ -1465,87 +1466,112 @@ fb_api_cb_publish_ms(FbApi *api, GByteArray *pload)
 }
 
 static void
-fb_api_cb_publish_p(FbApi *api, GByteArray *pload)
+fb_api_cb_publish_pt(FbThrift *thft, GSList **press, GError **error)
 {
     FbApiPresence *pres;
-    FbThrift *thft;
     FbThriftType type;
+    gint16 id;
     gint32 i32;
     gint64 i64;
-    GSList *press;
     guint i;
-    guint size;
+    guint size = 0;
 
-    /* Start at 1 to skip the NULL byte */
-    thft  = fb_thrift_new(pload, 1);
-    press = NULL;
+    /* Read identifier string (for Facebook employees) */
+    FB_API_TCHK(fb_thrift_read_str(thft, NULL));
 
-    /* Skip the full list boolean field */
-    fb_thrift_read_field(thft, &type, NULL);
-    g_warn_if_fail(type == FB_THRIFT_TYPE_BOOL);
-    fb_thrift_read_bool(thft, NULL);
+    /* Read the full list boolean field */
+    FB_API_TCHK(fb_thrift_read_field(thft, &type, &id, 0));
+    FB_API_TCHK(type == FB_THRIFT_TYPE_BOOL);
+    FB_API_TCHK(id == 1);
+    FB_API_TCHK(fb_thrift_read_bool(thft, NULL));
 
     /* Read the list field */
-    fb_thrift_read_field(thft, &type, NULL);
-    g_warn_if_fail(type == FB_THRIFT_TYPE_LIST);
+    FB_API_TCHK(fb_thrift_read_field(thft, &type, &id, id));
+    FB_API_TCHK(type == FB_THRIFT_TYPE_LIST);
+    FB_API_TCHK(id == 2);
 
     /* Read the list */
-    fb_thrift_read_list(thft, &type, &size);
-    g_warn_if_fail(type == FB_THRIFT_TYPE_STRUCT);
+    FB_API_TCHK(fb_thrift_read_list(thft, &type, &size));
+    FB_API_TCHK(type == FB_THRIFT_TYPE_STRUCT);
 
     for (i = 0; i < size; i++) {
         /* Read the user identifier field */
-        fb_thrift_read_field(thft, &type, NULL);
-        g_warn_if_fail(type == FB_THRIFT_TYPE_I64);
-        fb_thrift_read_i64(thft, &i64);
+        FB_API_TCHK(fb_thrift_read_field(thft, &type, &id, 0));
+        FB_API_TCHK(type == FB_THRIFT_TYPE_I64);
+        FB_API_TCHK(id == 1);
+        FB_API_TCHK(fb_thrift_read_i64(thft, &i64));
 
         /* Read the active field */
-        fb_thrift_read_field(thft, &type, NULL);
-        g_warn_if_fail(type == FB_THRIFT_TYPE_I32);
-        fb_thrift_read_i32(thft, &i32);
+        FB_API_TCHK(fb_thrift_read_field(thft, &type, &id, id));
+        FB_API_TCHK(type == FB_THRIFT_TYPE_I32);
+        FB_API_TCHK(id == 2);
+        FB_API_TCHK(fb_thrift_read_i32(thft, &i32));
 
         pres = fb_api_presence_dup(NULL);
         pres->uid = i64;
         pres->active = i32 != 0;
-        press = g_slist_prepend(press, pres);
+        *press = g_slist_prepend(*press, pres);
 
         fb_util_debug_info("Presence: %" FB_ID_FORMAT " (%d)",
                            i64, i32 != 0);
 
-        /* Skip the last active timestamp field */
-        if (!fb_thrift_read_field(thft, &type, NULL)) {
-            continue;
-        }
+        while (id <= 5) {
+            if (fb_thrift_read_isstop(thft)) {
+                break;
+            }
 
-        g_warn_if_fail(type == FB_THRIFT_TYPE_I64);
-        fb_thrift_read_i64(thft, NULL);
+            FB_API_TCHK(fb_thrift_read_field(thft, &type, &id, id));
 
-        /* Skip the active client bits field */
-        if (!fb_thrift_read_field(thft, &type, NULL)) {
-            continue;
-        }
+            switch (id) {
+            case 3:
+                /* Read the last active timestamp field */
+                FB_API_TCHK(type == FB_THRIFT_TYPE_I64);
+                FB_API_TCHK(fb_thrift_read_i64(thft, NULL));
+                break;
 
-        g_warn_if_fail(type == FB_THRIFT_TYPE_I16);
-        fb_thrift_read_i16(thft, NULL);
+            case 4:
+                /* Read the active client bits field */
+                FB_API_TCHK(type == FB_THRIFT_TYPE_I16);
+                FB_API_TCHK(fb_thrift_read_i16(thft, NULL));
+                break;
 
-        /* Skip the VoIP compatibility bits field */
-        if (!fb_thrift_read_field(thft, &type, NULL)) {
-            continue;
-        }
+            case 5:
+                /* Read the VoIP compatibility bits field */
+                FB_API_TCHK(type == FB_THRIFT_TYPE_I64);
+                FB_API_TCHK(fb_thrift_read_i64(thft, NULL));
+                break;
 
-        g_warn_if_fail(type == FB_THRIFT_TYPE_I64);
-        fb_thrift_read_i64(thft, NULL);
+            default:
+                FB_API_TCHK(FALSE);
+                break;
+            }
+        }
 
         /* Read the field stop */
-        fb_thrift_read_stop(thft);
+        FB_API_TCHK(fb_thrift_read_stop(thft));
     }
 
     /* Read the field stop */
-    fb_thrift_read_stop(thft);
+    FB_API_TCHK(fb_thrift_read_stop(thft));
+}
+
+static void
+fb_api_cb_publish_p(FbApi *api, GByteArray *pload)
+{
+    FbThrift *thft;
+    GError *err = NULL;
+    GSList *press = NULL;
+
+    thft = fb_thrift_new(pload, 0);
+    fb_api_cb_publish_pt(thft, &press, &err);
     g_object_unref(thft);
 
-    press = g_slist_reverse(press);
-    g_signal_emit_by_name(api, "presences", press);
+    if (G_LIKELY(err == NULL)) {
+        g_signal_emit_by_name(api, "presences", press);
+    } else {
+        fb_api_error_emit(api, err);
+    }
+
     g_slist_free_full(press, (GDestroyNotify) fb_api_presence_free);
 }
 
diff --git a/facebook/facebook-api.h b/facebook/facebook-api.h
index 15e4cb2..757de9d 100644
--- a/facebook/facebook-api.h
+++ b/facebook/facebook-api.h
@@ -190,6 +190,30 @@
 #define FB_API_CONTACTS_COUNT  500
 
 /**
+ * FB_API_TCHK:
+ * @e: The expression.
+ *
+ * Checks the Thrift related expression to ensure that it evaluates to
+ * #TRUE. If the expression evaluates to #FALSE, a #GError is assigned
+ * to the local `error` variable, then returns with no value.
+ *
+ * This macro is meant to only be used for Thrift related expressions,
+ * where the calling function has a `void` return type. This macro also
+ * requires the existence of a predefined `error` variable, which is a
+ * pointer of a pointer to a #GError.
+ */
+#define FB_API_TCHK(e) \
+    G_STMT_START { \
+        if (G_UNLIKELY(!(e))) { \
+            g_set_error(error, FB_API_ERROR, FB_API_ERROR_GENERAL, \
+                        "Failed to read thrift: %s:%d " \
+                        "%s: assertion '%s' failed", \
+                        __FILE__, __LINE__, G_STRFUNC, #e); \
+            return; \
+        } \
+    } G_STMT_END
+
+/**
  * FB_API_MSGID:
  * @m: The time in milliseconds.
  * @i: The random integer.
diff --git a/facebook/facebook-thrift.c b/facebook/facebook-thrift.c
index bde1eb1..9604e4d 100644
--- a/facebook/facebook-thrift.c
+++ b/facebook/facebook-thrift.c
@@ -26,7 +26,6 @@ struct _FbThriftPrivate
     guint offset;
     guint pos;
     guint lastbool;
-    gint16 lastid;
 };
 
 G_DEFINE_TYPE(FbThrift, fb_thrift, G_TYPE_OBJECT);
@@ -330,7 +329,8 @@ fb_thrift_read_str(FbThrift *thft, gchar **value)
 }
 
 gboolean
-fb_thrift_read_field(FbThrift *thft, FbThriftType *type, gint16 *id)
+fb_thrift_read_field(FbThrift *thft, FbThriftType *type, gint16 *id,
+                     gint16 lastid)
 {
     FbThriftPrivate *priv;
     gint16 i16;
@@ -338,6 +338,7 @@ fb_thrift_read_field(FbThrift *thft, FbThriftType *type, gint16 *id)
 
     g_return_val_if_fail(FB_IS_THRIFT(thft), FALSE);
     g_return_val_if_fail(type != NULL, FALSE);
+    g_return_val_if_fail(id != NULL, FALSE);
     priv = thft->priv;
 
     if (!fb_thrift_read_byte(thft, &byte)) {
@@ -352,29 +353,22 @@ fb_thrift_read_field(FbThrift *thft, FbThriftType *type, gint16 *id)
     *type = fb_thrift_ct2t(byte & 0x0F);
     i16 = (byte & 0xF0) >> 4;
 
-    if (*type == FB_THRIFT_TYPE_BOOL) {
-        priv->lastbool = 0x01;
-
-        if ((byte & 0x0F) == 0x01) {
-            priv->lastbool |= 0x01 << 2;
-        }
-
-        return TRUE;
-    }
-
     if (i16 == 0) {
-        if (!fb_thrift_read_i16(thft, &i16)) {
+        if (!fb_thrift_read_i16(thft, id)) {
             return FALSE;
         }
     } else {
-        i16 = priv->lastid + i16;
+        *id = lastid + i16;
     }
 
-    if (id != NULL) {
-        *id = i16;
+    if (*type == FB_THRIFT_TYPE_BOOL) {
+        priv->lastbool = 0x01;
+
+        if ((byte & 0x0F) == 0x01) {
+            priv->lastbool |= 0x01 << 2;
+        }
     }
 
-    priv->lastid = i16;
     return TRUE;
 }
 
@@ -585,7 +579,8 @@ fb_thrift_write_str(FbThrift *thft, const gchar *value)
 }
 
 void
-fb_thrift_write_field(FbThrift *thft, FbThriftType type, gint16 id)
+fb_thrift_write_field(FbThrift *thft, FbThriftType type, gint16 id,
+                      gint16 lastid)
 {
     FbThriftPrivate *priv;
     gint16 diff;
@@ -598,16 +593,14 @@ fb_thrift_write_field(FbThrift *thft, FbThriftType type, gint16 id)
     }
 
     type = fb_thrift_t2ct(type);
-    diff = id - priv->lastid;
+    diff = id - lastid;
 
-    if ((id <= priv->lastid) || (diff > 0x0F)) {
+    if ((id <= lastid) || (diff > 0x0F)) {
         fb_thrift_write_byte(thft, type);
         fb_thrift_write_i16(thft, id);
     } else {
         fb_thrift_write_byte(thft, (diff << 4) | type);
     }
-
-    priv->lastid = id;
 }
 
 void
diff --git a/facebook/facebook-thrift.h b/facebook/facebook-thrift.h
index 65257c4..943825d 100644
--- a/facebook/facebook-thrift.h
+++ b/facebook/facebook-thrift.h
@@ -326,14 +326,16 @@ fb_thrift_read_str(FbThrift *thft, gchar **value);
  * fb_thrift_read_field:
  * @thft: The #FbThrift.
  * @type: The return location for the #FbThriftType.
- * @id: The return location for the identifier or #NULL.
+ * @id: The return location for the identifier.
+ * @lastid: The identifier of the previous field.
  *
  * Reads a field header from the #FbThrift.
  *
  * Returns: #TRUE if the field header was read, otherwise #FALSE.
  */
 gboolean
-fb_thrift_read_field(FbThrift *thft, FbThriftType *type, gint16 *id);
+fb_thrift_read_field(FbThrift *thft, FbThriftType *type, gint16 *id,
+                     gint16 lastid);
 
 /**
  * fb_thrift_read_stop:
@@ -520,11 +522,13 @@ fb_thrift_write_str(FbThrift *thft, const gchar *value);
  * @thft: The #FbThrift.
  * @type: The #FbThriftType.
  * @id: The identifier.
+ * @lastid: The identifier of the previous field.
  *
  * Writes a field header to the #FbThrift.
  */
 void
-fb_thrift_write_field(FbThrift *thft, FbThriftType type, gint16 id);
+fb_thrift_write_field(FbThrift *thft, FbThriftType type, gint16 id,
+                      gint16 lastid);
 
 /**
  * fb_thrift_write_stop: