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

Make REG_MULTI_SZ support zero-length strings #76768

Closed
nanonyme mannequin opened this issue Jan 17, 2018 · 16 comments
Closed

Make REG_MULTI_SZ support zero-length strings #76768

nanonyme mannequin opened this issue Jan 17, 2018 · 16 comments
Assignees
Labels
3.7 (EOL) end of life 3.8 only security fixes OS-windows type-bug An unexpected behavior, bug, or error

Comments

@nanonyme
Copy link
Mannequin

nanonyme mannequin commented Jan 17, 2018

BPO 32587
Nosy @pfmoore, @jaraco, @tjguk, @zware, @eryksun, @zooba, @ZackerySpytz, @miss-islington
PRs
  • bpo-32587: Make winreg.REG_MULTI_SZ support zero-length strings #13239
  • [3.7] bpo-32587: Make winreg.REG_MULTI_SZ support zero-length strings (GH-13239) #15745
  • [3.8] bpo-32587: Make winreg.REG_MULTI_SZ support zero-length strings (GH-13239) #15746
  • bpo-32587: Fixes unsafe downcast in PC/winreg.c #15766
  • [3.8] bpo-32587: Fixes unsafe downcast in PC/winreg.c (GH-15766) #15777
  • [3.7] bpo-32587: Fixes unsafe downcast in PC/winreg.c (GH-15766) #15778
  • Files
  • rtlqueryreg.py
  • 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 2019-09-09.13:33:21.848>
    created_at = <Date 2018-01-17.18:09:12.308>
    labels = ['3.8', 'type-bug', '3.7', 'OS-windows']
    title = 'Make REG_MULTI_SZ support zero-length strings'
    updated_at = <Date 2019-09-09.14:25:23.793>
    user = 'https://bugs.python.org/nanonyme'

    bugs.python.org fields:

    activity = <Date 2019-09-09.14:25:23.793>
    actor = 'miss-islington'
    assignee = 'steve.dower'
    closed = True
    closed_date = <Date 2019-09-09.13:33:21.848>
    closer = 'steve.dower'
    components = ['Windows']
    creation = <Date 2018-01-17.18:09:12.308>
    creator = 'nanonyme'
    dependencies = []
    files = ['47394']
    hgrepos = []
    issue_num = 32587
    keywords = ['patch']
    message_count = 16.0
    messages = ['310202', '310216', '310217', '310254', '310266', '342349', '342385', '342431', '351376', '351401', '351402', '351411', '351456', '351459', '351477', '351479']
    nosy_count = 9.0
    nosy_names = ['paul.moore', 'jaraco', 'tim.golden', 'zach.ware', 'eryksun', 'steve.dower', 'ZackerySpytz', 'nanonyme', 'miss-islington']
    pr_nums = ['13239', '15745', '15746', '15766', '15777', '15778']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue32587'
    versions = ['Python 3.7', 'Python 3.8']

    @nanonyme
    Copy link
    Mannequin Author

    nanonyme mannequin commented Jan 17, 2018

    MSDN documents that REG_MULTI_SZ is not supposed to have \0\0 anywhere else than in the end. The comment in

    Private Helper functions for the registry interfaces

    clearly shows that Python has the assumption that this specification is actually correct. However, Microsoft is violating it eg in https://technet.microsoft.com/en-us/library/cc960241.aspx which prevents you from reading that registry key in Python at all. This is a list which is treated as pairs:
    """
    foo

    meep
    bar
    """
    so you can have empty items which in Windows semantics means "remove this file". This results in a string like "foo\0\0meep\0\bar\0\0". I'm proposing relaxing Python registry handling semantics because it's clearly Microsoft isn't following this either

    @eryksun
    Copy link
    Contributor

    eryksun commented Jan 18, 2018

    PyWin32's win32api.RegQueryValueEx and win32api.RegEnumValue also use the same documented interpretation of REG_MULTI_SZ 1 (i.e. a "sequence of null-terminated strings, terminated by an empty string").

    However, in practice Windows doesn't follow this literal interpretation. For example, the Session Manager (smss.exe) reads the "PendingFileRenameOperations" REG_MULTI_SZ value using the documented runtime library function RtlQueryRegistryValues 2. By default, this function calls the specified QueryRoutine for each null-terminated string when reading a REG_MULTI_SZ value.

    I've verified that RtlQueryRegistryValues does not terminate the read on the first empty string, but instead continues to iterate through the entire value. See the attached test script, rtlqueryreg.py. The following output was obtained after two calls to MoveFileEx with the flag MOVEFILE_DELAY_UNTIL_REBOOT that pended operations to delete "foo" and rename "meep" to "bar":

    PendingFileRenameOperations, REG_SZ, \??\C:\Temp\foo
    PendingFileRenameOperations, REG_SZ, <empty>
    PendingFileRenameOperations, REG_SZ, \??\C:\Temp\meep
    PendingFileRenameOperations, REG_SZ, \??\C:\Temp\bar
    

    Currently, winreg terminates reading this value at the first empty string. For example:

        >>> hkey = winreg.OpenKey(winreg.HKEY_LOCAL_MACHINE,
        ...     r'SYSTEM\CurrentControlSet\Control\Session Manager')
        >>> winreg.QueryValueEx(hkey, 'PendingFileRenameOperations')
        (['\\??\\C:\\Temp\\foo'], 7)

    This behavior is inconsistent with Windows, which I think presents a strong case for the suggested change. As always, there's an issue with introducing breaking changes. It may be too late in the development cycle to introduce this change in 3.7.

    @eryksun eryksun added 3.7 (EOL) end of life 3.8 only security fixes type-bug An unexpected behavior, bug, or error labels Jan 18, 2018
    @zooba
    Copy link
    Member

    zooba commented Jan 18, 2018

    I don't think it's too late (and I don't think this counts as a feature either, so the b1 deadline doesn't apply), but the patch would need to include an update to What's New as well as the usual stuff.

    @nanonyme
    Copy link
    Mannequin Author

    nanonyme mannequin commented Jan 18, 2018

    I don't personally know enough of CPython (I barely know enough C) to do the patches but I'd just comment that as this might increase complexity of implementation and wide char API's are used, it might be prudent to make sure the new implementation works with some more exotic input than fits into ASCII. PendingFileRenameOperations can contain any valid Windows path. Microsoft also warns that the end null byte can be redacted in some cases by the party storing the data.

    @eryksun
    Copy link
    Contributor

    eryksun commented Jan 19, 2018

    Currently countStrings and fixupMultiSZ halt the outer loop as soon as they hit an empty string. Basically, we'd be getting rid of that check. Then we have to prevent including an empty string for the final NUL in the normal case. We can do this by decrementing the length by 1 if the data ends on a NUL. We also need a check in the inner loop of fixupMultiSZ to ensure it doesn't get out of bounds.

    Example:

        static void
        fixupMultiSZ(wchar_t **str, wchar_t *data, int len)
        {
            int i;
            wchar_t *P, *Q;
    
            if (len > 0 && data[len - 1] == '\0') {
                Q = data + len - 1;
            } else {
                Q = data + len;
            }
    
            for (P = data, i = 0; P < Q; P++, i++) {
                str[i] = P;
                for(; P < Q && *P != '\0'; P++)
                    ;
            }
        }
    
        static int
        countStrings(wchar_t *data, int len)
        {
            int strings;
            wchar_t *P, *Q;
    
            if (len > 0 && data[len - 1] == '\0') {
                Q = data + len - 1;
            } else {
                Q = data + len;
            }
    
            for (P = data, strings = 0; P < Q; P++, strings++) {
                for (; P < Q && *P != '\0'; P++)
                    ;
            }
            return strings;
        }

    Also, the REG_MULTI_SZ case in Reg2Py should use wcsnlen instead of wcslen, in case of malformed data that doesn't end on at least one NUL character. The upper limit would be the remaining length.

    @zooba
    Copy link
    Member

    zooba commented May 13, 2019

    Updating the title to actually reflect the issue - Zackery, could you update your PR and NEWS entry?

    I just spent way too long looking for a MoveFileEx call that doesn't exist, because this isn't actually about doing things on reboot :)

    For tests, you should be able to create your own REG_MULTI_SZ key with zero-length strings and read it back. If the winreg module won't let you do it, you may need ctypes (but that's okay in Windows-only tests).

    Alternatively, if you want to dive deeper into the C API code, you could expose the fixupMultiSz function as a private function (winreg._split_multi_sz) and test it directly. That's my preferred way of unit testing things like this, but it's a decent amount of boilerplate just for a test.

    @zooba zooba changed the title Make REG_MULTI_SZ support PendingFileRenameOperations Make REG_MULTI_SZ support zero-length strings May 13, 2019
    @eryksun
    Copy link
    Contributor

    eryksun commented May 13, 2019

    For tests, you should be able to create your own REG_MULTI_SZ key with
    zero-length strings and read it back. If the winreg module won't let
    you do it, you may need ctypes (but that's okay in Windows-only
    tests).

    winreg.SetValueEx can create the value. We just need to add a case to test_case in Lib/test/test_winreg.py, such as the following:

    ("Multi-nul", ["", "", "", ""], REG_MULTI_SZ)
    

    I just spent way too long looking for a MoveFileEx call that doesn't
    exist, because this isn't actually about doing things on reboot :)

    Do you mean a variation of MOVEFILE_DELAY_UNTIL_REBOOT that doesn't require administrator access? That would be useful if it existed. I suppose a MOVEFILE_DELAY_UNTIL_LOGOFF flag could be added to have the logon process (winlogon.exe, instead of smss.exe) perform the delete/rename operation under impersonation at the end of the session, or (given a system crash or power failure) the next time the user logs on.

    @ZackerySpytz
    Copy link
    Mannequin

    ZackerySpytz mannequin commented May 14, 2019

    Thank you for the comments. I've updated the PR.

    winreg.SetValueEx can create the value. We just need to add a case to test_case in Lib/test/test_winreg.py, such as the following:

    test_case? I think you mean test_data?

    @zooba
    Copy link
    Member

    zooba commented Sep 9, 2019

    New changeset e223ba1 by Steve Dower (Zackery Spytz) in branch 'master':
    bpo-32587: Make winreg.REG_MULTI_SZ support zero-length strings (bpo-13239)
    e223ba1

    @miss-islington
    Copy link
    Contributor

    New changeset bbf02da by Miss Islington (bot) in branch '3.7':
    bpo-32587: Make winreg.REG_MULTI_SZ support zero-length strings (GH-13239)
    bbf02da

    @miss-islington
    Copy link
    Contributor

    New changeset ebca7eb by Miss Islington (bot) in branch '3.8':
    bpo-32587: Make winreg.REG_MULTI_SZ support zero-length strings (GH-13239)
    ebca7eb

    @zooba
    Copy link
    Member

    zooba commented Sep 9, 2019

    This introduced a new warning:

    c:\projects\cpython\pc\winreg.c(775): warning C4267: '-=': conversion from 'size_t' to 'int', possible loss of data [C:\Projects\cpython\PCbuild\pythoncore.vcxproj]

    I'll fix it (after I'm done with my current task) unless someone else gets there first.

    @jaraco
    Copy link
    Member

    jaraco commented Sep 9, 2019

    New changeset ef66f31 by Jason R. Coombs (Steve Dower) in branch 'master':
    bpo-32587: Fixes unsafe downcast in PC/winreg.c (GH-15766)
    ef66f31

    @zooba
    Copy link
    Member

    zooba commented Sep 9, 2019

    Fixed, assuming the backports merge okay.

    @zooba zooba closed this as completed Sep 9, 2019
    @zooba zooba self-assigned this Sep 9, 2019
    @miss-islington
    Copy link
    Contributor

    New changeset 44729c9 by Miss Islington (bot) in branch '3.8':
    bpo-32587: Fixes unsafe downcast in PC/winreg.c (GH-15766)
    44729c9

    @miss-islington
    Copy link
    Contributor

    New changeset 55a6f73 by Miss Islington (bot) in branch '3.7':
    bpo-32587: Fixes unsafe downcast in PC/winreg.c (GH-15766)
    55a6f73

    @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.7 (EOL) end of life 3.8 only security fixes OS-windows type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants