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

Reconsider magic static usage, as there should be no portability issues since XP dropping #673

Closed
AlexGuteniev opened this issue Apr 2, 2020 · 6 comments
Labels
enhancement Something can be improved wontfix This will not be worked on

Comments

@AlexGuteniev
Copy link
Contributor

I'm referring to this #448 (comment)

Adding a .TLS section to a DLL that previously didn't have one can push a program over the TLS slot limit, so it's a breaking change.

My understanding is the limit in Vista was changed from ~58 to ~1000ish but that there is still a limit.

I think that TLS algorithm starting from Vista allows virtually infinite TLS slots (that is limited only by virtual address space).

I think the algorithm abandons old TLS tables without deallocating them. They are deallocated on thread exit. TLS slot table just grows, but with ability of slot reuse. Something like this.

I'm not sure if it is true for Vista, but at least for Windows 10 it seems to work.

I can show, please follow me:


d:\Temp>type myshka.cpp
thread_local int i = 0;

extern "C" __declspec(dllexport) int* GetTlsVar()
{
    i += 1;
    return &i;
}

d:\Temp>type koshka.cpp
#include <Windows.h>
#include <cstdlib>
#include <iostream>
#include <sstream>
#include <unordered_set>

int main()
{
    std::vector<HMODULE> dlls;
    for (int i = 0; i < 30'000; i++)
    {
         std::ostringstream name;
         std::ostringstream cmd;
         name << "myshka_" << i <<".dll";
         if (!::CopyFileA("myshka.dll", name.str().c_str(), FALSE))
         {
             std::cerr << "can't copy\n";
             return 2;
         }
         HMODULE m = ::LoadLibraryA(name.str().c_str());
         if (m == NULL)
         {
             std::cerr << "can't load\n";
             return 3;
         }
         dlls.push_back(m);
    }
    std::cout << "dlls loaded\n";
    for (int expectation : {1, 2})
    {
        std::unordered_set<int*> uniq;
        for (HMODULE m : dlls)
        {
            auto TlsVarFcnPtr = reinterpret_cast<int*(*)()>(::GetProcAddress(m, "GetTlsVar"));
            auto TlsVarPtr = TlsVarFcnPtr();
            if (TlsVarPtr == NULL)
            {
                std::cerr << "can't indirect\n";
                return 4;
            }
            auto TlsVarVal = *TlsVarPtr;
            if (TlsVarVal != expectation || uniq.count(TlsVarPtr) != 0)
            {
               std::cerr << "Bad TLS data " << TlsVarPtr << '\t' << TlsVarVal << '\n';
               return 5;
            }
            uniq.insert(TlsVarPtr);
       }
    }
    std::cout << "all fine\n";
    return 0;
}


d:\Temp>"C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\vcvarsall.bat" x86_amd64

d:\Temp>cl /MD /EHsc myshka.cpp /link /DLL /out:myshka.dll
Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24215.1 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

myshka.cpp
Microsoft (R) Incremental Linker Version 14.00.24215.1
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:myshka.exe
/DLL
/out:myshka.dll
myshka.obj
   Creating library myshka.lib and object myshka.exp

d:\Temp>cl /MD /EHsc koshka.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24215.1 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

koshka.cpp
Microsoft (R) Incremental Linker Version 14.00.24215.1
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:koshka.exe
koshka.obj

d:\Temp>koshka
dlls loaded
all fine

@StephanTLavavej StephanTLavavej added decision needed We need to choose something before working on this enhancement Something can be improved labels Apr 3, 2020
@StephanTLavavej
Copy link
Member

Windows itself had concerns about using magic statics (solved by our avoidance of them). I'm not an expert in this area, but the issue involved DLLs calling DisableThreadLibraryCalls. If the DLL uses TLS (e.g. due to magic statics), this call has no effect, leading to DllMain being called on thread start/end, which increases costs. (Microsoft-internal wiki page.)

I believe that we should continue avoiding magic statics, but I've marked this as decision needed since someone more knowledgeable than I am could provide better guidance.

@AlexGuteniev
Copy link
Contributor Author

@StephanTLavavej , thanks, this sound like strong enough reason. I wouldn't object if you close this issue right away.

@StephanTLavavej
Copy link
Member

Thanks for filing the issue! There are definitely things we avoided in the past that are safe to use now, so it's always good to take another look.

@StephanTLavavej StephanTLavavej added wontfix This will not be worked on and removed decision needed We need to choose something before working on this labels Apr 3, 2020
AlexGuteniev added a commit to AlexGuteniev/sdk-api that referenced this issue Apr 3, 2020
@AlexGuteniev
Copy link
Contributor Author

Yeah, this particular problem possibly could be be fixed in future, especially if concentrate on just magic static, and don't fix it for thread_local generic case.

TLS directory is this (I'm showing 64-bit versions, but 32-bit only differs in types):

typedef struct _IMAGE_TLS_DIRECTORY64 {
    ULONGLONG StartAddressOfRawData;
    ULONGLONG EndAddressOfRawData;
    ULONGLONG AddressOfIndex;         // PDWORD
    ULONGLONG AddressOfCallBacks;     // PIMAGE_TLS_CALLBACK *;
    DWORD SizeOfZeroFill;
    union {
        DWORD Characteristics;
        struct {
            DWORD Reserved0 : 20;
            DWORD Alignment : 4;
            DWORD Reserved1 : 8;
        } DUMMYSTRUCTNAME;
    } DUMMYUNIONNAME;

} IMAGE_TLS_DIRECTORY64

AddressOfCallBacks is a pointer to nullptr-terminated array of function pointers with PIMAGE_TLS_CALLBACK signature.

With only magic statics in module the array is empty, it starts with nullptr-terminator.

But DisableThreadLibraryCalls still does not ignore such DLLs. Not even if AddressOfCallBacks is itself nullptr.

I believe that such behavior of DisableThreadLibraryCalls is intentional, it is to handle the situation when DLL thread callbacks are used in conjunction with static TLS.

But actually magic static does not rely on DLL callbacks, since it works for exe the same as for DLL.

So if a function is added, say, DisableThreadLibraryCallsEx, which could still succeed if AddressOfCallBacks is zero or it points to zero-length array, the problem would be solved.

(Sure above is a speculation, based on the information known to me so far)

@AlexGuteniev
Copy link
Contributor Author

I guess the fixed function DisableThreadLibraryCallsEx could be implemented even outside of Kernel32.dll and inside C runtime, this would give the ability to apply it retroactively to Windows up to Vista. But implementation would require knowledge of Windows internals of each version it targets.

@StephanTLavavej
Copy link
Member

Sounds like a bug/suggestion for the Windows API which could be reported via the Feedback Hub app. (We can't do anything about this in the STL.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants