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

PyRun_String not exported in python38.dll #81370

Closed
cgohlke mannequin opened this issue Jun 7, 2019 · 18 comments
Closed

PyRun_String not exported in python38.dll #81370

cgohlke mannequin opened this issue Jun 7, 2019 · 18 comments
Labels
3.8 only security fixes 3.9 only security fixes OS-windows

Comments

@cgohlke
Copy link
Mannequin

cgohlke mannequin commented Jun 7, 2019

BPO 37189
Nosy @pfmoore, @vstinner, @tjguk, @benjaminp, @zware, @eryksun, @zooba, @miss-islington
PRs
  • bpo-37189: Export old PyRun_XXX() functions #14142
  • [3.8] bpo-37189: Export old PyRun_XXX() functions (GH-14142) #14177
  • 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 2019-06-18.10:06:56.449>
    created_at = <Date 2019-06-07.07:55:23.953>
    labels = ['3.8', '3.9', 'OS-windows']
    title = 'PyRun_String not exported in python38.dll'
    updated_at = <Date 2019-06-18.10:06:56.440>
    user = 'https://bugs.python.org/cgohlke'

    bugs.python.org fields:

    activity = <Date 2019-06-18.10:06:56.440>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-06-18.10:06:56.449>
    closer = 'vstinner'
    components = ['Windows']
    creation = <Date 2019-06-07.07:55:23.953>
    creator = 'cgohlke'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 37189
    keywords = ['patch']
    message_count = 18.0
    messages = ['344905', '344982', '344989', '344993', '345056', '345792', '345809', '345871', '345873', '345915', '345919', '345926', '345928', '345929', '345930', '345931', '345934', '345975']
    nosy_count = 10.0
    nosy_names = ['paul.moore', 'vstinner', 'tim.golden', 'pyscripter', 'benjamin.peterson', 'cgohlke', 'zach.ware', 'eryksun', 'steve.dower', 'miss-islington']
    pr_nums = ['14142', '14177']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue37189'
    versions = ['Python 3.8', 'Python 3.9']

    @cgohlke
    Copy link
    Mannequin Author

    cgohlke mannequin commented Jun 7, 2019

    While testing third party packages on Python 3.8.0b1 for Windows, I noticed that the PyRun_String function is no longer exported from python38.dll.

    Is this intentional? I can't see this mentioned at <https://docs.python.org/3.8/whatsnew/3.8.html\> or <https://docs.python.org/3.8/c-api/veryhigh.html#c.PyRun_String\>

    This change breaks existing code. But then PyRun_String is easy to replace with PyRun_StringFlags.

    @cgohlke cgohlke mannequin added 3.8 only security fixes OS-windows labels Jun 7, 2019
    @zooba
    Copy link
    Member

    zooba commented Jun 7, 2019

    Guessing Victor may have touched something in this area.

    Was it exported before? Or did we just have a macro for it? Maybe the macro was moved to an internal header by mistake?

    @cgohlke
    Copy link
    Mannequin Author

    cgohlke mannequin commented Jun 7, 2019

    PyRun_String was exported at least since python23.dll.

    Python.Net relies on it at <https://github.com/pythonnet/pythonnet/blob/master/src/runtime/runtime.cs#L858\>

    @zware
    Copy link
    Member

    zware commented Jun 7, 2019

    A look through git log -p looks like bpo-34646 is likely to be related.

    @benjaminp
    Copy link
    Contributor

    It shouldn't break existing code because PyRun_String has a macro expansion to PyRun_StringFlags. ABI compatibility between major releases is not provded.

    @pyscripter
    Copy link
    Mannequin

    pyscripter mannequin commented Jun 17, 2019

    This does break PyScripter Python for Delphi as well. The question whether this change was intentional in which case it would need to be explained and documented, or accidental and will be reversed begs an answer.

    @vstinner
    Copy link
    Member

    Attached PR 14142 fix bpo-34646 regression.

    It shouldn't break existing code because PyRun_String has a macro expansion to PyRun_StringFlags. ABI compatibility between major releases is not provided.

    Many applications don't use Python header files, but access directly libpython. For example, py2app uses dlsym():
    https://bitbucket.org/ronaldoussoren/py2app/src/default/py2app/apptemplate/src/main.c

    PyInstaller uses GetProcAddress() on Windows or dlsym() on other platforms:

    That's why PyRun_String() is defined as an alias using a macro *and* as a function in pythonrun.c:

    #undef PyRun_String
    PyObject *
    PyRun_String(const char *str, int s, PyObject *g, PyObject *l)
    {
        return PyRun_StringFlags(str, s, g, l, NULL);
    }

    @vstinner
    Copy link
    Member

    A look through git log -p looks like bpo-34646 is likely to be related.

    New changeset e502451 by Benjamin Peterson in branch 'master':
    closes bpo-34646: Remove PyAPI_* macros from declarations. (GH-9218)
    e502451

    Extract of this change:

     
     #undef PyRun_String
    -PyAPI_FUNC(PyObject *)
    +PyObject *
     PyRun_String(const char *str, int s, PyObject *g, PyObject *l)
     {
         return PyRun_StringFlags(str, s, g, l, NULL);
     }
     

    On Windows, this change removed dllexport from PyRun_String(). My PR 14142 adds it back, to pythonrun.h.

    @zooba
    Copy link
    Member

    zooba commented Jun 17, 2019

    I haven't fully tested this, but a suitable test using ctypes might look like:

    py = ctypes.PyDLL("", handle=sys.dllhandle)
    missing = {name for name in EXPECTED_NAMES if not hasattr(py, name)}
    # assert 'missing' is empty

    @vstinner
    Copy link
    Member

    I tested the following code:
    ---

    import ctypes, sys 
    names = """
    PyRun_String
    PyRun_AnyFile
    PyRun_AnyFileEx
    PyRun_AnyFileFlags
    PyRun_SimpleString
    PyRun_SimpleFile
    PyRun_SimpleFileEx
    PyRun_InteractiveOne
    PyRun_InteractiveLoop
    PyRun_File
    PyRun_FileEx
    PyRun_FileFlags
    """
    api = ctypes.pythonapi
    api2 = ctypes.PyDLL("", handle=sys.dllhandle)
    for name in names.split():
        if not hasattr(api, name) or not hasattr(api2, name):
            print("MISSING NAME", name)

    Current output:

    Missing names ['PyRun_AnyFile', 'PyRun_AnyFileEx', 'PyRun_File', 'PyRun_FileEx',
    'PyRun_FileFlags', 'PyRun_InteractiveLoop', 'PyRun_InteractiveOne', 'PyRun_Simp
    leFile', 'PyRun_SimpleFileEx', 'PyRun_SimpleString', 'PyRun_String']

    With my PR 14142:

    Missing names []

    @eryksun
    Copy link
    Contributor

    eryksun commented Jun 17, 2019

    api = ctypes.pythonapi
    api2 = ctypes.PyDLL("", handle=sys.dllhandle)

    Those should be the same. In Windows, pythonapi is defined as PyDLL("python dll", None, sys.dllhandle).

    Wouldn't it be better to add a function to _testcapi that checks GetProcAddress(PyWin_DLLhModule, name)?

    @vstinner
    Copy link
    Member

    "Those should be the same."

    Well, I wasn't 100% sure so I tested both. At least, I can now confirm that missing symbols are now exposed in both :-D

    @zooba
    Copy link
    Member

    zooba commented Jun 17, 2019

    Ah, that's what ctypes.pythonapi is :) I looked at PyDLL first and figured it out from there.

    Should we add a regression test to avoid this happening in the future?

    @vstinner
    Copy link
    Member

    New changeset 343ed0f by Victor Stinner in branch 'master':
    bpo-37189: Export old PyRun_XXX() functions (bpo-14142)
    343ed0f

    @vstinner
    Copy link
    Member

    Should we add a regression test to avoid this happening in the future?

    I'm not sure where to add such test, nor which kind of test is needed. I mean, should we only test that the symbol is present? Or should we also test the ABI? Or even write a functional test?

    Since I didn't know, I just merged my fix, to make sure that it lands before 3.8beta2.

    @vstinner
    Copy link
    Member

    My notes on checking an ABI:
    https://pythoncapi.readthedocs.io/stable_abi.html#check-for-abi-changes

    I didn't check if https://abi-laboratory.pro/tracker/timeline/python/ supports Windows or not.

    @miss-islington
    Copy link
    Contributor

    New changeset 8cb8d5d by Miss Islington (bot) in branch '3.8':
    bpo-37189: Export old PyRun_XXX() functions (GH-14142)
    8cb8d5d

    @vstinner
    Copy link
    Member

    I close the issue. The initial issue has been fixed (PyRun_String is now exported again in the Python DLL).

    Eryk Sun:

    Wouldn't it be better to add a function to _testcapi that checks GetProcAddress(PyWin_DLLhModule, name)?

    I have no idea.

    Steve Dower:

    Should we add a regression test to avoid this happening in the future?

    As I explained, I'm not sure where to put such test, I'm not sure what should be tested. If someone wants to work on that, I suggest to open a separated issue.

    @vstinner vstinner added the 3.9 only security fixes label Jun 18, 2019
    @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.8 only security fixes 3.9 only security fixes OS-windows
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants