Clang detects potential memory leak.

[ttosbt]

Howdy, I am new to adding issues to public libraries my projects use, so please have patience with me. :^)

Using clang-tidy (LLVM 10.0.1), during build I receive the error clang-analyzer-cplusplus.NewDeleteLeaks. Looks to be related to the way Pico is handling the string, but I am not 100% on that. This is with an RSA 256 token.

The issue is encountered at jwt.h:2934

I am on an x86_64 Linux machine running kernel 4.18.5. CentOS 7.7.1908.

It should be noted that instead of running the example code directly I split up the creation of the verifier and any follow on validations that need to be added based on configuration.

For example in my header:

jwt::verifier<jwt::default_clock, jwt::picojson_traits> the_verifier{jwt::verify()}; (Which admittedly seems ridiculously complicated to define this way...but it's the best I could do given the examples all use auto and everything is constructed in place and seemingly never reused.)

Then, later in my implementation:

if (verify_issuer) {
    the_verifier.with_issuer(the_issuer_from_configuration_to_use);
}

I don't want to just run the code in the example file because I won't know the issuer that's come in until it's come in and I am dynamically building objects to handle connections which are created at start and put into a pool for use/reuse. Running the creation every time seemed wasteful and less performant than just creating a verifier and configuring it on the fly with configuration as needed.

Thanks in advance for any help you may be able to provide.

[Thalhammer]

I split up the creation of the verifier and any follow on validations that need to be added based on configuration

This should be fine and is a supported use case.

ridiculously complicated to define this way

Thats often the price you pay for flexibility. However since you initialize it directly you should be fine using auto as the variable type even in a header.

Assuming you mean line 2934 in the current master https://github.com/Thalhammer/jwt-cpp/blob/ac0424b115721e4066d2fb99f72ba0cd58759882/include/jwt-cpp/jwt.h#L2934 I can't see how it should leak (assuming picojson is fine, which we do not maintain) and running the test suit in valgrind says everything is deleted, so none of the test cases leak anything. Given this I am tempted to say its a false positive, however just to be sure, can you try actually running the relevant code with something like asan or valgrind to check if it does leak ?

[ttosbt]

Cool, that was a fast turn around! Thanks for that. Appreciate the feedback.

Yeah, I did run it with asan and I didn't get any errors about leaking memory...so I was treating as a false positive. More senior folks on my team requested I reach out and at least ask.

I am leaning on it maybe being an issue with Pico, where they're calling new std::string... in value::value().

Thanks!

[Thalhammer]

I am leaning on it maybe being an issue with Pico, where they're calling new std::string... in value::value().

Might well be, picojson doesn't seem to be all that well maintained lately. I'll keep the issue open as something to look into (maybe adding clang-analyzer) to the ci.

@prince-chrismc You did most of the ci, is this something worth looking into ?

[prince-chrismc]

is this something worth looking into?

Yes! @ttosbt Can you please see if you can share you .clang-tidy settings, this way we can easily reproduce the issue =)

---

You bring up a great point in your comment

Which admittedly seems ridiculously complicated to define this way

I'd suggest somelike...

// might be a namespace missing 🤔 
using pico_verifier = jwt::verifier<default_clock, picojson_traits>; // Break the type out so it's less cumbersome
pico_verifier verify_{ verify() };

[ttosbt]

Hey @prince-chrismc , that's a good tip! Thanks.

Unfortunately, the configuration for clang-tidy is on an isolated network and it's a big'un. I'd be more than happy to answer any questions about it though.

Specifically, the clang error I am getting is:

clang-analyzer-cplusplus.NewDeleteLeaks

[prince-chrismc]

I can confirm it's a false positive from the older style in picojson.

[1/2] Building CXX object example/CMakeFiles/rsa-create.dir/rsa-create.cpp.o
/mnt/c/Users/Chris McArthur/Documents/jwt-cpp/include/picojson/picojson.h:311:1: warning: Potential leak of memory pointed to by field 'object_' [clang-analyzer-cplusplus.NewDeleteLeaks]
}
^
/mnt/c/Users/Chris McArthur/Documents/jwt-cpp/include/jwt-cpp/jwt.h:2629:11: note: Calling 'builder::sign'
                        return sign(
                               ^
/mnt/c/Users/Chris McArthur/Documents/jwt-cpp/include/jwt-cpp/jwt.h:2608:54: note: Calling constructor for 'value'
                        const auto header = encode(json_traits::serialize(typename json_traits::value_type(obj_header)));
                                                                          ^
/mnt/c/Users/Chris McArthur/Documents/jwt-cpp/include/picojson/picojson.h:269:16: note: Memory is allocated
  u_.object_ = new object(o);
               ^
/mnt/c/Users/Chris McArthur/Documents/jwt-cpp/include/jwt-cpp/jwt.h:2608:54: note: Returning from constructor for 'value'
                        const auto header = encode(json_traits::serialize(typename json_traits::value_type(obj_header)));
                                                                          ^
/mnt/c/Users/Chris McArthur/Documents/jwt-cpp/include/jwt-cpp/jwt.h:2608:97: note: Calling '~value'
                        const auto header = encode(json_traits::serialize(typename json_traits::value_type(obj_header)));
                                                                                                                     ^
/mnt/c/Users/Chris McArthur/Documents/jwt-cpp/include/picojson/picojson.h:311:1: note: Potential leak of memory pointed to by field 'object_'
}
^
[2/2] Linking CXX executable example/rsa-create

[ttosbt]

Thanks! Appreciate the FP confirmation.