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

Jump to bottom

Double-free in Argument Clinic str_converter generated code #99240

Open
colorfulappl opened this issue Nov 8, 2022 · 5 comments
Open

Double-free in Argument Clinic str_converter generated code #99240

colorfulappl opened this issue Nov 8, 2022 · 5 comments
Assignees
@gpshead
Labels
expert-argument-clinic type-bug An unexpected behavior, bug, or error

Comments

@colorfulappl
Copy link
Contributor

colorfulappl commented Nov 8, 2022 •
edited by bedevere-bot

Argument Clinic str_converter generate such code when encoding is set
(see function test_str_converter_encoding in file Lib/test/clinic.test):

    /* -- snip -- */
    if (!_PyArg_ParseStack(args, nargs, "esesetes#et#:test_str_converter_encoding",
        "idna", &a, "idna", &b, "idna", &c, "idna", &d, &d_length, "idna", &e, &e_length)) {
        goto exit;
    }
    return_value = test_str_converter_encoding_impl(module, a, b, c, d, d_length, e, e_length);

exit:
    /* Cleanup for a */
    if (a) {
       PyMem_FREE(a);
    }
    /* Cleanup for b */
    if (b) {
       PyMem_FREE(b);
    }
    /* Cleanup for c */
    if (c) {
       PyMem_FREE(c);
    }
    /* -- snip -- */

If parsing a successes, a will be assigned an address points to an allocated memory.
After that, if parsing b fails, the memory which a points to is freed by function _PyArg_ParseStack,
and _PyArg_ParseStack returns 0, then control flow goes to label "exit".
At this time, a is not NULL, so the memory it points to is freed again, which cause a double-free problem and a runtime crash.

This bug is found in #96178 "Argument Clinic functional test".
@colorfulappl colorfulappl added the type-bug An unexpected behavior, bug, or error label Nov 8, 2022
@colorfulappl colorfulappl changed the title Double-fre in Argument clinic str_converter generated code Double-free in Argument clinic str_converter generated code Nov 8, 2022
@colorfulappl colorfulappl changed the title Double-free in Argument clinic str_converter generated code Double-free in Argument Clinic str_converter generated code Nov 8, 2022
@colorfulappl colorfulappl mentioned this issue Nov 8, 2022
Merged
colorfulappl added a commit to colorfulappl/cpython that referenced this issue Nov 8, 2022
@colorfulappl
Add PoC of pythonGH-99240
2cc6c0a
@colorfulappl
Copy link
Contributor Author

colorfulappl commented Nov 8, 2022

There are two ways to fix this bug,

  1. Avoid free any parsed arguments if an error occurred in function _PyArg_ParseStack,
    as gh-99240: Fix double-free bug in Argument Clinic str_converter generated code #99241 have done.
  2. If function _PyArg_ParseStack parses failed, assign all the parsed arguments to "NULL" after they are freed, this should be done in _PyArg_ParseStack.

@gpshead gpshead self-assigned this Nov 9, 2022
miss-islington pushed a commit that referenced this issue Nov 24, 2022
@colorfulappl
gh-99240: Fix double-free bug in Argument Clinic str_converter genera…
8dbe08e 
…ted code (GH-99241)

Fix double-free bug mentioned at #99240,
by moving memory clean up out of "exit" label.

Automerge-Triggered-By: GH:erlend-aasland
@hauntsaninja
Copy link
Contributor

hauntsaninja commented Nov 29, 2022

Thanks for reporting and fixing, looks like this has been completed

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Nov 29, 2022

We should consider backporting this, IMO.

@bedevere-bot bedevere-bot mentioned this issue Nov 30, 2022
Merged
@colorfulappl
Copy link
Contributor Author

colorfulappl commented Nov 30, 2022

We should consider backporting this, IMO.

Should we backport #96002 before backporting this?

And I made a new bug fix #99890 .

@hauntsaninja hauntsaninja reopened this Nov 30, 2022
@erlend-aasland
Copy link
Contributor

erlend-aasland commented Nov 30, 2022

Should we backport #96002 before backporting this?

Yes, I think we should. Although that PR introduced a new C file, it expands the Argument Clinic coverage considerably, so I think it is definitely worth it.

And I made a new bug fix #99890 .

Great :)

kumaraditya303 added a commit that referenced this issue Dec 17, 2022
@colorfulappl @kumaraditya303 @erlend-aasland
gh-99240: Reset pointer to NULL when the pointed memory is freed in a…
a6f82f1 
…rgument parsing (#99890)

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
kumaraditya303 added a commit to kumaraditya303/cpython that referenced this issue Dec 17, 2022
@colorfulappl @kumaraditya303 @erlend-aasland
pythongh-99240: Reset pointer to NULL when the pointed memory is free…
efbb1eb 
…d in argument parsing (python#99890)

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
shihai1991 added a commit to shihai1991/cpython that referenced this issue Dec 18, 2022
@shihai1991
Merge remote-tracking branch 'origin/main' into bpo_34697
2878571 
* origin/main: (1306 commits)
  Correct CVE-2020-10735 documentation (python#100306)
  pythongh-100272: Fix JSON serialization of OrderedDict (pythonGH-100273)
  pythongh-93649: Split tracemalloc tests from _testcapimodule.c (python#99551)
  Docs: Use `PY_VERSION_HEX` for version comparison (python#100179)
  pythongh-97909: Fix markup for `PyMethodDef` members (python#100089)
  pythongh-99240: Reset pointer to NULL when the pointed memory is freed in argument parsing (python#99890)
  pythongh-99240: Reset pointer to NULL when the pointed memory is freed in argument parsing (python#99890)
  pythonGH-98831: Add DECREF_INPUTS(), expanding to DECREF() each stack input (python#100205)
  pythongh-78707: deprecate passing >1 argument to `PurePath.[is_]relative_to()` (pythonGH-94469)
  pythongh-99540: Constant hash for _PyNone_Type to aid reproducibility (pythonGH-99541)
  pythongh-100039: enhance __signature__ to work with str and callables (pythonGH-100168)
  pythongh-99830: asyncio: Document returns of remove_{reader,writer} (python#100302)
  "Compound statement" docs: Fix with-statement step indexing (python#100286)
  pythonGH-90043: Handle NaNs in COMPARE_OP_FLOAT_JUMP (pythonGH-100278)
  Improve stats presentation for calls. (pythonGH-100274)
  Better stats for `LOAD_ATTR` and `STORE_ATTR` (pythonGH-100295)
  pythongh-81057: Move the Cached Parser Dummy Name to _PyRuntimeState (python#100277)
  Document that zipfile's pwd parameter is a `bytes` object (python#100209)
  pythongh-99767: mark `PyTypeObject.tp_watched` as internal use only in table (python#100271)
  Fix typo in introduction.rst (python#100266)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gpshead gpshead

Labels
expert-argument-clinic type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@gpshead @hauntsaninja @erlend-aasland @colorfulappl