[ploxiln]

That's code copied from the linked gist. But, it omits some key lines in the middle, and does not actually overflow:

    DWORD required_length;
    auto rc = RegQueryValueExW(key, version, NULL, NULL, NULL, &required_length);
    if (rc != 0)  return NULL;

    DWORD length = required_length + 2;  // The +2 is for the maybe optional zero later on. Probably we are over-allocating.
    wchar_t *value = (wchar_t *)malloc(length);
    if (!value) return NULL;

    rc = RegQueryValueExW(key, version, NULL, NULL, (LPBYTE)value, &length);  // We know that version is zero-terminated...
    if (rc != 0)  return NULL;

    // The documentation says that if the string for some reason was not stored
    // with zero-termination, we need to manually terminate it. Sigh!!

    if (value[length]) {
        value[length+1] = 0;
    }

`length` starts at 2 greater than naively needed, then is updated to the real value (again, always 2 less I guess).

[pierrebai]

That only works if someone does not race and modify the content of that registry key between the two function calls. And that, my friend, is how buffer overflow exploits are born.

Don't poke beyond your end. Don't poke using a value that was returned by a function you don't control. The code shown does both. Such quality, it Jonathan Blow.

[ploxiln]

Yeah, this function has a few surprising bits of sloppyness ... but:

> NOTE(Kalinovcic): I have translated the original implementation to C

Another point, this is a build-time tool: some assumptions of good-faith input are reasonable and necessary. If an attacker can modify paths to visual studio components in your registry, you have bigger problems (just running the attacker's code directly regardless of safe string handling).

[malft]

Pray tell, what is the size of a wchar_t on your system?

[ploxiln]

hah, more than 1 byte for sure, I missed that detail ...

EDIT: I develop on unix systems, not windows, so I'm rusty here ... looking up RegQueryValueExW() the `length` ("lpcbData") is in bytes so everything goes fine until `value[length]` which is indexing by wchar_t (16-bit on windows I guess, 32-bit on linux systems) so it's double the byte offset intended, that's way out.