Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

growable_comment_array_add leaks, causes crash #84201

Closed
ariccio mannequin opened this issue Mar 20, 2020 · 5 comments
Closed

growable_comment_array_add leaks, causes crash #84201

ariccio mannequin opened this issue Mar 20, 2020 · 5 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@ariccio
Copy link
Mannequin

ariccio mannequin commented Mar 20, 2020

BPO 40020
Nosy @benjaminp, @ariccio, @pablogsal
PRs
  • bpo-40020: fix realloc leak on failure in growable_comment_array_add #19083
  • 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:

    assignee = None
    closed_at = <Date 2020-03-31.20:37:15.742>
    created_at = <Date 2020-03-20.00:58:27.029>
    labels = ['interpreter-core', '3.7', '3.8', '3.9', 'type-crash']
    title = 'growable_comment_array_add leaks, causes crash'
    updated_at = <Date 2020-03-31.20:37:15.741>
    user = 'https://github.com/ariccio'

    bugs.python.org fields:

    activity = <Date 2020-03-31.20:37:15.741>
    actor = 'Alexander Riccio'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-03-31.20:37:15.742>
    closer = 'Alexander Riccio'
    components = ['Interpreter Core']
    creation = <Date 2020-03-20.00:58:27.029>
    creator = 'Alexander Riccio'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 40020
    keywords = ['patch']
    message_count = 5.0
    messages = ['364644', '364645', '365351', '365352', '365421']
    nosy_count = 3.0
    nosy_names = ['benjamin.peterson', 'Alexander Riccio', 'pablogsal']
    pr_nums = ['19083']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue40020'
    versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']

    @ariccio
    Copy link
    Mannequin Author

    ariccio mannequin commented Mar 20, 2020

    growable_comment_array_add in parsetok.c incorrectly uses realloc, which leaks the array when allocation fails, and then causes a null pointer deref crash later when the array is freed in growable_comment_array_deallocate (the array pointer is dereferenced, passing null to free is fine).

    It's unlikely that this codepath is reached in normal use, since type comments need to be turned on (via the PyCF_TYPE_COMMENTS compiler flag), but I've managed to replicate the issue by injecting faults with Application Verifier. It's easiest to cause it to fail with a very large number of type comments, but presumably this could also happen with some form of heap fragmentation.

    The buggy code is:

    static int
    growable_comment_array_add(growable_comment_array *arr, int lineno, char *comment) {
        if (arr->num_items >= arr->size) {
            arr->size *= 2;
            arr->items = realloc(arr->items, arr->size * sizeof(*arr->items));
            if (!arr->items) {
                return 0;
            }
        }
    
        arr->items[arr->num_items].lineno = lineno;
        arr->items[arr->num_items].comment = comment;
        arr->num_items++;
        return 1;
    }

    and the correct code would be something like:

    static int
    growable_comment_array_add(growable_comment_array *arr, int lineno, char *comment) {
        if (arr->num_items >= arr->size) {
            arr->size *= 2;
            void* new_items_array = realloc(arr->items, arr->size * sizeof(*arr->items));
            if (!new_items_array) {
                return 0;
            }
            arr->items = new_items_array;
        }
    
        arr->items[arr->num_items].lineno = lineno;
        arr->items[arr->num_items].comment = comment;
        arr->num_items++;
        return 1;
    }

    @ariccio ariccio mannequin added 3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump labels Mar 20, 2020
    @ariccio
    Copy link
    Mannequin Author

    ariccio mannequin commented Mar 20, 2020

    Sidenote: visual studio was misleading and made this look like a use-after-free for a little while, which was interesting.

    @corona10 corona10 added 3.7 (EOL) end of life 3.8 only security fixes labels Mar 20, 2020
    @vstinner
    Copy link
    Member

    New changeset 51e3e45 by Alexander Riccio in branch 'master':
    bpo-40020: Fix realloc leak on failure in growable_comment_array_add (GH-19083)
    51e3e45

    @vstinner
    Copy link
    Member

    Alexander Riccio: Do you know want to propose a change to replace direct usage of malloc/realloc/free with PyMem_Malloc, PyMem_Realloc and PyMem_RawFree? It would add their builtin debug feature for free, and also detect most obvious buffer overflow (reject size larger than PY_SSIZE_T_MAX).

    @ariccio
    Copy link
    Mannequin Author

    ariccio mannequin commented Mar 31, 2020

    Sure, should I open a new issue?

    @ariccio ariccio mannequin closed this as completed Mar 31, 2020
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants