Menu

#232 nsopenssl: Memory leak in v3_0beta25

modules
closed-fixed
8
2004-12-02
2004-11-19
No

A memory leak has been introduced sometime AFTER
v3_0beta21 and was detected in v3_0beta25. The cvs
diff is approx. 650 lines long.

Cause of the leak is not yet known.

Discussion

  • Dossy Shiobara

    Dossy Shiobara - 2004-11-20

    Logged In: YES
    user_id=21885

    I suspect the leak was introduced in sslcontext.c rev 1.9 in
    IssueTmpRSAKey. The change fixed a server crash bug (which
    explains why the "leak" was never observed before).

    RSA_generate_key allocates a new RSA structure and returns a
    pointer to it. IssueTmpRSAKey returns that pointer but
    nothing ever frees it.

    The reality is that RSA_generate_key is "slow" and only
    needs to be executed once per key-length (512 and 1024 seem
    to be the only ones used as far as I could find). So, I
    suggest that the two temporary RSA key key-lengths be
    generated from nsopenssl's Ns_ModuleInit. If IssueTmpRSAKey
    gets executed with any other key-lengths, we'll throw an
    error -- we have no way of freeing the newly allocated RSA
    structure, which would lead to a memory leak.

    Other applications use a function scoped static RSA *rsaPtr
    which is a bad idea because suppose the first call is for a
    512-bit key, then the second call is for a 1024-bit key:
    since rsaPtr isn't NULL (it points to a 512-bit RSA key),
    the function erroneously returns a pointer to a 512-bit RSA
    key when a 1024-bit key was requested. Modifying this
    slightly to have a separate rsaPtr_512 and rsaPtr_1024 (but
    still function scoped static) still has problems in a
    multi-threaded app, because typically there's a check to see
    if the ptr is NULL and if it isn't, generate the temp. RSA
    key and store the ptr -- we COULD make this safe by adding a
    mutex lock around the whole mess, but performance-wise, it's
    just smarter to pre-generate the keys ahead of time and
    don't use locks.

    Which is, exactly what I've implemented as the "fix" for
    this memory leak.

     
  • Dossy Shiobara

    Dossy Shiobara - 2004-11-20

    Logged In: YES
    user_id=21885

    Attaching a patch which implements the fix -- the patch is a
    lot bigger than the minimal change because I cleaned up the
    Makefile as well as reformatted some code to make it closer
    to the AOLserver Coding Style.

    The meaningful changes are in sslcontext.c in IssueTmpRSAKey
    and the new NsMakeTmpRSAKey, and a minor change in
    nsopenssl.c in Ns_ModuleInit.

    I've committed the change and have tagged nsopenssl
    v3_0beta26 which contains the fix.

     
  • Dossy Shiobara

    Dossy Shiobara - 2004-11-20

    nsopenssl patch against HEAD (v3_0beta25)

     
  • Dossy Shiobara

    Dossy Shiobara - 2004-11-20
    • status: open --> open-fixed
     
  • Dossy Shiobara

    Dossy Shiobara - 2004-12-02
    • status: open-fixed --> closed-fixed
     

Log in to post a comment.