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

Importing uuid should not try to load libc on Windows #68822

Closed
zooba opened this issue Jul 14, 2015 · 10 comments
Closed

Importing uuid should not try to load libc on Windows #68822

zooba opened this issue Jul 14, 2015 · 10 comments
Assignees
Labels
OS-windows type-bug An unexpected behavior, bug, or error

Comments

@zooba
Copy link
Member

zooba commented Jul 14, 2015

BPO 24634
Nosy @pfmoore, @tjguk, @zware, @eryksun, @zooba
Files
  • 24634_1.patch
  • 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 = 'https://github.com/zooba'
    closed_at = <Date 2015-08-08.16:18:18.667>
    created_at = <Date 2015-07-14.20:24:04.862>
    labels = ['type-bug', 'OS-windows']
    title = 'Importing uuid should not try to load libc on Windows'
    updated_at = <Date 2015-08-08.16:18:40.710>
    user = 'https://github.com/zooba'

    bugs.python.org fields:

    activity = <Date 2015-08-08.16:18:40.710>
    actor = 'python-dev'
    assignee = 'steve.dower'
    closed = True
    closed_date = <Date 2015-08-08.16:18:18.667>
    closer = 'steve.dower'
    components = ['Windows']
    creation = <Date 2015-07-14.20:24:04.862>
    creator = 'steve.dower'
    dependencies = []
    files = ['39927']
    hgrepos = []
    issue_num = 24634
    keywords = ['patch']
    message_count = 10.0
    messages = ['246740', '246741', '246742', '246744', '246747', '246748', '246753', '248280', '248281', '248282']
    nosy_count = 6.0
    nosy_names = ['paul.moore', 'tim.golden', 'python-dev', 'zach.ware', 'eryksun', 'steve.dower']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue24634'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5', 'Python 3.6']

    @zooba
    Copy link
    Member Author

    zooba commented Jul 14, 2015

    Lib/uuid.py includes the following code that runs on import:

        import ctypes, ctypes.util
    
        # The uuid_generate_* routines are provided by libuuid on at least
        # Linux and FreeBSD, and provided by libc on Mac OS X.
        for libname in ['uuid', 'c']:
            try:
                lib = ctypes.CDLL(ctypes.util.find_library(libname))
            except Exception:
                continue
            if hasattr(lib, 'uuid_generate_random'):
                _uuid_generate_random = lib.uuid_generate_random
            if hasattr(lib, 'uuid_generate_time'):
                _uuid_generate_time = lib.uuid_generate_time
                if _uuid_generate_random is not None:
                    break  # found everything we were looking for

    This code can cause issues on Windows when loading the CRT outside of an activation context (2.7) or in one case crashed Python 3.4 on a server (unfortunately I couldn't get access to the error logs - but I guessed that this section was responsible and turned out it was).

    I propose adding a sys.platform check before running this code, to omit it on any platform starting with 'win' (patch to follow). I believe this should apply to all current versions of Python.

    It is possible that someone has added their own "uuid.dll" to the DLL search path and is relying on that being found by uuid.py. I consider that unlikely and unsupported, but if people think that's a valid concern I can rework the patch to attempt to load uuid.dll but not the CRT on Windows.

    @zooba zooba self-assigned this Jul 14, 2015
    @zooba zooba added OS-windows type-bug An unexpected behavior, bug, or error labels Jul 14, 2015
    @zooba
    Copy link
    Member Author

    zooba commented Jul 14, 2015

    Patch is against Python 3.5, but uuid.py is identical in all versions and the change should be applied to all four branches.

    @eryksun
    Copy link
    Contributor

    eryksun commented Jul 14, 2015

    This code can cause issues on Windows

    What's the issue or issues here? For 2.7, Windows won't be able to find msvcr90.dll without an activation context, but that's just an ERROR_MOD_NOT_FOUND OS error. It should be ignored by the try/except block. For 3.4, it shouldn't be using SxS for msvcr100.dll, so the server in question must have an unusual configuration. Still, the OSError that gets raised should be ignored.

    3.5 built with VC 14 has a separate issue related to this. Due to changes from bpo-23606, find_msvcrt now returns None. But the TypeError raised by CDLL(None) should be ignored here all the same.

    BTW, in Windows 10 I'm still able to reference CRT functions by name using CDLL('ucrtbase'). Are you sure the ultimate plan is to remove the named exports?

    @zooba
    Copy link
    Member Author

    zooba commented Jul 14, 2015

    For 2.7, Windows won't be able to find msvcr90.dll without an activation context, but that's just an ERROR_MOD_NOT_FOUND OS error.

    Actually, it finds the DLL fine and the DLL terminates the entire process when it fails to detect an activation context. There's bpo-24429 on this, which I forgot to link to (note that this bug is related to that one, but is much smaller in scope).

    For 3.4, it shouldn't be using SxS for msvcr100.dll, so the server in question must have an unusual configuration. Still, the OSError that gets raised should be ignored.

    Unfortunately I can't get hold of the error. It's almost certainly a strange configuration, but it was a commodity, publicly available server where the configuration is completely outside of the user's control. If we can avoid crashing when importing stdlib modules regardless of configuration, we should.

    3.5 built with VC 14 has a separate issue related to this. Due to changes from bpo-23606, find_msvcrt now returns None. But the TypeError raised by CDLL(None) should be ignored here all the same.

    And it will be ignored, but when we know it's going to raise, why bother trying to load the library?

    BTW, in Windows 10 I'm still able to reference CRT functions by name using CDLL('ucrtbase'). Are you sure the ultimate plan is to remove the named exports?

    It looks like on Windows 10 the CRT has switched to the transparent API schema. My python35.dll does not directly reference ucrtbase, and so it's still intended to be non-public API, and I still want to discourage its use because the chance of applications on different Windows versions is too high.

    @eryksun
    Copy link
    Contributor

    eryksun commented Jul 15, 2015

    Actually, it finds the DLL fine and the DLL terminates the entire
    process when it fails to detect an activation context.

    OK, in that case it finds msvcr90.dll via PATH. I was only thinking of the DLL being installed in a subdirectory of the system WinSxS directory. I still think in this case it would be better to let extension modules get access to the activation context that Python saves and subsequently activates when loading extension modules. ctypes could activate this context before calling LoadLibrary.

    @zooba
    Copy link
    Member Author

    zooba commented Jul 15, 2015

    ctypes could activate this context before calling LoadLibrary.

    That would break anyone else who's manually managing their own activation context around ctypes. At best we could expose functions to enable Python's activation context (which I'm willing to help review and merge a patch for on the other issue), but calling them while importing uuid when we know that we should just skip that part of the module's logic is silly.

    @eryksun
    Copy link
    Contributor

    eryksun commented Jul 15, 2015

    That would break anyone else who's manually managing their own
    activation context around ctypes.

    Since this can't be made to work automatically, I prefer your suggestion to continue checking for uuid.dll. You could just do something like the following:

        libnames = ['uuid']
        if sys.platform != 'win32':
            libnames.append('c')
    
        for libname in libnames:
            ...

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 8, 2015

    New changeset 1df7a0821c73 by Steve Dower in branch '3.5':
    Issue bpo-24634: Importing uuid should not try to load libc on Windows
    https://hg.python.org/cpython/rev/1df7a0821c73

    @zooba
    Copy link
    Member Author

    zooba commented Aug 8, 2015

    Done for 2.7, 3.5 and 3.6. Decided against touching 3.4 because it's not really an issue with MSVC 10.0, just 9.0 and 14.0. And I used eryksun's suggested approach.

    @zooba zooba closed this as completed Aug 8, 2015
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 8, 2015

    New changeset 90e2747425ad by Steve Dower in branch '2.7':
    Issue bpo-24634: Importing uuid should not try to load libc on Windows
    https://hg.python.org/cpython/rev/90e2747425ad

    @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
    OS-windows type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants