From 19bbed475dd044163f7bbeaf533abaa850380633 Mon Sep 17 00:00:00 2001 From: Maxim Biro Date: Fri, 9 Aug 2013 15:58:08 -0400 Subject: [PATCH 1/5] Extracted repeated code into a function --- core/Messenger.c | 46 +++++++++++++++++----------------------------- 1 file changed, 17 insertions(+), 29 deletions(-) diff --git a/core/Messenger.c b/core/Messenger.c index 00f54cfa..5589e752 100644 --- a/core/Messenger.c +++ b/core/Messenger.c @@ -56,6 +56,7 @@ static uint32_t numfriends; static void set_friend_status(int friendnumber, uint8_t status); +static int write_cryptpacket_id(int crypt_connection_id, uint8_t packet_id, uint8_t *data, uint32_t length); /* 1 if we are online 0 if we are offline @@ -240,11 +241,10 @@ uint32_t m_sendmessage_withid(int friendnumber, uint32_t theid, uint8_t *message /* this does not mean the maximum message length is MAX_DATA_SIZE - 1, it is actually 17 bytes less. */ return 0; uint8_t temp[MAX_DATA_SIZE]; - temp[0] = PACKET_ID_MESSAGE; theid = htonl(theid); - memcpy(temp + 1, &theid, sizeof(theid)); - memcpy(temp + 1 + sizeof(theid), message, length); - return write_cryptpacket(friendlist[friendnumber].crypt_connection_id, temp, length + 1 + sizeof(theid)); + memcpy(temp, &theid, sizeof(theid)); + memcpy(temp + sizeof(theid), message, length); + return write_cryptpacket_id(friendlist[friendnumber].crypt_connection_id, PACKET_ID_MESSAGE, temp, length + sizeof(theid)); } /* send an action to an online friend @@ -256,10 +256,7 @@ int m_sendaction(int friendnumber, uint8_t *action, uint32_t length) return 0; if (length >= MAX_DATA_SIZE || friendlist[friendnumber].status != FRIEND_ONLINE) return 0; - uint8_t temp[MAX_DATA_SIZE]; - temp[0] = PACKET_ID_ACTION; - memcpy(temp + 1, action, length); - return write_cryptpacket(friendlist[friendnumber].crypt_connection_id, temp, length + 1); + return write_cryptpacket_id(friendlist[friendnumber].crypt_connection_id, PACKET_ID_ACTION, action, length); } /* send a name packet to friendnumber @@ -268,10 +265,7 @@ static int m_sendname(int friendnumber, uint8_t * name, uint16_t length) { if(length > MAX_NAME_LENGTH || length == 0) return 0; - uint8_t temp[MAX_NAME_LENGTH + 1]; - memcpy(temp + 1, name, length); - temp[0] = PACKET_ID_NICKNAME; - return write_cryptpacket(friendlist[friendnumber].crypt_connection_id, temp, length + 1); + return write_cryptpacket_id(friendlist[friendnumber].crypt_connection_id, PACKET_ID_NICKNAME, name, length); } /* set the name of a friend @@ -399,22 +393,12 @@ USERSTATUS m_get_self_userstatus(void) static int send_statusmessage(int friendnumber, uint8_t * status, uint16_t length) { - uint8_t *thepacket = malloc(length + 1); - memcpy(thepacket + 1, status, length); - thepacket[0] = PACKET_ID_STATUSMESSAGE; - int written = write_cryptpacket(friendlist[friendnumber].crypt_connection_id, thepacket, length + 1); - free(thepacket); - return written; + return write_cryptpacket_id(friendlist[friendnumber].crypt_connection_id, PACKET_ID_STATUSMESSAGE, status, length); } static int send_userstatus(int friendnumber, USERSTATUS status) { - uint8_t *thepacket = malloc(1 + 1); - memcpy(thepacket + 1, &status, 1); - thepacket[0] = PACKET_ID_USERSTATUS; - int written = write_cryptpacket(friendlist[friendnumber].crypt_connection_id, thepacket, 1 + 1); - free(thepacket); - return written; + return write_cryptpacket_id(friendlist[friendnumber].crypt_connection_id, PACKET_ID_USERSTATUS, (uint8_t*)&status, sizeof(USERSTATUS)); } static int set_friend_statusmessage(int friendnumber, uint8_t * status, uint16_t length) @@ -517,6 +501,14 @@ static void set_friend_status(int friendnumber, uint8_t status) friendlist[friendnumber].status = status; } +static int write_cryptpacket_id(int crypt_connection_id, uint8_t packet_id, uint8_t *data, uint32_t length) +{ + uint8_t packet[length + 1]; + packet[0] = packet_id; + memcpy(packet + 1, data, length); + return write_cryptpacket(crypt_connection_id, packet, length + 1); +} + #define PORT 33445 /* run this at startup */ int initMessenger(void) @@ -619,11 +611,7 @@ static void doFriends(void) } case PACKET_ID_MESSAGE: { if (friendlist[i].receives_read_receipts) { - uint8_t *thepacket = malloc(5); - thepacket[0] = PACKET_ID_RECEIPT; - memcpy(thepacket + 1, temp + 1, 4); - write_cryptpacket(friendlist[i].crypt_connection_id, thepacket, 5); - free(thepacket); + write_cryptpacket_id(friendlist[i].crypt_connection_id, PACKET_ID_RECEIPT, temp + 1, 4); } if (friend_message_isset) (*friend_message)(i, temp + 5, len - 5); From c6d06ae6ee99749b90c1f9498589cd0175ab81f4 Mon Sep 17 00:00:00 2001 From: Maxim Biro Date: Fri, 9 Aug 2013 16:28:12 -0400 Subject: [PATCH 2/5] Moved some checks around --- core/Messenger.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/core/Messenger.c b/core/Messenger.c index 5589e752..1bd0d541 100644 --- a/core/Messenger.c +++ b/core/Messenger.c @@ -56,7 +56,7 @@ static uint32_t numfriends; static void set_friend_status(int friendnumber, uint8_t status); -static int write_cryptpacket_id(int crypt_connection_id, uint8_t packet_id, uint8_t *data, uint32_t length); +static int write_cryptpacket_id(int friendnumber, uint8_t packet_id, uint8_t *data, uint32_t length); /* 1 if we are online 0 if we are offline @@ -235,16 +235,11 @@ uint32_t m_sendmessage(int friendnumber, uint8_t *message, uint32_t length) uint32_t m_sendmessage_withid(int friendnumber, uint32_t theid, uint8_t *message, uint32_t length) { - if (friendnumber < 0 || friendnumber >= numfriends) - return 0; - if (length >= (MAX_DATA_SIZE - sizeof(theid)) || friendlist[friendnumber].status != FRIEND_ONLINE) - /* this does not mean the maximum message length is MAX_DATA_SIZE - 1, it is actually 17 bytes less. */ - return 0; uint8_t temp[MAX_DATA_SIZE]; theid = htonl(theid); memcpy(temp, &theid, sizeof(theid)); memcpy(temp + sizeof(theid), message, length); - return write_cryptpacket_id(friendlist[friendnumber].crypt_connection_id, PACKET_ID_MESSAGE, temp, length + sizeof(theid)); + return write_cryptpacket_id(friendnumber, PACKET_ID_MESSAGE, temp, length + sizeof(theid)); } /* send an action to an online friend @@ -252,11 +247,7 @@ uint32_t m_sendmessage_withid(int friendnumber, uint32_t theid, uint8_t *message return 0 if it was not */ int m_sendaction(int friendnumber, uint8_t *action, uint32_t length) { - if (friendnumber < 0 || friendnumber >= numfriends) - return 0; - if (length >= MAX_DATA_SIZE || friendlist[friendnumber].status != FRIEND_ONLINE) - return 0; - return write_cryptpacket_id(friendlist[friendnumber].crypt_connection_id, PACKET_ID_ACTION, action, length); + return write_cryptpacket_id(friendnumber, PACKET_ID_ACTION, action, length); } /* send a name packet to friendnumber @@ -265,7 +256,7 @@ static int m_sendname(int friendnumber, uint8_t * name, uint16_t length) { if(length > MAX_NAME_LENGTH || length == 0) return 0; - return write_cryptpacket_id(friendlist[friendnumber].crypt_connection_id, PACKET_ID_NICKNAME, name, length); + return write_cryptpacket_id(friendnumber, PACKET_ID_NICKNAME, name, length); } /* set the name of a friend @@ -393,12 +384,12 @@ USERSTATUS m_get_self_userstatus(void) static int send_statusmessage(int friendnumber, uint8_t * status, uint16_t length) { - return write_cryptpacket_id(friendlist[friendnumber].crypt_connection_id, PACKET_ID_STATUSMESSAGE, status, length); + return write_cryptpacket_id(friendnumber, PACKET_ID_STATUSMESSAGE, status, length); } static int send_userstatus(int friendnumber, USERSTATUS status) { - return write_cryptpacket_id(friendlist[friendnumber].crypt_connection_id, PACKET_ID_USERSTATUS, (uint8_t*)&status, sizeof(USERSTATUS)); + return write_cryptpacket_id(friendnumber, PACKET_ID_USERSTATUS, (uint8_t*)&status, sizeof(USERSTATUS)); } static int set_friend_statusmessage(int friendnumber, uint8_t * status, uint16_t length) @@ -501,12 +492,16 @@ static void set_friend_status(int friendnumber, uint8_t status) friendlist[friendnumber].status = status; } -static int write_cryptpacket_id(int crypt_connection_id, uint8_t packet_id, uint8_t *data, uint32_t length) +static int write_cryptpacket_id(int friendnumber, uint8_t packet_id, uint8_t *data, uint32_t length) { + if (friendnumber < 0 || friendnumber >= numfriends) + return 0; + if (length >= MAX_DATA_SIZE || friendlist[friendnumber].status != FRIEND_ONLINE) + return 0; uint8_t packet[length + 1]; packet[0] = packet_id; memcpy(packet + 1, data, length); - return write_cryptpacket(crypt_connection_id, packet, length + 1); + return write_cryptpacket(friendlist[friendnumber].crypt_connection_id, packet, length + 1); } #define PORT 33445 @@ -611,7 +606,7 @@ static void doFriends(void) } case PACKET_ID_MESSAGE: { if (friendlist[i].receives_read_receipts) { - write_cryptpacket_id(friendlist[i].crypt_connection_id, PACKET_ID_RECEIPT, temp + 1, 4); + write_cryptpacket_id(i, PACKET_ID_RECEIPT, temp + 1, 4); } if (friend_message_isset) (*friend_message)(i, temp + 5, len - 5); From 9dd691d106f3a77e4c679df5d71023ab6a4f4316 Mon Sep 17 00:00:00 2001 From: Maxim Biro Date: Fri, 9 Aug 2013 16:34:53 -0400 Subject: [PATCH 3/5] Changed the way statusmessage is allocated --- core/Messenger.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/core/Messenger.c b/core/Messenger.c index 1bd0d541..58e6085f 100644 --- a/core/Messenger.c +++ b/core/Messenger.c @@ -47,8 +47,9 @@ uint8_t self_public_key[crypto_box_PUBLICKEYBYTES]; static uint8_t self_name[MAX_NAME_LENGTH]; static uint16_t self_name_length; -static uint8_t *self_statusmessage; -static uint16_t self_statusmessage_len; +static uint8_t self_statusmessage[MAX_STATUSMESSAGE_LENGTH]; +static uint16_t self_statusmessage_length; + static USERSTATUS self_userstatus; static Friend *friendlist; @@ -315,11 +316,8 @@ int m_set_statusmessage(uint8_t *status, uint16_t length) { if (length > MAX_STATUSMESSAGE_LENGTH) return -1; - uint8_t *newstatus = calloc(length, 1); - memcpy(newstatus, status, length); - free(self_statusmessage); - self_statusmessage = newstatus; - self_statusmessage_len = length; + memcpy(self_statusmessage, status, length); + self_statusmessage_length = length; uint32_t i; for (i = 0; i < numfriends; ++i) @@ -565,7 +563,7 @@ static void doFriends(void) friendlist[i].name_sent = 1; } if (friendlist[i].statusmessage_sent == 0) { - if (send_statusmessage(i, self_statusmessage, self_statusmessage_len)) + if (send_statusmessage(i, self_statusmessage, self_statusmessage_length)) friendlist[i].statusmessage_sent = 1; } if (friendlist[i].userstatus_sent == 0) { From 0f8eea89fab7e3e219819d9c7eee65b454ddab7e Mon Sep 17 00:00:00 2001 From: Maxim Biro Date: Fri, 9 Aug 2013 16:56:11 -0400 Subject: [PATCH 4/5] Introduced data pointer and data_length --- core/Messenger.c | 41 ++++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/core/Messenger.c b/core/Messenger.c index 58e6085f..89fc290b 100644 --- a/core/Messenger.c +++ b/core/Messenger.c @@ -571,55 +571,62 @@ static void doFriends(void) friendlist[i].userstatus_sent = 1; } len = read_cryptpacket(friendlist[i].crypt_connection_id, temp); + uint8_t packet_id = temp[0]; + uint8_t* data = temp + 1; + int data_length = len - 1; if (len > 0) { - switch (temp[0]) { + switch (packet_id) { case PACKET_ID_NICKNAME: { - if (len >= MAX_NAME_LENGTH + 1 || len == 1) + if (data_length >= MAX_NAME_LENGTH || data_length == 0) break; if(friend_namechange_isset) - friend_namechange(i, temp + 1, len - 1); - memcpy(friendlist[i].name, temp + 1, len - 1); - friendlist[i].name[len - 2] = 0; /* make sure the NULL terminator is present. */ + friend_namechange(i, data, data_length); + memcpy(friendlist[i].name, data, data_length); + friendlist[i].name[data_length - 1] = 0; /* make sure the NULL terminator is present. */ break; } case PACKET_ID_STATUSMESSAGE: { - if (len < 2) + if (data_length == 0) break; - uint8_t *status = calloc(MIN(len - 1, MAX_STATUSMESSAGE_LENGTH), 1); - memcpy(status, temp + 1, MIN(len - 1, MAX_STATUSMESSAGE_LENGTH)); + uint8_t *status = calloc(MIN(data_length, MAX_STATUSMESSAGE_LENGTH), 1); + memcpy(status, data, MIN(data_length, MAX_STATUSMESSAGE_LENGTH)); if (friend_statusmessagechange_isset) - friend_statusmessagechange(i, status, MIN(len - 1, MAX_STATUSMESSAGE_LENGTH)); - set_friend_statusmessage(i, status, MIN(len - 1, MAX_STATUSMESSAGE_LENGTH)); + friend_statusmessagechange(i, status, MIN(data_length, MAX_STATUSMESSAGE_LENGTH)); + set_friend_statusmessage(i, status, MIN(data_length, MAX_STATUSMESSAGE_LENGTH)); free(status); break; } case PACKET_ID_USERSTATUS: { - if (len != 2) + if (data_length != 1) break; - USERSTATUS status = temp[1]; + USERSTATUS status = data[0]; if (friend_userstatuschange_isset) friend_userstatuschange(i, status); set_friend_userstatus(i, status); break; } case PACKET_ID_MESSAGE: { + uint8_t *message_id = data; + uint8_t message_id_length = 4; + uint8_t *message = data + message_id_length; + uint16_t message_length = data_length - message_id_length; if (friendlist[i].receives_read_receipts) { - write_cryptpacket_id(i, PACKET_ID_RECEIPT, temp + 1, 4); + write_cryptpacket_id(i, PACKET_ID_RECEIPT, message_id, message_id_length); } if (friend_message_isset) - (*friend_message)(i, temp + 5, len - 5); + (*friend_message)(i, message, message_length); break; } case PACKET_ID_ACTION: { if (friend_action_isset) - (*friend_action)(i, temp + 1, len - 1); + (*friend_action)(i, data, data_length); break; } case PACKET_ID_RECEIPT: { uint32_t msgid; - if (len < 1 + sizeof(msgid)) + if (data_length < sizeof(msgid)) break; - memcpy(&msgid, temp + 1, sizeof(msgid)); + memcpy(&msgid, data, sizeof(msgid)); msgid = ntohl(msgid); if (read_receipt_isset) (*read_receipt)(i, msgid); From f0786d1fcd312eac5f3b104f3763b4863b5af344 Mon Sep 17 00:00:00 2001 From: Maxim Biro Date: Fri, 9 Aug 2013 19:16:51 -0400 Subject: [PATCH 5/5] Returned the length check --- core/Messenger.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/Messenger.c b/core/Messenger.c index 89fc290b..5f33886f 100644 --- a/core/Messenger.c +++ b/core/Messenger.c @@ -236,6 +236,8 @@ uint32_t m_sendmessage(int friendnumber, uint8_t *message, uint32_t length) uint32_t m_sendmessage_withid(int friendnumber, uint32_t theid, uint8_t *message, uint32_t length) { + if (length >= (MAX_DATA_SIZE - sizeof(theid))) + return 0; uint8_t temp[MAX_DATA_SIZE]; theid = htonl(theid); memcpy(temp, &theid, sizeof(theid));