Memory leak in 4.13.2

[owainhunt]

Describe the issue

After upgrading from JWTKit 4.13.1 to 4.13.2, memory use grows linearly with traffic

Vapor version

4.92.5

Operating system and version

Ubuntu Linux 22.04

Swift version

5.10

Steps to reproduce

Update JWTKit from 4.13.1 to 4.13.2. The only change in this version is a bump of the bundled BoringSSL.

Outcome

Memory usage increases with traffic. Graph attached shows memory use before and after reverting from 4.13.2 to 4.13.1, and the traffic over the same period. You can see that memory increases in line with traffic, so the increase slows overnight while traffic is lower. The sharp drops are pods being killed for OOM.

Additional notes

We are using jemalloc.

We have many similar apps, which use JWTKit in two ways:

• authenticate users against a JWKS from a URL
• create and sign JWTs to authenticate against other services in our estate
  In both cases, we're using RS256 keys.

All our apps have exhibited this behaviour when updating to 4.13.2, and it's resolved by reverting back to 4.13.1.

This behaviour has been observed on both Swift 5.9.2 and 5.10, Ubuntu 22.04 and 20.04, and against multiple versions of Vapor.

Also discussed on this thread on the Vapor Discord: https://discord.com/channels/431917998102675485/1227934412595925033

[0xTim]

@Lukasa are you aware of any issues with BoringSSL around memory usage?

[Lukasa]

This isn't a BoringSSL issue, it is a JWTKit issue. Note that the patch that updated BoringSSL also tweaked the behaviour of the RSAKey.swift type:

e05513b#diff-9ac49656898ca44bb3cec2fde11a7146c77fd06a2d23bbaf849354c3ab5356dc

This change introduced a memory safety issue that probably also introduced a memory leak. Specifically, the type of cRaw was changed from RSA * to EVP_PKEY *. However, the deinit was not changed to free cRaw: instead it still frees c, leaving cRaw leaked. It also frees cRaw with the wrong function, using EVP_PKEY_free to free an RSA *.

Is there a reason this library can't cut over to Crypto now that we support RSA?

[ptoffy]

@Lukasa we're already using SwiftCrypto in version 5 (which is in beta), we're keeping version 4 for compatibility reasons

[ptoffy]

@owainhunt closing this after #158 but feel free to reopen if the issue isn't resolved

[owainhunt]

@ptoffy not resolved, sadly. If anything, 4.13.3 leaks more than 4.13.2. Charts here are the same test (100rps) using 4.13.1, 4.13.2 and 4.13.3.
Note 4.13.3 goes up to >100MB, compared to ~50MB for 4.13.2 and <10MB for 4.13.1.

[Lukasa]

This line is now at fault:

   internal var c: OpaquePointer {
        return CJWTKitBoringSSL_EVP_PKEY_get1_RSA(self.cRaw)
    }

This returns a retained RSA pointer (get1). Previously this returned a reference without doing a +1. That prior behaviour was unsafe, but avoided leaks: the new behaviour gained safety but gained leaks as well.

I recommend making c private, or indeed removing it entirely, and replacing it with a with style function:

func withRSAPointer<ReturnType>(_ body: (OpaquePointer) throws -> ReturnType) rethrows -> ReturnType

This will appropriately keep self alive through the lifetime of the call.

[gwynne]

This is a legacy branch, so we're just gonna change get1 to get0, which doesn't return a +1 pointer.

[gwynne]

@owainhunt As before, please let us know if the issue still isn't resolved!

[owainhunt]

Much better, thanks!

[0xTim]

As an aside - that average memory use for the app is insane 😆

[Lukasa]

That's long been one of Swift's great strengths. Very svelte.