python / cpython

The Python programming language
https://www.python.org/
Other
59.69k stars 28.93k forks source link

socketmodule SSL free calls in wrong order #34522

Closed 50eff062-408a-4098-b1b2-8222303b9d0c closed 22 years ago

50eff062-408a-4098-b1b2-8222303b9d0c commented 22 years ago
BPO 425370
Nosy @gvanrossum, @warsaw
Files
  • 425370.txt
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields: ```python assignee = 'https://github.com/warsaw' closed_at = created_at = labels = ['extension-modules'] title = 'socketmodule SSL free calls in wrong order' updated_at = user = 'https://bugs.python.org/anonymous' ``` bugs.python.org fields: ```python activity = actor = 'barry' assignee = 'barry' closed = True closed_date = None closer = None components = ['Extension Modules'] creation = creator = 'anonymous' dependencies = [] files = ['65'] hgrepos = [] issue_num = 425370 keywords = [] message_count = 7.0 messages = ['4815', '4816', '4817', '4818', '4819', '4820', '4821'] nosy_count = 3.0 nosy_names = ['gvanrossum', 'jhylton', 'barry'] pr_nums = [] priority = 'normal' resolution = 'fixed' stage = None status = 'closed' superseder = None type = None url = 'https://bugs.python.org/issue425370' versions = [] ```

    3772858d-27d8-44b0-a664-d68674859f36 commented 22 years ago

    I was examining socketmodule.c and notice the following ordering of allocation and deallocation regarding SSL.

    static SSLObject *
    newSSLObject(PySocketSockObject *Sock, char *key_file, char *cert_file)
    {
        ...
        self->ctx = SSL_CTX_new(SSLv23_method()); /* Set up context */
        ...
        self->ssl = SSL_new(self->ctx); /* New ssl struct */
        ...
    }
        static void SSL_dealloc(SSLObject *self)
        {
            ...
            SSL_CTX_free(self->ctx);
            SSL_free(self->ssl);
            ...
        }

    Perhaps it works for now, but I think that the order of free() calls in SSL_dealloc should be reversed.

    03bde425-37ce-4291-88bd-d6cecc46a30e commented 22 years ago

    Logged In: YES user_id=31392

    Barry, This is a memory leak/free problem, right? So you get it. Seriously, sounds like this would be easy to fix.

    warsaw commented 22 years ago

    Logged In: YES user_id=12800

    I believe it is easy to fix (see attached patch), but I don't have a way of testing it. There appears to be no regression test for SSL support, nor is there documentation, so I have no idea how to /use/ the SSL support in socket module.

    Assigning back to Jeremy to double check the patch.

    gvanrossum commented 22 years ago

    Logged In: YES user_id=6380

    Testing SSL support is easy in interactive mode, e.g. urllib.urlopen("https://www.openair.com/index.pl")

    warsaw commented 22 years ago

    Logged In: YES user_id=12800

    Ah, or https://sf.net :)

    Maybe add this as a test case to test_socket but don't bomb out if that host isn't responding or reachable?

    warsaw commented 22 years ago

    Logged In: YES user_id=12800

    Assigning this back to me.

    warsaw commented 22 years ago

    Logged In: YES user_id=12800

    Alright, I'm committing this change to the trunk for 2.2a2. I've got some mods to regrtest which will allow us to test or skip SSL socket test depending on a command line switch (it's a good framework which extends the ideas prototyped in test_largefile). I'll use this to double check memory access with Insure.