3
\$\begingroup\$

In c++, I've created a application launcher DLL for .ekccafile file types. This extensions stands for EK CEF Client App. Obviously, .ekccafile are file dependent to CEF dlls. If that file type is double clicked in explorer, my dll gets called via rundll.exe and its exported function is executed. Inside that function, it checks for the folder path set in the target file's configuration. See if the files required by the target file are all present. If all goes well it then calls SetDLLDirectory pointing to that path then launches the target file via CreateProcess. My goal here is to put all CEF dlls in one location so that I don't have multiple copies of half gigabytes of CEF resources in my drive. I've just came to know about SetDLLDirectory very recently and my question is, how does SetDLLDirectory affects other executables?

Here’s the DLL functions

static bool wmCmdProcd;
static WCHAR _path[MAX_PATH];

template<size_t size>
static bool CheckDepends(HWND hwnd, WCHAR(&cefpath)[size]) {
    static LPCWSTR __cefBins[] = {
        L"libcef.dll",
        L"chrome_elf.dll",
        L"icudtl.dat",
        L"v8_context_snapshot.bin"
    };
    std::wstring base = cefpath, missing;
    FILE* f;
    for (auto path: __cefBins) {
        std::wstring w = base + L"\\" + path;
        f = nullptr;
        auto err = _wfopen_s(&f, w.c_str(), L"r");
        if (err || !f) {
            if (missing.size())
                missing += L"\n";
            missing += L"\t";
            missing += path;
        }
        else
            fclose(f);
    }
    if (missing.size()) {
        missing = L"The followig files are missing in " + std::wstring(cefpath) + L"\n" + missing + L".\nDo you want to browse the folder where the CefSharp binaries are located?";
        wmCmdProcd = false;
        return ek::Window::HookedMsgBox([](ek::Window::HookedMsgBox& mbox, UINT uMsg, WPARAM wParam, LPARAM lParam, LRESULT& res)->bool {
            if (uMsg == WM_COMMAND) {
                if (LOWORD(wParam) == IDYES) {
                    if (!wmCmdProcd) {
                        ek::Window::FileDialog fd;
                        fd.Options = ek::Window::FDlgOpts::PickFolders | fd.Options;
                        if (fd.Show(mbox)) {
                            wmCmdProcd = true;
                            wcscpy_s((LPWSTR)mbox.Param(), MAX_PATH, fd.Result);
                            char sz[MAX_PATH];
                            WideCharToMultiByte(CP_ACP, 0, fd.Result, -1, sz, MAX_PATH, nullptr, nullptr);
                            std::wstring wsz = _path;
                            wsz += L"\\cefshared.bin";
                            ek::io::FileStream fs(wsz, L"w");
                            fs.Write(sz, strlen(sz));
                            SendMessage(mbox, uMsg, wParam, lParam);
                        }
                        return true;
                    }
                }
            }
            return false;
            },(LPARAM)cefpath).Show(hwnd, missing.c_str(), L"Missing Important CEF Files", MB_ICONINFORMATION | MB_YESNO) == IDYES;
    }
    return true;
}

void CALLBACK LaunchFile(HWND hwnd, HINSTANCE hinst, LPSTR lpCmdLine, int nCmdShow) {
    if (!szCefPath[0]) {
        GetModuleFileName(reinterpret_cast<HINSTANCE>(&__ImageBase), _path, MAX_PATH);
        WCHAR* ptr = wcsrchr(_path, '\\');
        *ptr = 0;
        wcscpy_s(szCefPath, _path);
        wcscat_s(szCefPath, L"\\cefshared.bin");
        ek::io::FileStream fs(szCefPath, L"r");
        if (!fs.is_open) {
            *(wcsrchr(szCefPath, '\\')) = 0;
        }
        else {
            char sz[MAX_PATH];
            auto size = fs.Read(sz, MAX_PATH);
            sz[size] = 0;
            MultiByteToWideChar(CP_ACP, 0, sz, -1, szCefPath, MAX_PATH);
        }
    }

    if (!CheckDepends(hwnd, szCefPath))
        return;

    SetDllDirectory(szCefPath);

    WCHAR wsz[MAX_PATH] = {};
    STARTUPINFO si = { sizeof(si) };
    PROCESS_INFORMATION pi;

    MultiByteToWideChar(CP_ACP, 0, lpCmdLine, -1, wsz, MAX_PATH);
    if (wsz[0] == '"') {
        WCHAR* p1 = wsz, * p2 = wsz + 1;
        while (*p2) {
            if (*p2 == '"')break;
            *p1++ = *p2++;
        }
        *p1 = 0;
    }

    if (!CheckHeader(wsz)) {
        MessageBox(hwnd, (std::wstring(wsz) + L" is not a valid executable.").c_str(), L"Invalid File", MB_ICONINFORMATION);
        return;
    }

    BOOL a = CreateProcess(
        wsz,     // Application path
        nullptr,               // Command line args
        nullptr, nullptr,      // Security attributes
        FALSE,                 // Inherit handles
        0,                     // Creation flags
        nullptr,               // Environment
        szCefPath,               // Current directory
        &si, &pi               // Startup info and process info
    );

    if (a) {
        CloseHandle(pi.hProcess);
        CloseHandle(pi.hThread);
    }
}

Please ignore the following since they are just calls to built-in windows native DLL functions:

  • ek::Window::HookedMsgBox
  • ek::Window::FileDialog fd;
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Style

Looking at the following snippet, there are some things that rub me the wrong way.

  • Long lines
  • Inconsistent whitespace/newlines
  • Nested conditionals that can be combined.
    if (missing.size()) {
        missing = L"The followig files are missing in " + std::wstring(cefpath) + L"\n" + missing + L".\nDo you want to browse the folder where the CefSharp binaries are located?";
        wmCmdProcd = false;
        return ek::Window::HookedMsgBox([](ek::Window::HookedMsgBox& mbox, UINT uMsg, WPARAM wParam, LPARAM lParam, LRESULT& res)->bool {
            if (uMsg == WM_COMMAND) {
                if (LOWORD(wParam) == IDYES) {
                    if (!wmCmdProcd) {
                        ek::Window::FileDialog fd;
                        fd.Options = ek::Window::FDlgOpts::PickFolders | fd.Options;
                        if (fd.Show(mbox)) {
                            wmCmdProcd = true;
                            wcscpy_s((LPWSTR)mbox.Param(), MAX_PATH, fd.Result);
                            char sz[MAX_PATH];
                            WideCharToMultiByte(CP_ACP, 0, fd.Result, -1, sz, MAX_PATH, nullptr, nullptr);
                            std::wstring wsz = _path;
                            wsz += L"\\cefshared.bin";
                            ek::io::FileStream fs(wsz, L"w");
                            fs.Write(sz, strlen(sz));
                            SendMessage(mbox, uMsg, wParam, lParam);
                        }
                        return true;
                    }
                }
            }
            return false;
            },(LPARAM)cefpath).Show(hwnd, missing.c_str(), L"Missing Important CEF Files", MB_ICONINFORMATION | MB_YESNO) == IDYES;
    }

Addressing some of these issues:

    if (missing.size()) {
        missing = 
            L"The followig files are missing in " + 
            std::wstring(cefpath) + L"\n" + missing + 
            L".\nDo you want to browse the folder where the CefSharp binaries are located?";
        wmCmdProcd = false;
        
        return ek::Window::HookedMsgBox(
            [](
                ek::Window::HookedMsgBox& mbox, 
                UINT uMsg, 
                WPARAM wParam, 
                LPARAM lParam, 
                LRESULT& res
            ) -> bool {
                if (uMsg == WM_COMMAND && LOWORD(wParam) == IDYES && !wmCmdProcd) {
                    ek::Window::FileDialog fd;
                    fd.Options = ek::Window::FDlgOpts::PickFolders | fd.Options;
                    
                    if (fd.Show(mbox)) {
                        wmCmdProcd = true;
                        wcscpy_s((LPWSTR)mbox.Param(), MAX_PATH, fd.Result);
                        char sz[MAX_PATH];
                        WideCharToMultiByte(
                            CP_ACP, 0, fd.Result, -1, sz, 
                            MAX_PATH, nullptr, nullptr
                        );
                        std::wstring wsz = _path;
                        wsz += L"\\cefshared.bin";
                        ek::io::FileStream fs(wsz, L"w");
                        fs.Write(sz, strlen(sz));
                        SendMessage(mbox, uMsg, wParam, lParam);
                    }
                
                    return true;
                }
            
                return false;
            },
            (LPARAM)cefpath).Show(hwnd, missing.c_str(), 
            L"Missing Important CEF Files", 
            MB_ICONINFORMATION | MB_YESNO
        ) == IDYES;
    }
\$\endgroup\$
1
  • \$\begingroup\$ Ah, this line: missing = L"The followig files are missing in " + std::wstring(cefpath) + L"\n" + missing + L".\nDo you want to browse the folder where the CefSharp binaries are located?";. Sorry, I didn't find this to be a really big deal since when it is compiled, line breaks won't matter anyway. But it's not a big problem for me. If it really needs to be broken into lines, OK. But I don't really see the significance doing that. \$\endgroup\$ Commented 17 hours ago
2
\$\begingroup\$

Identifiers

Identifiers that contain an underscore followed by a capital letter or another underscore are reserved for the implementation. Identifiers in the global namespace that start with an underscore are reserved for the implementation as well.

So your __cefBins and _path give undefined behavior.

Parameters

Right now, you go to rather elaborate lengths to pass the path to CheckDepends as an array instead of a std::wstring. That could (probably would) make sense if you were exporting CheckDepends from the DLL, and you wanted to assure that any sort of code could call it without causing problems with library version mismatches (or similar). But it's internal to the DLL, so there's no real point in that.

I'd change it to take the path parameter as a std::wstring.

Standard Library

Right now you use seqences of C library calls where you could use one simpler thing from the C++ library. An obvious example would be to replace this:

        WCHAR *ptr = wcsrchr(_path, '\\');
        *ptr = 0;
        wcscpy_s(szCefPath, _path);
        wcscat_s(szCefPath, L"\\cefshared.bin");

...with something like:

auto file_path = _path / L"cefshared.bin";

Your code also has potential undefined behavior: if wcsrchr doesn't find a trailing backslash, it'll return a null pointer. Then you write through that pointer without checking that it's non-null first.

Almost as bad, (or possibly even worse) if you pass a path without a trailing backslash, it can/will cut off the last subdirectory name. For example, let's assume the files are in a subdirectory: c:\configuration\CEF\. If this gets passed without a trailing backslash (c:\configuration\CEF), it'll search for the last \, and terminate the path there, so it'll look in c:\configuration, which is not what you want at all. To work correctly, you probably want to remove the last character if and only if it's a backslash.

Oh, except it doesn't necessarily need to be a backslash either. When Windows displays a path, it normally uses backslashes to separate components of the path. But it'll happily accept components separated with either backslashes or forward slashes. In fact, you can even combine backslashes and forward slashes, so something like c:\foo/bar\baz/qux.txt is a perfectly valid path, which is precisely equivalent to c:/foo\bar/baz\qux.txt.

The C++ version creates an std::filesystem::path object. Rather than extracting wchar_t * of that data to pass to ek::io::FileStream, I'd add an overload of ek::io::FileStream to accept a std::filesystem::path (and extract the wchar_t * internally there).

Functions

The code leans heavily toward a a small number of large functions. Personally, I'd much rather see smaller functions that are easier to understand.

For example, right now your CheckDepends is all one large function. I'd break it up into a few pieces. I'd start with a function to just see if a file exists, and return true or false to indicate the result:

bool find_file(std::wstring const &path, std::wstring const &file) {
    std::wstring full_path = L"/" + path + "/" + file;
    FILE *f = nullptr;
    auto err = _wfopen_s(&f, full_path.c_str(), L"r");
    fclose(f);
    return !err;
}

Then I'd have a function to use that to find any files missing from a specified set:

strings find_missing(std::wstring const &path, strings const &files) {
    strings ret;
    for (auto const &filename : files) {
        if (!find_file(path, filename))
            ret.push_back(filename);
        }
    }
    return ret;
}

I'm going to skip the code for the rest, but next would be a function to just display the message box and return its result.

Finally, we'd have CheckDepends that makes one call to see if the files are found, and if not makes a call to display the MessageBox/Dialog.

Likewise, in LaunchFile, it looks like the first sequence tries to find the path where we expect to find the other files. That looks to me like it could be a function of its own.

[And so on...]

CheckHeader

In LaunchProcess, you call CheckHeader, apparently to check the validity of the header of the executable you found. I'd leave that task to CreateProcess. At least as far as I can see, there are only really two possibilities here: either your CheckHeader precisely duplicates the header checking that CreateProcess will do anyway, or else your CheckHeader has a bug, and should be fixed so it does exactly what CreateProcess will do anyway.

Either way, I don't see any real way for CheckHeader to contribute anything useful here.

SetDllDirectory

Getting to the question you asked, separate from the actual code review: SetDllDirectory modifies searching for DLLs for both the current process, and any child processes it spawns. But of course, it doesn't affect other processes that are independent from this one. So if the user had previously launched A.exe, which sometimes loads DLLs, and then they launch something that calls this code, the fact that this code calls SetDllDirectory won't affect A.exe (but, as you apparently intended, it will affect the search for DLLs in the almost-immediately following call to CreateProcess).

\$\endgroup\$
1
  • \$\begingroup\$ I really like your idea of separating tasks. It didn't really come to mind as I haven't undergone any formal or something like that regarding proper code structuring. Anyway, about the _path, it came from this call: GetModuleFileName(reinterpret_cast<HINSTANCE>(&__ImageBase), _path, MAX_PATH);. So it is expected that the filename is included in the path. So what I did strip off the filename and concatenate \cefshared.bin to it. \$\endgroup\$ Commented 17 hours ago

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.