Skip to content

Python macros don’t shield arguments #87347

Closed
Closed
Python macros don’t shield arguments#87347
Labels
3.10 (EOL)end of lifebuildThe build process and cross-buildtopic-C-API

Description

@numberZero
numberZero
mannequin
opened on Feb 9, 2021
BPO 43181
Nosy @gpshead, @vstinner, @WildCard65, @erlend-aasland, @numberZero
PRs
  • bpo-43181: Make PyObject_TypeCheck a static inline function #24533
    		•

    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 2021-02-15.16:21:14.723>
created_at = <Date 2021-02-09.22:05:50.575>
labels = ['expert-C-API', 'build', '3.10']
title = 'Python macros don\xe2\x80\x99t shield arguments'
updated_at = <Date 2021-03-22.16:47:15.483>
user = 'https://github.com/numberZero'

    bugs.python.org fields:

    activity = <Date 2021-03-22.16:47:15.483>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2021-02-15.16:21:14.723>
closer = 'vstinner'
components = ['C API']
creation = <Date 2021-02-09.22:05:50.575>
creator = 'numberZero'
dependencies = []
files = []
hgrepos = []
issue_num = 43181
keywords = ['patch', '3.9regression']
message_count = 17.0
messages = ['386746', '386754', '386760', '386822', '386827', '386998', '387001', '387007', '387023', '387024', '387360', '388509', '388714', '388727', '388730', '388733', '389331']
nosy_count = 5.0
nosy_names = ['gregory.p.smith', 'vstinner', 'WildCard65', 'erlendaasland', 'numberZero']
pr_nums = ['24533']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'compile error'
url = 'https://bugs.python.org/issue43181'
versions = ['Python 3.10']

    Activity

    1. numberZero commented on Feb 9, 2021

      @numberZero
      numberZeromannequin
      on Feb 9, 2021
      MannequinAuthor

      There is a lot of macros like:

          #define PyObject_TypeCheck(ob, tp) \
    (Py_IS_TYPE(ob, tp) || PyType_IsSubtype(Py_TYPE(ob), (tp)))

      These work fine until an argument happen to contain a comma. That’s possible as a result of other macro’s expansion. E.g. if U(x) is defined as x,

          PyObject_TypeCheck(ob, U(f<a,b>(c)))

      expands to

          (Py_IS_TYPE(ob, f<a,b>(c)) || ...)

      but < and > aren’t treated as brackets by the preprocessor so Py_IS_TYPE is now invoked with 3 arguments instead of just 2, breaking module compilation.

      As arguments are expected to be values, surrounding each with parentheses solves the problem. But there are many such macros so that’s not an one-line fix.

      Note: the example is from PyGLM (simplified), it doesn’t compile on 3.9 due to this issue.

    2. numberZero
      added
      3.9 (EOL)end of life
      topic-C-API
      on Feb 9, 2021

    3. WildCard65 commented on Feb 10, 2021

      @WildCard65
      WildCard65mannequin
      on Feb 10, 2021
      Mannequin

      This feels like it's more of an issue with the C++ compiler you're using. (I can tell it's C++ because of template syntax)

    4. gpshead commented on Feb 10, 2021

      @gpshead
      gpshead
      on Feb 10, 2021
      Member

      This bug is valid, that macro and likely others aren't up to best practices.

      C macros are a PITA to get right.

    5. gpshead
      added
      3.10 (EOL)end of life
      buildThe build process and cross-build
      on Feb 10, 2021

    6. vstinner commented on Feb 11, 2021

      @vstinner
      vstinner
      on Feb 11, 2021
      Member

      I'm replacing macros with static inline functions to avoid issues like this one (ex: bpo-35059). Are you interested to convert the PyObject_TypeCheck() macro to a static inline function?

      """
There is a lot of macros like:
    #define PyObject_TypeCheck(ob, tp) \
    (Py_IS_TYPE(ob, tp) || PyType_IsSubtype(Py_TYPE(ob), (tp)))
These work fine until an argument happen to contain a comma.
"""

      This macro also has a common bug of macros: if one argument has a side effect, it is executed twice! For example, if an argument is a function call, the function is called twice.

      See:

    7. erlend-aasland commented on Feb 11, 2021

      @erlend-aasland
      erlend-aasland
      on Feb 11, 2021
      Contributor

      Are you interested to convert the PyObject_TypeCheck() macro to a static inline function?

      I'd like to give it a shot if it's ok for you all.

    8. erlend-aasland commented on Feb 15, 2021

      @erlend-aasland
      erlend-aasland
      on Feb 15, 2021
      Contributor

      Silence implies consent? I'll throw up a PR, then :)

    9. vstinner commented on Feb 15, 2021

      @vstinner
      vstinner
      on Feb 15, 2021
      Member

      I don't think that PR 24533 should be backported to Python 3.8 and Python 3.9. I prefer to avoid any risk of regression, and so only change Python 3.10.

    10. 8 remaining items

    11. vstinner commented on Mar 11, 2021

      @vstinner
      vstinner
      on Mar 11, 2021
      Member

      Yet... the first argument is still unshielded, passed to a macro that expects one single macro argument.

      Are you talking about the current code in the master branch or the 3.9 branch? If you are talking about the _PyObject_CAST(ob) call, would you mind to elaborate how it is an issue? The code in master LGTM.

      That’s not a regression, it wasn’t shielded in 3.8 either, but why not just parenthesise each macro argument that denotes an expression (as opposed to e.g. name)?

      In the master branch, Include/ and its subdirectories contains 158 .h files for a total of 20K lines of C code. It's not easy to check all files. If you spotted bugs, would you mind to give examples, or a way to automatically detect bugs?

      As I wrote previously, I dislike macros. If someone is changed, I would prefer to convert the function into a static inline function which doesn't have macros pitfalls.

    12. erlend-aasland commented on Mar 15, 2021

      @erlend-aasland
      erlend-aasland
      on Mar 15, 2021
      Contributor

      As I wrote previously, I dislike macros. If someone is changed, I would prefer to convert the function into a static inline function which doesn't have macros pitfalls.

      Should we create a meta issue for this? Most macros are trivial, and can safely be converted, given that they're not used as l-values. Converting one header file at the time would make it easy to review, and it would be possible to make good progress in a few months time.

    13. vstinner commented on Mar 15, 2021

      @vstinner
      vstinner
      on Mar 15, 2021
      Member

      If possible, I would prefer to only replace macros with static inline functions if the changes avoids clear macro pitfalls. It's not because macros have pitfalls that we must replace them all blindly.

      Also, this issue is closed. At this point, I'm not convinced that it's worth it to fix PyObject_TypeCheck() in Python 3.8 and 3.9, so I leave the issue closed.

    14. erlend-aasland commented on Mar 15, 2021

      @erlend-aasland
      erlend-aasland
      on Mar 15, 2021
      Contributor

      If possible, I would prefer to only replace macros with static inline functions if the changes avoids clear macro pitfalls.

      Yes, of course. Should I create a meta issue for this, or do you prefer raising one issue pr. fix?

    15. vstinner commented on Mar 15, 2021

      @vstinner
      vstinner
      on Mar 15, 2021
      Member

      You can create a single issue, and later we will see if it's needed to split it into sub-issues.

    16. vstinner commented on Mar 22, 2021

      @vstinner
      vstinner
      on Mar 22, 2021
      Member

      Me:

      I don't think that PR 24533 should be backported to Python 3.8 and Python 3.9. I prefer to avoid any risk of regression, and so only change Python 3.10.

      Sometimes. I'm wise :-D This change broken 2 extension modules which rely on the fact that the macro magically add parenthesis:

      python-cvxopt: "if Matrix_Check((PyObject *)val)" with: "#define Matrix_Check(self) PyObject_TypeCheck(self, &matrix_tp)"
      https://bugzilla.redhat.com/show_bug.cgi?id=1941557
      PR proposed: cvxopt/cvxopt#190

      python-Bottleneck: "if PyArray_Check(a_obj) {" with: "#define PyArray_Check(op) PyObject_TypeCheck(op, &PyArray_Type)"
      https://bugzilla.redhat.com/show_bug.cgi?id=1941559

      "if func(...) {" is not valid C code, but the old macro added magically parenthesis!

      It's hard to say if the Python change is a backward incompatible or not... I prefer to keep it and fix the few broken C extensions.

    17. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    18. vstinner
      added 2 commits that reference this issue on Jun 16, 2022

      gh-87347: Add parenthesis around PyXXX_Check() arguments (#92815)

      7546914

      gh-87347: Add parenthesis around macro arguments (#93915)

      7ad6f74

    19. vstinner commented on Jun 20, 2022

      @vstinner
      vstinner
      on Jun 20, 2022
      Member

      More fixes:

    20. serhiy-storchaka
      added a commit that references this issue on Jun 26, 2022

      Revert 30935 defer bpo45162 to 312 (#94285)

      1ffde3e
    21. vstinner
      added 2 commits that reference this issue on Jun 26, 2022

      gh-87347: Fix PyObject_NEW() regression (#94234)

      44c8e68

      gh-87347: Fix test_pymem_new() reference leak (#94330)

      edb10ca
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    No one assigned

      Labels

      3.10 (EOL)end of lifebuildThe build process and cross-buildtopic-C-API

      Projects

      No projects

        Milestone

        No milestone

        Relationships

        None yet

        Development

        No branches or pull requests

        Issue actions