Custom CRTLogSystemInterface is impossible to use

[marack]

Describe the bug

Currently it is impossible to implement custom logging for CRT libraries by creating your own logger derived from the Aws::Utils::Logging::CRTLogSystemInterface. This is despite the equivalent functionality being available for the main SDK logging messages via the Aws::Utils::Logging::LogSystemInterface class.

The developer guide chapter on logging (https://docs.aws.amazon.com/sdk-for-cpp/v1/developer-guide/logging.html), strongly suggests a custom logger may be installed by overwriting the crt_logger_create_fn:

options.loggingOptions.crt_logger_create_fn =
    [](){ return Aws::MakeShared<Aws::Utils::Logging::DefaultCRTLogSystem>("CRTLogSystem", Aws::Utils::Logging::LogLevel::Warn); };

This crt_logger_create_fn member is a pointer to a function that provides a shared_ptr<CRTLogSystemInterface> (critically - not a DefaultCRTLogSystem).

However looking at the code for CRTLogSystemInterface reveals it provides no usable functions:

class AWS_CORE_API CRTLogSystemInterface    
{    
public:    
    virtual ~CRTLogSystemInterface() = default;    
};

Instead all of the function that one needs to overwrite are actually part of the DefaultCRTLogSystem class.

Under the hood, the stored pointer to a CRTLogSystemInterface is reinterpret_cast back to a DefaultCRTLogSystem when it is used.

Expected Behavior

It should be possible to provide a custom implementation of the CRTLogSystemInterface just as it is for the main LogSystemInterface.

Current Behavior

It is impossible to derive a class from CRTLogSystemInterface and use it within the logging system. Doing so will result in undefined behaviour. Update: Doing so has no effect since the user would also need to implement the hooks into the CRT libraries themselves, at which point they have gained nothing from the interface class (see below).

Reproduction Steps

#include <aws/core/utils/logging/CRTLogSystem.h>
#include <aws/core/Aws.h>

class my_logger : public Aws::Utils::Logging::CRTLogSystemInterface
{
    // no functions to override :(
};

int main(int argc, char const* argv[])
{
    Aws::SDKOptions options;
    options.loggingOptions.logLevel = Aws::Utils::Logging::LogLevel::Info;
    options.loggingOptions.crt_logger_create_fn = [](){ return std::make_shared<my_logger>(); };
    Aws::InitAPI(options);

    // there are no functions in the logging interface, so CRT logging cannot happen
}

Possible Solution

The functions that are currently defined within DefaultCRTLogSystem should be declared in the CRTLogSystemInterface as pure virtual functions. The reinterpret_cast performed in CRTLogSystem.cpp as part of the bridge between the CRT logging and the SDK logging mechanisms should be changed to target the correct base class.

Update: the top level interface class probably should not be a pure interface and instead provide at least the hookup into the CRT libraries. Then it should provide the pure virtual output function that users can implement when they derive from it.

Additional Information/Context

No response

AWS CPP SDK version used

main, 1.1.11

Compiler and Version used

gcc 12.2.1

Operating System and version

Fedora 37

[marack]

Hmm, looking further I realised that the DefaultCRTLogSystem class is itself responsible for the hook into the CRT logging system via aws_logger.

So there's not really any risk of undefined behaviour because if a user provides their own implementation of CRTLogSystemInterface then it will never be registered with aws_logger and therefore can never be a target of the callback functions that would trigger the invalid reinterpret_cast.

In effect, defining your own implementation of CRTLogSystemInterface does nothing at all.

It would seem more logical to divide the DefaultCRTLogSystem into a separate parent class that handles the hook to the CRT library, and a derived child class that handles the output. I think the expectation for the user would be that they only need to provide the output side - which led to my confusion in thinking I should be able to just implement the top level CRTLogSystemInterface.

I'll leave this open for comment for now - but others may consider this not a problem.

[jmklix]

This is fixed in the latest version of this sdk. Please update and let us know if you have any questions

[github-actions]

This issue is now closed. Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.