Skip to content

Commit

Permalink
Fix potential memory leaks in TcpClient::send (#2753)
Browse files Browse the repository at this point in the history
This PR addresses a concern raised by Coverity. The `TcpClient::send(const char*, ...` method is tricky because it can either use the existing stream or create a new one. Further leaks are possible in `TcpClient::send(IDataSourceStream*, ...` when inspecting the failure paths. The code has been refactored using `std::unique_ptr` guards.

[scan:coverity]
  • Loading branch information
mikee47 authored Apr 8, 2024
1 parent 8843f26 commit 1bcb31d
Showing 1 changed file with 17 additions and 10 deletions.
27 changes: 17 additions & 10 deletions Sming/Components/Network/src/Network/TcpClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,14 @@ bool TcpClient::send(const char* data, uint16_t len, bool forceCloseAfterSent)
return false;
}

std::unique_ptr<MemoryDataStream> newStream;
auto memoryStream = static_cast<MemoryDataStream*>(stream);
if(memoryStream == nullptr || memoryStream->getStreamType() != eSST_MemoryWritable) {
memoryStream = new MemoryDataStream();
newStream = std::make_unique<MemoryDataStream>();
if(!newStream) {
return false;
}
memoryStream = newStream.get();
}

if(!memoryStream->ensureCapacity(memoryStream->getSize() + len)) {
Expand All @@ -56,11 +61,17 @@ bool TcpClient::send(const char* data, uint16_t len, bool forceCloseAfterSent)

memoryStream->write(data, len);

(void)newStream.release();
return send(memoryStream, forceCloseAfterSent);
}

bool TcpClient::send(IDataSourceStream* source, bool forceCloseAfterSent)
{
std::unique_ptr<IDataSourceStream> sourceRef;
if(stream != source) {
sourceRef.reset(source);
}

if(state != eTCS_Connecting && state != eTCS_Connected) {
return false;
}
Expand All @@ -76,36 +87,32 @@ bool TcpClient::send(IDataSourceStream* source, bool forceCloseAfterSent)
auto chainStream = static_cast<StreamChain*>(stream);
if(!chainStream->attachStream(source)) {
debug_w("Unable to attach source to existing stream chain!");
delete source;
return false;
}
} else {
debug_d("Creating stream chain ...");
auto chainStream = new StreamChain();
if(chainStream == nullptr) {
delete source;
auto chainStream = std::make_unique<StreamChain>();
if(!chainStream) {
debug_w("Unable to create stream chain!");
return false;
}

if(!chainStream->attachStream(stream)) {
delete source;
delete chainStream;
debug_w("Unable to attach stream to new chain!");
return false;
}

if(!chainStream->attachStream(source)) {
delete source;
delete chainStream;
debug_w("Unable to attach source to new chain!");
return false;
}

stream = chainStream;
stream = chainStream.release();
}
}

(void)sourceRef.release();

int length = source->available();
if(length > 0) {
totalSentBytes += length;
Expand Down

0 comments on commit 1bcb31d

Please sign in to comment.