Fix possible JNI local reference overflow and races

Dumping connections payload requires creating (local) references, which
are limited to 512. This commit greatly reduces their lifetime, from
several seconds to less than 1 second, reducing the likehood of an
overflow. Moreover it adds missing synchronization to the connection
payload.

app/src/main/java/com/emanuelef/remote_capture/MitmReceiver.java

@@ -306,7 +306,7 @@ public class MitmReceiver implements Runnable, ConnectionsListener, MitmListener
             if(conn.status == ConnectionDescriptor.CONN_STATUS_CLOSED)
                 conn.status = ConnectionDescriptor.CONN_STATUS_CLIENT_ERROR;
         } else
-            conn.addPayloadChunk(new PayloadChunk(message, getChunkType(type), isSent(type), tstamp));
+            conn.addPayloadChunkMitm(new PayloadChunk(message, getChunkType(type), isSent(type), tstamp));
     }
 
     private synchronized void addPendingMessage(PendingMessage pending) {

app/src/main/java/com/emanuelef/remote_capture/model/ConnectionDescriptor.java

@@ -101,7 +101,7 @@ public class ConnectionDescriptor {
     public String info;
     public String url;
     public String l7proto;
-    private ArrayList<PayloadChunk> payload_chunks;
+    private final ArrayList<PayloadChunk> payload_chunks; // must be synchronized
     public final int uid;
     public final int ifidx;
     public final int incr_id;
@@ -140,6 +140,7 @@ public class ConnectionDescriptor {
         l7proto = "";
         country = "";
         asn = new Geomodel.ASN();
+        payload_chunks = new ArrayList<>();
         mitm_decrypt = _mitm_decrypt;
     }
 
@@ -180,8 +181,10 @@ public class ConnectionDescriptor {
             // Some pending updates with payload may still be received after low memory has been
             // triggered and payload disabled
             if(!CaptureService.isLowMemory()) {
-                payload_chunks = update.payload_chunks;
-                payload_truncated = update.payload_truncated;
+                synchronized (this) {
+                    payload_chunks.addAll(update.payload_chunks);
+                    payload_truncated = update.payload_truncated;
+                }
             }
         }
     }
@@ -294,29 +297,24 @@ public class ConnectionDescriptor {
     public boolean isDecrypted()        { return !isNotDecryptable() && (getNumPayloadChunks() > 0); }
     public boolean isCleartext()        { return !encrypted_payload && !encrypted_l7; }
 
-    public int getNumPayloadChunks() { return (payload_chunks == null) ? 0 : payload_chunks.size(); }
+    public synchronized int getNumPayloadChunks() { return payload_chunks.size(); }
 
-    public @Nullable PayloadChunk getPayloadChunk(int idx) {
+    public synchronized @Nullable PayloadChunk getPayloadChunk(int idx) {
         if(getNumPayloadChunks() <= idx)
             return null;
         return payload_chunks.get(idx);
     }
 
-    public void addPayloadChunk(PayloadChunk chunk) {
-        if(payload_chunks == null)
-            payload_chunks = new ArrayList<>();
+    public synchronized void addPayloadChunkMitm(PayloadChunk chunk) {
         payload_chunks.add(chunk);
         payload_length += chunk.payload.length;
     }
 
-    public void dropPayload() {
-        payload_chunks = null;
+    public synchronized void dropPayload() {
+        payload_chunks.clear();
     }
 
-    private boolean hasHttp(boolean is_sent) {
-        if(getNumPayloadChunks() == 0)
-            return false;
-
+    private synchronized boolean hasHttp(boolean is_sent) {
         for(PayloadChunk chunk: payload_chunks) {
             if(chunk.is_sent == is_sent)
                 return (chunk.type == PayloadChunk.ChunkType.HTTP);
@@ -327,7 +325,7 @@ public class ConnectionDescriptor {
     public boolean hasHttpRequest() { return hasHttp(true); }
     public boolean hasHttpResponse() { return hasHttp(false); }
 
-    private String getHttp(boolean is_sent) {
+    private synchronized String getHttp(boolean is_sent) {
         if(getNumPayloadChunks() == 0)
             return "";
 

app/src/main/jni/core/jni_impl.c

@@ -172,9 +172,12 @@ static jobject getConnUpdate(pcapdroid_t *pd, const conn_and_tuple_t *conn) {
         (*env)->DeleteLocalRef(env, url);
         (*env)->DeleteLocalRef(env, l7proto);
     }
-    if(data->update_type & CONN_UPDATE_PAYLOAD)
+    if(data->update_type & CONN_UPDATE_PAYLOAD) {
         (*env)->CallVoidMethod(env, update, mids.connUpdateSetPayload, data->payload_chunks,
                                data->payload_truncated);
+        (*pd->env)->DeleteLocalRef(pd->env, data->payload_chunks);
+        data->payload_chunks = NULL;
+    }
 
     // reset the update flag
     data->update_type = 0;
@@ -273,6 +276,8 @@ static int dumpConnectionUpdate(pcapdroid_t *pd, const conn_and_tuple_t *conn, j
 /* Perform a full dump of the active connections */
 static void sendConnectionsDump(pcapdroid_t *pd) {
     JNIEnv *env = pd->env;
+    //jniDumpReferences(env);
+
     jobject new_conns = (*env)->NewObjectArray(env, pd->new_conns.cur_items, cls.conn, NULL);
     jobject conns_updates = (*env)->NewObjectArray(env, pd->conns_updates.cur_items, cls.conn_update, NULL);
 
@@ -311,6 +316,7 @@ static void sendConnectionsDump(pcapdroid_t *pd) {
 cleanup:
     (*env)->DeleteLocalRef(env, new_conns);
     (*env)->DeleteLocalRef(env, conns_updates);
+    //jniDumpReferences(env);
 }
 
 /* ******************************************************* */
@@ -405,6 +411,9 @@ static bool dumpPayloadChunk(struct pcapdroid *pd, const pkt_context_t *pctx, in
     bool rv = false;
 
     if(pctx->data->payload_chunks == NULL) {
+        // Directly allocating an ArrayList<bytes> rather than creating it afterwards saves us from a data copy.
+        // However, this creates a local reference, which is retained until sendConnectionsDump is called.
+        // NOTE: Android only allows up to 512 local references.
         pctx->data->payload_chunks = (*env)->NewObject(env, cls.arraylist, mids.arraylistNew);
         if((pctx->data->payload_chunks == NULL) || jniCheckException(env))
             return false;