The comment before fixupMultiSZ and countString states:

** Note that fixupMultiSZ and countString have both had changes
** made to support "incorrect strings".  The registry specification
** calls for strings to be terminated with 2 null bytes.  It seems
** some commercial packages install strings which don't conform,
** causing this code to fail - however, "regedit" etc still work
** with these strings (ie only we don't!).

As indicated in the comments, the two functions dutifully check the supplied length parameter and do not trust the data to be in the correct format.

... except for the inner loop in fixupMultiSZ, which reads:

                for(; *P != '\0'; P++)
                        ;

It should be the same as the inner loop of countStrings:

                for (; P < Q && *P != '\0'; P++)
                        ;

@Tim is this something that you can comment on?

@Steve/Zach FYI.

I've created a PR for this issue.

There's still a potential problem when Reg2Py calls wcslen(str[index]). This could be addressed by having fixupMultiSZ take an int array to store the length of each string. For example:

    static void
    fixupMultiSZ(wchar_t **strings, int *lengths, wchar_t *data, int len)
    {
        wchar_t *P, *Q = data + len;
        int i;

        for (P = data, i = 0; P < Q && *P; P++, i++) {
            strings[i] = P;
            lengths[i] = 0;
            for (; P < Q && *P; P++) {
                lengths[i]++;
            }
        }
    }

We'd have to allocate the lengths array in Reg2Py, like we do for the strings array. Also, we can remove the overflow error check prior to PyUnicode_FromWideChar. The longest possible length is `retDataSize / 2`, which occurs if a single string is stored without any null terminators.

New changeset 56ed86490cb8221c874d432461d77702437f63e5 by Steve Dower (Zackery Spytz) in branch 'master':
bpo-9194: Fix the bounds checking in winreg.c's fixupMultiSZ() (GH-12687)
https://github.com/python/cpython/commit/56ed86490cb8221c874d432461d77702437f63e5

Thanks Zackery! I've merged this main part of the fix (though it requires a manual backport to 2.7). As it's a buffer overrun, I've sent it back to 3.6 as well.

Eryk - thanks for the additional detail. I wonder whether it would be just as easy to guarantee an over-allocation in this case and force a null terminator? (In fact, that would probably have handled the same case that Zackery just fixed, but we didn't have a patch ready for that approach)

New changeset 7038deed09784a03e2a7bad500f0054d29876ae7 by Miss Islington (bot) in branch '3.7':
bpo-9194: Fix the bounds checking in winreg.c's fixupMultiSZ() (GH-12687)
https://github.com/python/cpython/commit/7038deed09784a03e2a7bad500f0054d29876ae7

New changeset 84efbaecaf50b771cc7a95fd9dd9602bd31de305 by Miss Islington (bot) (Zackery Spytz) in branch '2.7':
[2.7] bpo-9194: Fix the bounds checking in winreg.c's fixupMultiSZ() (GH-12687) (GH-12916)
https://github.com/python/cpython/commit/84efbaecaf50b771cc7a95fd9dd9602bd31de305

Declaring this done - Ned can take the backport to 3.6 if/when he feels like it.

New changeset dadc34784444950c389c120fb16f44e5a29cc43f by Ned Deily (Miss Islington (bot)) in branch '3.6':
bpo-9194: Fix the bounds checking in winreg.c's fixupMultiSZ() (GH-12687) (GH-12910)
https://github.com/python/cpython/commit/dadc34784444950c389c120fb16f44e5a29cc43f

stage: patch review -> resolved

messages: + msg339447
versions: + Python 3.7, Python 3.8, - Python 3.2, Python 3.3, Python 3.4

type: behavior
components: + Extension Modules, Windows
stage: test needed