From e58c9de067f19d1fd9ecfd1ff9ec836cbb0cfd04 Mon Sep 17 00:00:00 2001 From: csoler Date: Sat, 28 Mar 2015 17:23:52 +0000 Subject: [PATCH] fixed a few missing deletes when handling errors in grouter. Experimenting a new scope guard to hold temporary memory. (Patch from GuessWho, modified) git-svn-id: http://svn.code.sf.net/p/retroshare/code/trunk@8093 b45a01b8-16f6-495d-af2f-9b41ad6348cc --- libretroshare/src/grouter/grouteritems.cc | 11 +++ libretroshare/src/grouter/p3grouter.cc | 101 ++++++++++++---------- libretroshare/src/libretroshare.pro | 1 + libretroshare/src/util/rsmemory.h | 39 +++++++++ 4 files changed, 106 insertions(+), 46 deletions(-) create mode 100644 libretroshare/src/util/rsmemory.h diff --git a/libretroshare/src/grouter/grouteritems.cc b/libretroshare/src/grouter/grouteritems.cc index f728c0d68..77b27629e 100644 --- a/libretroshare/src/grouter/grouteritems.cc +++ b/libretroshare/src/grouter/grouteritems.cc @@ -76,11 +76,13 @@ RsGRouterTransactionChunkItem *RsGRouterSerialiser::deserialise_RsGRouterTransac if( NULL == (item->chunk_data = (uint8_t*)malloc(item->chunk_size))) { std::cerr << __PRETTY_FUNCTION__ << ": Cannot allocate memory for chunk " << item->chunk_size << std::endl; + delete item; return NULL ; } if(item->chunk_size + offset > rssize) { std::cerr << __PRETTY_FUNCTION__ << ": Cannot read beyond item size. Serialisation error!" << std::endl; + delete item; return NULL ; } @@ -90,6 +92,7 @@ RsGRouterTransactionChunkItem *RsGRouterSerialiser::deserialise_RsGRouterTransac if (offset != rssize || !ok) { std::cerr << __PRETTY_FUNCTION__ << ": error while deserialising! Item will be dropped." << std::endl; + delete item; return NULL ; } @@ -109,6 +112,7 @@ RsGRouterTransactionAcknItem *RsGRouterSerialiser::deserialise_RsGRouterTransact if (offset != rssize || !ok) { std::cerr << __PRETTY_FUNCTION__ << ": error while deserialising! Item will be dropped." << std::endl; + delete item; return NULL ; } @@ -129,12 +133,14 @@ RsGRouterGenericDataItem *RsGRouterSerialiser::deserialise_RsGRouterGenericDataI if( NULL == (item->data_bytes = (uint8_t*)malloc(item->data_size))) { std::cerr << __PRETTY_FUNCTION__ << ": Cannot allocate memory for chunk " << item->data_size << std::endl; + delete item; return NULL ; } if(item->data_size + offset > rssize) { std::cerr << __PRETTY_FUNCTION__ << ": Cannot read beyond item size. Serialisation error!" << std::endl; + delete item; return NULL ; } @@ -149,6 +155,7 @@ RsGRouterGenericDataItem *RsGRouterSerialiser::deserialise_RsGRouterGenericDataI if (offset != rssize || !ok) { std::cerr << __PRETTY_FUNCTION__ << ": error while deserialising! Item will be dropped." << std::endl; + delete item; return NULL ; } @@ -172,6 +179,7 @@ RsGRouterSignedReceiptItem *RsGRouterSerialiser::deserialise_RsGRouterSignedRece if (offset != rssize || !ok) { std::cerr << __PRETTY_FUNCTION__ << ": error while deserialising! Item will be dropped." << std::endl; + delete item; return NULL ; } @@ -224,6 +232,7 @@ RsGRouterRoutingInfoItem *RsGRouterSerialiser::deserialise_RsGRouterRoutingInfoI if (offset != rssize || !ok) { std::cerr << __PRETTY_FUNCTION__ << ": error while deserialising! Item will be dropped." << std::endl; + delete item; return NULL ; } @@ -248,6 +257,7 @@ RsGRouterMatrixFriendListItem *RsGRouterSerialiser::deserialise_RsGRouterMatrixF if (offset != rssize || !ok) { std::cerr << __PRETTY_FUNCTION__ << ": error while deserialising! Item will be dropped." << std::endl; + delete item; return NULL ; } @@ -282,6 +292,7 @@ RsGRouterMatrixCluesItem *RsGRouterSerialiser::deserialise_RsGRouterMatrixCluesI if (offset != rssize || !ok) { std::cerr << __PRETTY_FUNCTION__ << ": error while deserialising! Item will be dropped." << std::endl; + delete item; return NULL ; } diff --git a/libretroshare/src/grouter/p3grouter.cc b/libretroshare/src/grouter/p3grouter.cc index 1f7f73da1..7933d9ee4 100644 --- a/libretroshare/src/grouter/p3grouter.cc +++ b/libretroshare/src/grouter/p3grouter.cc @@ -184,6 +184,7 @@ #include "util/rsrandom.h" #include "util/rsprint.h" +#include "util/rsmemory.h" #include "serialiser/rsconfigitems.h" #include "services/p3idservice.h" #include "turtle/p3turtle.h" @@ -467,6 +468,7 @@ void p3GRouter::receiveTurtleData(RsTurtleGenericTunnelItem *gitem,const RsFileH if(! ackn_item.serialise(turtle_data_item->data_bytes,turtle_data_item->data_size)) { std::cerr << " ERROR: Cannot serialise ACKN item." << std::endl; + delete turtle_data_item; return ; } @@ -920,7 +922,9 @@ bool p3GRouter::sendDataInTunnel(const TurtleVirtualPeerId& vpid,RsGRouterAbstra #endif uint32_t size = item->serial_size(); - uint8_t *data = (uint8_t*)malloc(size) ; + unsigned char *data = NULL ; + + TemporaryMemoryHolder f(data,size) ; // data will be freed on return, whatever the route taken. if(data == NULL) { @@ -930,7 +934,6 @@ bool p3GRouter::sendDataInTunnel(const TurtleVirtualPeerId& vpid,RsGRouterAbstra if(!item->serialise(data,size)) { - free(data) ; std::cerr << " ERROR: cannot serialise." << std::endl; return false; } @@ -939,58 +942,60 @@ bool p3GRouter::sendDataInTunnel(const TurtleVirtualPeerId& vpid,RsGRouterAbstra static const uint32_t CHUNK_SIZE = 15000 ; while(offset < size) - { - uint32_t chunk_size = std::min(size - offset, CHUNK_SIZE) ; + { + uint32_t chunk_size = std::min(size - offset, CHUNK_SIZE) ; - RsGRouterTransactionChunkItem *chunk_item = new RsGRouterTransactionChunkItem ; - chunk_item->propagation_id = item->routing_id ; - chunk_item->total_size = size; - chunk_item->chunk_start= offset; - chunk_item->chunk_size = chunk_size ; - chunk_item->chunk_data = (uint8_t*)malloc(chunk_size) ; + RsGRouterTransactionChunkItem *chunk_item = new RsGRouterTransactionChunkItem ; + chunk_item->propagation_id = item->routing_id ; + chunk_item->total_size = size; + chunk_item->chunk_start= offset; + chunk_item->chunk_size = chunk_size ; + chunk_item->chunk_data = (uint8_t*)malloc(chunk_size) ; #ifdef GROUTER_DEBUG - std::cerr << " preparing to send a chunk [" << offset << " -> " << offset + chunk_size << " / " << size << "]" << std::endl; + std::cerr << " preparing to send a chunk [" << offset << " -> " << offset + chunk_size << " / " << size << "]" << std::endl; #endif - if(chunk_item->chunk_data == NULL) - { - std::cerr << " ERROR: Cannot allocate memory for size " << chunk_size << std::endl; - return false; - } - memcpy(chunk_item->chunk_data,&data[offset],chunk_size) ; + if(chunk_item->chunk_data == NULL) + { + std::cerr << " ERROR: Cannot allocate memory for size " << chunk_size << std::endl; + delete chunk_item; + return false; + } + memcpy(chunk_item->chunk_data,&data[offset],chunk_size) ; - offset += chunk_size ; + offset += chunk_size ; - RsTurtleGenericDataItem *turtle_item = new RsTurtleGenericDataItem ; + RsTurtleGenericDataItem *turtle_item = new RsTurtleGenericDataItem ; - uint32_t turtle_data_size = chunk_item->serial_size() ; - uint8_t *turtle_data = (uint8_t*)malloc(turtle_data_size) ; + uint32_t turtle_data_size = chunk_item->serial_size() ; + uint8_t *turtle_data = (uint8_t*)malloc(turtle_data_size) ; - if(turtle_data == NULL) - { - std::cerr << " ERROR: Cannot allocate turtle data memory for size " << turtle_data_size << std::endl; - return false; - } - if(!chunk_item->serialise(turtle_data,turtle_data_size)) - { - std::cerr << " ERROR: cannot serialise RsGRouterTransactionChunkItem." << std::endl; - free(turtle_data) ; - return false; - } + if(turtle_data == NULL) + { + std::cerr << " ERROR: Cannot allocate turtle data memory for size " << turtle_data_size << std::endl; + delete chunk_item; + return false; + } + if(!chunk_item->serialise(turtle_data,turtle_data_size)) + { + std::cerr << " ERROR: cannot serialise RsGRouterTransactionChunkItem." << std::endl; + delete chunk_item; + free(turtle_data) ; + return false; + } - delete chunk_item ; + delete chunk_item ; - turtle_item->data_size = turtle_data_size ; - turtle_item->data_bytes = turtle_data ; + turtle_item->data_size = turtle_data_size ; + turtle_item->data_bytes = turtle_data ; #ifdef GROUTER_DEBUG - std::cerr << " sending to vpid " << vpid << std::endl; + std::cerr << " sending to vpid " << vpid << std::endl; #endif - mTurtle->sendTurtleData(vpid,turtle_item) ; - } + mTurtle->sendTurtleData(vpid,turtle_item) ; + } - free(data) ; return true ; } @@ -1136,6 +1141,7 @@ void p3GRouter::handleIncomingDataItem(const TurtleFileHash& hash,RsGRouterGener if(!signDataItem(receipt_item,generic_item->destination_key)) { + delete receipt_item; std::cerr << " signing: FAILED. Receipt dropped. ERROR." << std::endl; return ; } @@ -1156,6 +1162,7 @@ void p3GRouter::handleIncomingDataItem(const TurtleFileHash& hash,RsGRouterGener #ifdef GROUTER_DEBUG std::cerr << " sent signed receipt in tunnel " << generic_item->PeerId() << std::endl; #endif + delete receipt_item; } bool p3GRouter::locked_getClientAndServiceId(const TurtleFileHash& hash, const RsGxsId& destination_key, GRouterClientService *& client, GRouterServiceId& service_id) @@ -1372,14 +1379,17 @@ bool p3GRouter::signDataItem(RsGRouterAbstractMsgItem *item,const RsGxsId& signi } bool p3GRouter::verifySignedDataItem(RsGRouterAbstractMsgItem *item) { - uint8_t *data = NULL; - try { RsTlvSecurityKey signature_key ; uint32_t data_size = item->signed_data_size() ; - uint8_t *data = (uint8_t*)malloc(data_size) ; + uint8_t *data = NULL; + + TemporaryMemoryHolder f(data,data_size) ; + + if(data == NULL) + throw std::runtime_error("Cannot allocate data.") ; if(!item->serialise_signed_data(data,data_size)) throw std::runtime_error("Cannot serialise signed data.") ; @@ -1417,18 +1427,14 @@ bool p3GRouter::verifySignedDataItem(RsGRouterAbstractMsgItem *item) break ; default: break ; } - free(data) ; return false; } - free(data) ; return true ; } catch(std::exception& e) { std::cerr << " signature verification failed. Error: " << e.what() << std::endl; - if(data != NULL) - free(data) ; return false ; } } @@ -1494,6 +1500,7 @@ bool p3GRouter::sendData(const RsGxsId& destination,const GRouterServiceId& clie if(!encryptDataItem(data_item,destination)) { std::cerr << "Cannot encrypt data item. Some error occured!" << std::endl; + delete data_item; return false; } @@ -1502,6 +1509,7 @@ bool p3GRouter::sendData(const RsGxsId& destination,const GRouterServiceId& clie if(!signDataItem(data_item,signing_id)) { std::cerr << "Cannot sign data item. Some error occured!" << std::endl; + delete data_item; return false; } @@ -1510,6 +1518,7 @@ bool p3GRouter::sendData(const RsGxsId& destination,const GRouterServiceId& clie if(!verifySignedDataItem(data_item)) { std::cerr << "Cannot verify data item that was just signed. Some error occured!" << std::endl; + delete data_item; return false; } // push the item into pending messages. diff --git a/libretroshare/src/libretroshare.pro b/libretroshare/src/libretroshare.pro index 885d89e7f..1f520e30b 100644 --- a/libretroshare/src/libretroshare.pro +++ b/libretroshare/src/libretroshare.pro @@ -340,6 +340,7 @@ HEADERS += chat/distantchat.h \ HEADERS += pqi/authssl.h \ pqi/authgpg.h \ + pqi/rsmemory.h \ pgp/pgphandler.h \ pgp/pgpkeyutil.h \ pgp/rsaes.h \ diff --git a/libretroshare/src/util/rsmemory.h b/libretroshare/src/util/rsmemory.h new file mode 100644 index 000000000..8e422dd5b --- /dev/null +++ b/libretroshare/src/util/rsmemory.h @@ -0,0 +1,39 @@ +#pragma once + +#include + +// This is a scope guard to release the memory block when going of of the current scope. +// Can be very useful to auto-delete some memory on quit without the need to call free each time. +// +// Usage: +// +// { +// unsigned char *mem = NULL ; +// TemporaryMemoryHolder mem_holder(mem,size) ; +// +// [do something] +// } // mem gets freed automatically +// +class TemporaryMemoryHolder +{ + public: + TemporaryMemoryHolder(unsigned char *& mem,size_t s) + : _mem(mem) + { + _mem = (unsigned char *)malloc(s) ; + } + + ~TemporaryMemoryHolder() + { + if(_mem != NULL) + { + free(_mem) ; + _mem = NULL ; + } + } + + private: + unsigned char *& _mem ; +}; + +