Always kill invalid file transfers when receiving file controls.

Previously, toxcore would send a kill control to the friend only if the
file control was valid. Determining which file transfer is used does not
depend on the specific file control. We can always kill it in that case.

Also, added some logging for file control logic, since there is no other
feedback on error (failure of the file control handler is swallowed).

1 files changed, 99 insertions, 58 deletions

diff --git a/toxcore/Messenger.c b/toxcore/Messenger.c
index 48a79aa0..cd4c8380 100644
--- a/toxcore/Messenger.c
+++ b/toxcore/Messenger.c
@@ -1608,96 +1608,135 @@ static void break_files(const Messenger *m, int32_t friendnumber)
     }
 }
 
+static struct File_Transfers *get_file_transfer(uint8_t receive_send, uint8_t filenumber,
+        uint32_t *real_filenumber, Friend *sender)
+{
+    struct File_Transfers *ft;
+
+    if (receive_send == 0) {
+        *real_filenumber = (filenumber + 1) << 16;
+        ft = &sender->file_receiving[filenumber];
+    } else {
+        *real_filenumber = filenumber;
+        ft = &sender->file_sending[filenumber];
+    }
+
+    if (ft->status == FILESTATUS_NONE) {
+        return NULL;
+    }
+
+    return ft;
+}
+
 /* return -1 on failure, 0 on success.
  */
 static int handle_filecontrol(Messenger *m, int32_t friendnumber, uint8_t receive_send, uint8_t filenumber,
                               uint8_t control_type, const uint8_t *data, uint16_t length, void *userdata)
 {
     if (receive_send > 1) {
+        LOGGER_DEBUG(m->log, "file control (friend %d, file %d): receive_send value is invalid (should be 0 or 1): %d",
+                     friendnumber, filenumber, receive_send);
         return -1;
     }
 
-    if (control_type > FILECONTROL_SEEK) {
-        return -1;
-    }
-
-    uint32_t real_filenumber = filenumber;
-    struct File_Transfers *ft;
-
-    if (receive_send == 0) {
-        real_filenumber += 1;
-        real_filenumber <<= 16;
-        ft = &m->friendlist[friendnumber].file_receiving[filenumber];
-    } else {
-        ft = &m->friendlist[friendnumber].file_sending[filenumber];
-    }
+    uint32_t real_filenumber;
+    struct File_Transfers *ft = get_file_transfer(receive_send, filenumber, &real_filenumber, &m->friendlist[friendnumber]);
 
-    if (ft->status == FILESTATUS_NONE) {
-        /* File transfer doesn't exist, tell the other to kill it. */
+    if (ft == NULL) {
+        LOGGER_DEBUG(m->log, "file control (friend %d, file %d): file transfer does not exist; telling the other to kill it",
+                     friendnumber, filenumber);
         send_file_control_packet(m, friendnumber, !receive_send, filenumber, FILECONTROL_KILL, 0, 0);
         return -1;
     }
 
-    if (control_type == FILECONTROL_ACCEPT) {
-        if (receive_send && ft->status == FILESTATUS_NOT_ACCEPTED) {
-            ft->status = FILESTATUS_TRANSFERRING;
-        } else {
-            if (ft->paused & FILE_PAUSE_OTHER) {
-                ft->paused ^= FILE_PAUSE_OTHER;
+    switch (control_type) {
+        case FILECONTROL_ACCEPT: {
+            if (receive_send && ft->status == FILESTATUS_NOT_ACCEPTED) {
+                ft->status = FILESTATUS_TRANSFERRING;
             } else {
-                return -1;
+                if (ft->paused & FILE_PAUSE_OTHER) {
+                    ft->paused ^= FILE_PAUSE_OTHER;
+                } else {
+                    LOGGER_DEBUG(m->log, "file control (friend %d, file %d): friend told us to resume file transfer that wasn't paused",
+                                 friendnumber, filenumber);
+                    return -1;
+                }
             }
-        }
 
-        if (m->file_filecontrol) {
-            (*m->file_filecontrol)(m, friendnumber, real_filenumber, control_type, userdata);
-        }
-    } else if (control_type == FILECONTROL_PAUSE) {
-        if ((ft->paused & FILE_PAUSE_OTHER) || ft->status != FILESTATUS_TRANSFERRING) {
-            return -1;
+            if (m->file_filecontrol) {
+                m->file_filecontrol(m, friendnumber, real_filenumber, control_type, userdata);
+            }
+
+            return 0;
         }
 
-        ft->paused |= FILE_PAUSE_OTHER;
+        case FILECONTROL_PAUSE: {
+            if ((ft->paused & FILE_PAUSE_OTHER) || ft->status != FILESTATUS_TRANSFERRING) {
+                LOGGER_DEBUG(m->log, "file control (friend %d, file %d): friend told us to pause file transfer that is already paused",
+                             friendnumber, filenumber);
+                return -1;
+            }
 
-        if (m->file_filecontrol) {
-            (*m->file_filecontrol)(m, friendnumber, real_filenumber, control_type, userdata);
-        }
-    } else if (control_type == FILECONTROL_KILL) {
+            ft->paused |= FILE_PAUSE_OTHER;
+
+            if (m->file_filecontrol) {
+                m->file_filecontrol(m, friendnumber, real_filenumber, control_type, userdata);
+            }
 
-        if (m->file_filecontrol) {
-            (*m->file_filecontrol)(m, friendnumber, real_filenumber, control_type, userdata);
+            return 0;
         }
 
-        ft->status = FILESTATUS_NONE;
+        case FILECONTROL_KILL: {
+            if (m->file_filecontrol) {
+                m->file_filecontrol(m, friendnumber, real_filenumber, control_type, userdata);
+            }
 
-        if (receive_send) {
-            --m->friendlist[friendnumber].num_sending_files;
-        }
-    } else if (control_type == FILECONTROL_SEEK) {
-        uint64_t position;
+            ft->status = FILESTATUS_NONE;
 
-        if (length != sizeof(position)) {
-            return -1;
-        }
+            if (receive_send) {
+                --m->friendlist[friendnumber].num_sending_files;
+            }
 
-        /* seek can only be sent by the receiver to seek before resuming broken transfers. */
-        if (ft->status != FILESTATUS_NOT_ACCEPTED || !receive_send) {
-            return -1;
+            return 0;
         }
 
-        memcpy(&position, data, sizeof(position));
-        net_to_host((uint8_t *) &position, sizeof(position));
+        case FILECONTROL_SEEK: {
+            uint64_t position;
 
-        if (position >= ft->size) {
-            return -1;
+            if (length != sizeof(position)) {
+                LOGGER_DEBUG(m->log, "file control (friend %d, file %d): expected payload of length %d, but got %d",
+                             friendnumber, filenumber, (uint32_t)sizeof(position), length);
+                return -1;
+            }
+
+            /* seek can only be sent by the receiver to seek before resuming broken transfers. */
+            if (ft->status != FILESTATUS_NOT_ACCEPTED || !receive_send) {
+                LOGGER_DEBUG(m->log,
+                             "file control (friend %d, file %d): seek was either sent by a sender or by the receiver after accepting",
+                             friendnumber, filenumber);
+                return -1;
+            }
+
+            memcpy(&position, data, sizeof(position));
+            net_to_host((uint8_t *) &position, sizeof(position));
+
+            if (position >= ft->size) {
+                LOGGER_DEBUG(m->log,
+                             "file control (friend %d, file %d): seek position %lld exceeds file size %lld",
+                             friendnumber, filenumber, (unsigned long long)position, (unsigned long long)ft->size);
+                return -1;
+            }
+
+            ft->transferred = ft->requested = position;
+            return 0;
         }
 
-        ft->transferred = ft->requested = position;
-    } else {
-        return -1;
+        default: {
+            LOGGER_DEBUG(m->log, "file control (friend %d, file %d): invalid file control: %d",
+                         friendnumber, filenumber, control_type);
+            return -1;
+        }
     }
-
-    return 0;
 }
 
 /**************************************/
@@ -2271,6 +2310,8 @@ static int handle_packet(void *object, int i, const uint8_t *temp, uint16_t len,
             }
 
             if (handle_filecontrol(m, i, send_receive, filenumber, control_type, data + 3, data_length - 3, userdata) == -1) {
+                // TODO(iphydf): Do something different here? Right now, this
+                // check is pointless.
                 break;
             }