Incorrect error handling for APIs that can raise exceptions #105375

erlend-aasland · 2023-06-06T13:31:33Z

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:

cpython/Modules/_sqlite/connection.c

Lines 1870 to 1875 in 0cb6b9b

    
           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

The text was updated successfully, but these errors were encountered:

Check for error after each call to PyUnicode_FromStringAndSize().

) Check for error after each call to PyUnicode_FromStringAndSize().

…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>

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>

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>

Error handling was deferred in some cases, which could potentially lead to exceptions being overwritten.

vstinner · 2023-06-09T09:26:17Z

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 · 2023-06-09T09:27:35Z

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

vstinner · 2023-06-09T09:33:25Z

See also capi-workgroup/problems#47

(cherry picked from commit 20a56d8) Add proper error handling to add_errors_module() to prevent exceptions from possibly being overwritten. Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

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

…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>

(cherry picked from commit 20a56d8) Add proper error handling to add_errors_module() to prevent exceptions from possibly being overwritten.

serhiy-storchaka · 2023-06-12T08:06:14Z

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 · 2023-06-12T08:07:34Z

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

sobolevn · 2023-06-12T08:09:58Z

Refs #105673

…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.

…#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.

…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>

…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>

… 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>

* 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)

… 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>

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

…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>

…() (#105667) (#105721) Prevent exceptions from possibly being overwritten in case of multiple failures. (cherry picked from commit 217589d)

…() (#105667) (#105720) Prevent exceptions from possibly being overwritten in case of multiple failures. (cherry picked from commit 217589d)

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

erlend-aasland self-assigned this Jun 6, 2023

This comment was marked as outdated.

Sign in to view

AlexWaygood added the topic-sqlite3 label Jun 6, 2023

bedevere-bot mentioned this issue Jun 6, 2023

gh-105375: Improve error handling in sqlite3 collation callback #105412

Merged

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

pythongh-105375: Improve error handling in sqlite3 collation callback

5429c38

Check for error after each call to PyUnicode_FromStringAndSize().

erlend-aasland linked a pull request Jun 6, 2023 that will close this issue

gh-105375: Improve error handling in sqlite3 collation callback #105412

Merged

erlend-aasland closed this as completed in #105412 Jun 7, 2023

erlend-aasland added a commit that referenced this issue Jun 7, 2023

gh-105375: Improve error handling in sqlite3 collation callback (#105412

a24a780

) Check for error after each call to PyUnicode_FromStringAndSize().

This was referenced Jun 7, 2023

[3.12] gh-105375: Improve error handling in sqlite3 collation callback (GH-105412) #105440

Merged

[3.11] gh-105375: Improve error handling in sqlite3 collation callback (GH-105412) #105441

Merged

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 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 reopened this Jun 7, 2023

erlend-aasland added topic-C-API and removed topic-sqlite3 labels Jun 7, 2023

erlend-aasland mentioned this issue Jun 7, 2023

gh-105375: Improve _pickle error handling #105475

Merged

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

pythongh-105375: Improve _pickle error handling

9b514d5

Error handling was deferred in some cases, which could potentially lead to exceptions being overwritten.

erlend-aasland mentioned this issue Jun 8, 2023

gh-105375: Improve error handling in PyUnicode_BuildEncodingMap() #105491

Merged

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

pythongh-105375: Improve error handling in PyUnicode_BuildEncodingMap()

17065c4

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

pythongh-105375: Improve error handling in compiler_enter_scope()

a4d8114

bedevere-bot mentioned this issue Jun 8, 2023

gh-105375: Improve error handling in compiler_enter_scope() #105494

Merged

vstinner mentioned this issue Jun 9, 2023

Argument accepting a Python object or NULL: error prone API capi-workgroup/problems#47

Open

bedevere-bot mentioned this issue Jun 11, 2023

[3.11] gh-105375: Harden pyexpat initialisation (#105606) #105668

Merged

bedevere-bot mentioned this issue Jun 11, 2023

[3.12] gh-105375: Harden pyexpat initialisation (#105606) #105669

Merged

erlend-aasland added a commit that referenced this issue Jun 11, 2023

[3.11] gh-105375: Harden pyexpat initialisation (#105606) (#105668)

e5fe017

(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

[3.12] gh-105375: Harden pyexpat initialisation (#105606) (#105669)

d310fc7

(cherry picked from commit 20a56d8) Add proper error handling to add_errors_module() to prevent exceptions from possibly being overwritten.

bedevere-bot mentioned this issue Jun 12, 2023

gh-105375: Explicitly initialise all {Pickler,Unpickler}Object fields #105686

Merged

bedevere-bot mentioned this issue Jun 12, 2023

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

Merged

bedevere-bot mentioned this issue Jun 12, 2023

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

Merged

erlend-aasland added a commit that referenced this issue Jun 13, 2023

gh-105375: Improve error handling in _Unpickler_SetInputStream() (#10…

217589d

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

This was referenced Jun 13, 2023

[3.12] gh-105375: Improve error handling in _Unpickler_SetInputStream() (#105667) #105720

Merged

[3.11] gh-105375: Improve error handling in _Unpickler_SetInputStream() (#105667) #105721

Merged

erlend-aasland closed this as completed Jun 13, 2023

salty-horse mentioned this issue Aug 27, 2023

Duplicate entry in 3.11.5 changelog - pickle module #108542

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incorrect error handling for APIs that can raise exceptions #105375

Incorrect error handling for APIs that can raise exceptions #105375

erlend-aasland commented Jun 6, 2023 •

edited by bedevere-bot

This comment was marked as outdated.

vstinner commented Jun 9, 2023

vstinner commented Jun 9, 2023

vstinner commented Jun 9, 2023

serhiy-storchaka commented Jun 12, 2023

erlend-aasland commented Jun 12, 2023

sobolevn commented Jun 12, 2023

Incorrect error handling for APIs that can raise exceptions #105375

Incorrect error handling for APIs that can raise exceptions #105375

Comments

erlend-aasland commented Jun 6, 2023 • edited by bedevere-bot

Linked PRs

This comment was marked as outdated.

vstinner commented Jun 9, 2023

vstinner commented Jun 9, 2023

vstinner commented Jun 9, 2023

serhiy-storchaka commented Jun 12, 2023

erlend-aasland commented Jun 12, 2023

sobolevn commented Jun 12, 2023

erlend-aasland commented Jun 6, 2023 •

edited by bedevere-bot