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

Incorrect error handling for APIs that can raise exceptions #105375

Closed
18 tasks done
erlend-aasland opened this issue Jun 6, 2023 · 9 comments · Fixed by #105412, #105475, #105494, #105590 or #105592
Closed
18 tasks done

Incorrect error handling for APIs that can raise exceptions #105375

erlend-aasland opened this issue Jun 6, 2023 · 9 comments · Fixed by #105412, #105475, #105494, #105590 or #105592
Assignees
Labels
3.11 only security fixes 3.12 bugs and security fixes 3.13 bugs and security fixes topic-C-API type-bug An unexpected behavior, bug, or error

Comments

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Jun 6, 2023

For example, in the collation callback two str objects (string1 and string2) are created using PyUnicode_FromStringAndSize. Error handling should happen directly after each call to PyUnicode_FromStringAndSize:

string1 = PyUnicode_FromStringAndSize((const char*)text1_data, text1_length);
string2 = PyUnicode_FromStringAndSize((const char*)text2_data, text2_length);
if (!string1 || !string2) {
goto finally; /* failed to allocate strings */
}

Other cases where error handling is not done immediately after the API has been used:

I might have missed some; I did not do a complete audit yet.

Linked PRs

@erlend-aasland erlend-aasland added the type-bug An unexpected behavior, bug, or error label Jun 6, 2023
@erlend-aasland erlend-aasland self-assigned this Jun 6, 2023
@erlend-aasland

This comment was marked as outdated.

erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Jun 6, 2023
Check for error after each call to PyUnicode_FromStringAndSize().
erlend-aasland added a commit that referenced this issue Jun 7, 2023
)

Check for error after each call to PyUnicode_FromStringAndSize().
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 7, 2023
…pythonGH-105412)

Check for error after each call to PyUnicode_FromStringAndSize().
(cherry picked from commit a24a780)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 7, 2023
…pythonGH-105412)

Check for error after each call to PyUnicode_FromStringAndSize().
(cherry picked from commit a24a780)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@erlend-aasland erlend-aasland changed the title Incorrect error handling in sqlite3 collation callback Missing error handling for APIs that can raise exceptions Jun 7, 2023
@erlend-aasland erlend-aasland changed the title Missing error handling for APIs that can raise exceptions Incorrect error handling for APIs that can raise exceptions Jun 7, 2023
erlend-aasland added a commit that referenced this issue Jun 7, 2023
GH-105412) (#105440)

Check for error after each call to PyUnicode_FromStringAndSize().
(cherry picked from commit a24a780)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@erlend-aasland erlend-aasland reopened this Jun 7, 2023
erlend-aasland added a commit that referenced this issue Jun 7, 2023
GH-105412) (#105441)

Check for error after each call to PyUnicode_FromStringAndSize().
(cherry picked from commit a24a780)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Jun 7, 2023
Error handling was deferred in some cases, which could potentially lead
to exceptions being overwritten.
erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Jun 8, 2023
@vstinner
Copy link
Member

vstinner commented Jun 9, 2023

There were many "init" functions of C extensions calling all C functions without checking for exceptions, and only checking for exceptions once at the end. Most of them have been fixed with better error checking, but not all of them yet.

@vstinner
Copy link
Member

vstinner commented Jun 9, 2023

See issue #105374 about C API being error prone: when it's unclear if passing NULL was done on purpose or by mistake

@vstinner
Copy link
Member

vstinner commented Jun 9, 2023

See also capi-workgroup/problems#47

erlend-aasland added a commit that referenced this issue Jun 11, 2023
…H-105611) (#105665)

In _PySys_AddXOptionWithError() and sys_add_xoption(),
bail on first error to prevent exceptions from possibly being
overwritten.
(cherry picked from commit 41cddc2)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
erlend-aasland added a commit that referenced this issue Jun 11, 2023
(cherry picked from commit 20a56d8)

Add proper error handling to add_errors_module() to prevent exceptions
from possibly being overwritten.
erlend-aasland added a commit that referenced this issue Jun 11, 2023
(cherry picked from commit 20a56d8)

Add proper error handling to add_errors_module() to prevent exceptions
from possibly being overwritten.
@serhiy-storchaka
Copy link
Member

Thank you, @erlend-aasland. These are minor errors, but it is nice to have the code cleaner.

Although you introduced new bugs in #105475 and #105611 related to clearing non-initialized references.

@erlend-aasland
Copy link
Contributor Author

Thanks for double-checking, Serhiy. I really appreciate that. I will address your remarks.

@sobolevn
Copy link
Member

Refs #105673

erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Jun 12, 2023
…fields

PyObject_GC_New() only initialises the object header. All other fields
must be explicitly initialised to prevent manipulation of uninitialised
fields in dealloc.

Align initialisation order with the layout of the object structs.
erlend-aasland added a commit that referenced this issue Jun 12, 2023
…#105686)

All fields must be explicitly initialised to prevent manipulation of
uninitialised fields in dealloc.

Align initialisation order with the layout of the object structs.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 12, 2023
…fields (pythonGH-105686)

All fields must be explicitly initialised to prevent manipulation of
uninitialised fields in dealloc.

Align initialisation order with the layout of the object structs.
(cherry picked from commit ca3cc4b)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Jun 12, 2023
…Object fields (python#105686)

All fields must be explicitly initialised to prevent manipulation of
uninitialised fields in dealloc.

Align initialisation order with the layout of the object structs.

(cherry picked from commit ca3cc4b)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
erlend-aasland added a commit that referenced this issue Jun 12, 2023
… fields (GH-105686) (#105710)

All fields must be explicitly initialised to prevent manipulation of
uninitialised fields in dealloc.

Align initialisation order with the layout of the object structs.

(cherry picked from commit ca3cc4b)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
carljm added a commit to carljm/cpython that referenced this issue Jun 12, 2023
* main:
  pythongh-105540: Fix code generator tests (python#105707)
  pythongh-105375: Explicitly initialise all {Pickler,Unpickler}Object fields (python#105686)
  pythongh-105331: Change `asyncio.sleep` to raise ``ValueError` for nan (python#105641)
  Remove support for legacy bytecode instructions (python#105705)
kumaraditya303 pushed a commit that referenced this issue Jun 13, 2023
… fields (#105686) (#105711)

* [3.11] gh-105375: Explicitly initialise all {Pickler,Unpickler}Object fields (#105686)

All fields must be explicitly initialised to prevent manipulation of
uninitialised fields in dealloc.

Align initialisation order with the layout of the object structs.

(cherry picked from commit ca3cc4b)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
erlend-aasland added a commit that referenced this issue Jun 13, 2023
…5667)

Prevent exceptions from possibly being overwritten in case of multiple
failures.
erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Jun 13, 2023
…Stream() (python#105667)

Prevent exceptions from possibly being overwritten in case of multiple
failures.

(cherry picked from commit 217589d)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Jun 13, 2023
…Stream() (python#105667)

Prevent exceptions from possibly being overwritten in case of multiple
failures.

(cherry picked from commit 217589d)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
erlend-aasland added a commit that referenced this issue Jun 13, 2023
…() (#105667) (#105721)

Prevent exceptions from possibly being overwritten in case of multiple
failures.

(cherry picked from commit 217589d)
erlend-aasland added a commit that referenced this issue Jun 13, 2023
…() (#105667) (#105720)

Prevent exceptions from possibly being overwritten in case of multiple
failures.

(cherry picked from commit 217589d)
bbannier added a commit to zeek/pysubnettree that referenced this issue May 8, 2024
On lookup failure we would previously assume that binary addresses are
valid unicode when constructing exception messages. With
python/cpython#105375 which appeared e.g., in python-3.11.9 this starts
to cause failures as invalid unicode is now more consistently rejected; e.g., in
the test `pysubnettree.lookup` we construct a binary address `1.3.3.255`
which corresponds to `b'\x01\x03\x03\xff'` which is not valid unicode.

With this patch we set messages for `KeyError` from a `bytes` object
instead of a `str`, so that user's see e.g.,

    KeyError: b'1:3:3::3'

instead of the previous

    KeyError: '1:3:3::3'

This should still provide all the information necessary while working
with our interface which allows both `str` and `bytes` inputs.

The changes to `SubnetTree_wrap.cc` are generated automatically with
swig-3.0.12.
timwoj pushed a commit to zeek/pysubnettree that referenced this issue May 8, 2024
On lookup failure we would previously assume that binary addresses are
valid unicode when constructing exception messages. With
python/cpython#105375 which appeared e.g., in python-3.11.9 this starts
to cause failures as invalid unicode is now more consistently rejected; e.g., in
the test `pysubnettree.lookup` we construct a binary address `1.3.3.255`
which corresponds to `b'\x01\x03\x03\xff'` which is not valid unicode.

With this patch we set messages for `KeyError` from a `bytes` object
instead of a `str`, so that user's see e.g.,

    KeyError: b'1:3:3::3'

instead of the previous

    KeyError: '1:3:3::3'

This should still provide all the information necessary while working
with our interface which allows both `str` and `bytes` inputs.

The changes to `SubnetTree_wrap.cc` are generated automatically with
swig-3.0.12.
freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this issue Jun 19, 2024
Also fix trace-summary python backtrace.

Intermittent crash:

    zeek/zeek@8c337bd

    threading/MsgThread: Decouple IO source and thread

    A MsgThread acting as an IO source itself can result in the
    scenario where the threading manager's heartbeat timer deletes
    a terminated MsgThread instance, but at the same time this
    instance is in the list of ready IO sources as determined by
    the IO loop in the current iteration.

trace-summary backtrace:

    zeek/pysubnettree#38

    Fix extension for stricter unicode validation

    This fixes our extension module for python/cpython#105375 which
    made unicode validation stricter.

Reported by:	Arne Welzel (crash), ogogon (backtrace)
timwoj pushed a commit to zeek/pysubnettree that referenced this issue Jul 31, 2024
On lookup failure we would previously assume that binary addresses are
valid unicode when constructing exception messages. With
python/cpython#105375 which appeared e.g., in python-3.11.9 this starts
to cause failures as invalid unicode is now more consistently rejected; e.g., in
the test `pysubnettree.lookup` we construct a binary address `1.3.3.255`
which corresponds to `b'\x01\x03\x03\xff'` which is not valid unicode.

With this patch we set messages for `KeyError` from a `bytes` object
instead of a `str`, so that user's see e.g.,

    KeyError: b'1:3:3::3'

instead of the previous

    KeyError: '1:3:3::3'

This should still provide all the information necessary while working
with our interface which allows both `str` and `bytes` inputs.

The changes to `SubnetTree_wrap.cc` are generated automatically with
swig-3.0.12.

(cherry picked from commit d61edb0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment