awslabs / aws-crt-cpp

C++ wrapper around the aws-c-* libraries. Provides Cross-Platform Transport Protocols and SSL/TLS implementations for C++.
Apache License 2.0
73 stars 64 forks source link

Fix memory leak when UnsubscribePacket is reused #617

Closed sfod closed 3 months ago

sfod commented 4 months ago

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

sfod commented 3 months ago

Caching ephemeral data on the packet right before we submit to the native client feels broken.

Unfortunately, the existing API of the UnsubscribePacket doesn't give much options on how to fix this issue:

bool initializeRawOptions(aws_mqtt5_packet_unsubscribe_view &raw_options) noexcept;

I wanted to replace this method with the following:


// Cleans up unsubscribeView on destruction.
class AwsMqtt5PacketUnsubscribeView_WrapperRAII {
    // ctor and dtor with RAII

    aws_mqtt5_packet_unsubscribe_view unsubscribeView;
};

AwsMqtt5PacketUnsubscribeView_WrapperRAII UnsubscribePacket::getRawOptions() noexcept;

, then the calling code would look like this:

bool Mqtt5ClientCore::Unsubscribe()
{
    ... 
    auto unsubscribeViewWrapper = unsubscribePacket->getRawOptions();
    int result = aws_mqtt5_client_unsubscribe(m_client, &unsubscribeViewWrapper.unsubscribeView, &options);
    return result;
}

But it'll be backward-incompatible change for UnsubscribePacket.

bretambrose commented 3 months ago

Why not ditch (ignore initializeFromRawOptions) and use something new instead?

sfod commented 3 months ago

initializeFromRawOptions has to remain valid, and it depends on the "cached" data inside the UnsubscribePacket, so we can't remove these fields, right? Having two independent methods performing the same logic by two different ways feels wrong.