0

I'm trying to make a program that deletes a list of files one by one, using system(). The reason I'm using system() instead of remove() is because remove() doesn't support environment variables.

I've tried checking the return value, but my code seems to just always output that it's been deleted, even when it hasn't.

Code:

void fileLoop() {
    std::vector<std::string> fileList = { "\"%userprofile%\\test.txt\"" };
    while (!inter) {
        for (int count = 0; count <= 0; count++) {
            std::string moddedstring = "del /F " + fileList[count];
            DWORD checker = GetFileAttributes((LPCWSTR)fileList[count].c_str());
            if (checker == INVALID_FILE_ATTRIBUTES) {
                goto next;
            }
            else {
                system(moddedstring.c_str());
                MessageBoxW(NULL, L"File found, successfully deleted", L"File Deleted", MB_OK | MB_ICONWARNING);
            }
            next:
            std::cout << "\n";
        }
        Sleep(500);
    }
}

I thought there is some easy way to find this out. I haven't found a way like that yet.

I will be adding more paths later.

Update:

I've tried using OpenProcessToken() with ExpandEnvironmentStringsForUserW() to add ENVs

But it complains that my buffer is of type LPCSTR* even when I set it to LPCSTR

Thanks!

clouded.
  • 15
  • 6
  • related: https://stackoverflow.com/questions/22953027/batch-file-and-del-errorlevel-0-issue - only way with `del` is to either parse its output and look for an error message (you could use e.g. `_popen` or similar instead of `system`), or use something like `GetFileAttributes` (or `lstat`) to see if the file exists. – Jason C Jan 10 '23 at 02:45
  • PS while `del` doesn't return a meaningful status, you still probably want to get that `echo $?` out of there since it could just further confuse return codes. – Jason C Jan 10 '23 at 02:47
  • 6
    "The reason I'm using `system()` instead of `remove()` is because `remove()` doesn't support environment variables." - Then why don't you construct your file name by [looking up the environment variables yourself](https://stackoverflow.com/q/631664/364696)? – ShadowRanger Jan 10 '23 at 02:49
  • 5
    ^^^ you can use [`ExpandEnvironmentStringsW`](https://learn.microsoft.com/en-us/windows/win32/api/processenv/nf-processenv-expandenvironmentstringsw) (or `*A`) – Jason C Jan 10 '23 at 02:50
  • @JasonC: Thanks, I forgot they made it even easier on Windows. :-) I suppose for this specific case, `GetUserProfileDirectory` (`*W` or `*A` variant) or one of [the other `userenv.h` functions](https://learn.microsoft.com/en-us/windows/win32/api/userenv/) would also apply. – ShadowRanger Jan 10 '23 at 02:51
  • @JasonC I tried `GetFileAttributes` but when the file was found it didn't throw my Msgbox. Any idea why? – clouded. Jan 10 '23 at 02:55
  • To be clear, the reason I suggest this is that calling out via `system` limits your ability to detect and respond to errors; the exit code is not super-informative, relative to return codes, `errno`, `GetLastError()`, etc. – ShadowRanger Jan 10 '23 at 02:55
  • you'd have to show what you did. could be a good new question. – Jason C Jan 10 '23 at 02:56
  • Don't use `system`, it is a gaping hole into your program and allows remote code execution. – NathanOliver Jan 10 '23 at 02:57
  • @ShadowRanger My ENV that I want to use isn't the environment variable of the path, it's the ENV of `%USERPROFILE%`. If you know how to do that then please show me. – clouded. Jan 10 '23 at 02:57
  • @JasonC I have removed the `echo $?` out of my main program – clouded. Jan 10 '23 at 02:59
  • 1
    @TheException: `%USERPROFILE%` is just how you say "look up `USERPROFILE` from the environment and replace the placeholder with it". The `ExpandEnvironmentStrings` function JasonC mentioned can do the same work; [the docs have examples](https://learn.microsoft.com/en-us/windows/win32/sysinfo/getting-system-information). – ShadowRanger Jan 10 '23 at 03:16
  • 1
    @TheException the *better* way to get the user's profile path is to use `SHGetFolderPath(CSIDL_PROFILE)` or `SHGetKnownFolderPath(FOLDERID_Profile)`. That said, use `DeleteFile()` instead of `system("del")`. And, `(LPCWSTR)fileList[count].c_str()` will not work like you think, drop the typecast and use `GetFileAttributesA()` instead. Or, simply don't bother checking attributes at all, call `DeleteFile()` unconditionally and let it tell you if it succeeded or failed. – Remy Lebeau Jan 10 '23 at 04:09
  • @RemyLebeau `SHGetFolderPath()` and `SHGetKnownFolderPath()` have **a lot** more params than that. – clouded. Jan 10 '23 at 04:17
  • @TheException I'm aware of that, I was not showing you actual code snippets, simply mentioning the function names and the appropriate folder IDs to use with them. – Remy Lebeau Jan 10 '23 at 09:08
  • There is no reason for not using [file APIs](https://learn.microsoft.com/en-us/windows/win32/api/fileapi/) and the number of parameters should not be a disadvantage. Or at least, as @JasonC suggested, use [ExpandEnvironmentStrings](https://learn.microsoft.com/en-us/windows/win32/sysinfo/getting-system-information) which works for me. – YangXiaoPo-MSFT Jan 11 '23 at 06:09

1 Answers1

2

A better way to get the user's profile path is to simply ask the OS directly, via SHGetFolderPath(CSIDL_PROFILE) or SHGetKnownFolderPath(FOLDERID_Profile).

Also, you should use DeleteFileA() instead of system("del"). There is no need to spawn an external console process when you can do the deletion directly. Also, because you are interested in error checking, which is much easier if you use the Win32 API directly.

Also, (LPCWSTR)fileList[count].c_str() will not work like you think. You can't convert a const char* to a const wchar_t* using a simple typecast. Drop the typecast and use GetFileAttributesA() instead.

Or, simply don't bother checking attributes at all. You can call DeleteFileA() unconditionally and let it tell you if it actually succeeded or failed.

With that said, try something more like this:

#include <shlobj.h>

std::string getUserProfilePath() {
    CHAR szPath[MAX_PATH];
    if (FAILED(SHGetFolderPathA(NULL, CSIDL_PROFILE, NULL, SHGFP_TYPE_CURRENT, szPath))) {
        // error handling...
        throw ...;
    }
    int len = lstrlenA(szPath);
    szPath[len] = '\\';
    return std::string(szPath, len + 1);

    /*
    PWSTR pszPath;
    if (FAILED(SHGetKnownFolderPath(FOLDERID_Profile, KF_FLAG_DEFAULT, NULL, &pszPath))) {
        // error handling...
        throw ...;
    }
    int wlen = lstrlenW(pszPath);
    int len = WideCharToMultiByte(0, 0, pszPath, wlen, NULL, 0, NULL, NULL);
    if (len == 0) {
        // error handling...
        throw ...;
    }
    std::vector<CHAR> buffer(len + 1);
    len = WideCharToMultiByte(CP_ACP, 0, pszPath, wlen, buffer.data(), len, NULL, NULL);
    if (len == 0) {
        // error handling...
        throw ...;
    }
    buffer[len] = '\\';
    CoTaskMemFree(pszPath);
    return std::wstring(buffer.data(), buffer.size());
    */
}

void fileLoop() {
    std::vector<std::string> fileList = { getUserProfilePath() + "test.txt" };
    while (!inter) {
        for (size_t count = 0; count < fileList.size(); ++count) {
            if (DeleteFileA(fileList[count].c_str())) {
                MessageBoxW(NULL, L"File found, successfully deleted", L"File Deleted", MB_OK | MB_ICONWARNING);
            } else {
                // error handling...
            }
            std::cout << "\n";
        }
        Sleep(500);
    }
}

Alternatively, using Unicode strings instead (which you really should use when interacting with the filesystem):

#include <shlobj.h>

std::wstring getUserProfilePath() {
    WCHAR szPath[MAX_PATH];
    if (FAILED(SHGetFolderPathW(NULL, CSIDL_PROFILE, NULL, SHGFP_TYPE_CURRENT, szPath))) {
        // error handling...
        throw ...;
    }
    int len = lstrlenW(szPath);
    szPath[len] = '\\';
    return std::wstring(szPath, len + 1);

    /*
    PWSTR pszPath;
    if (FAILED(SHGetKnownFolderPath(FOLDERID_Profile, KF_FLAG_DEFAULT, NULL, &pszPath))) {
        // error handling...
        throw ...;
    }
    int len = lstrlenW(pszPath);
    std::wstring sPath(len + 1, '\0');
    std::copy(pszPath, pszPath + len, sPath.begin());
    sPath[len] = '\\';
    CoTaskMemFree(pszPath);
    return sPath;
    */
}

void fileLoop() {
    std::vector<std::wstring> fileList = { getUserProfilePath() + L"test.txt" };
    while (!inter) {
        for (size_t count = 0; count < fileList.size(); ++count) {
            if (DeleteFileW(fileList[count].c_str())) {
                MessageBoxW(NULL, L"File found, successfully deleted", L"File Deleted", MB_OK | MB_ICONWARNING);
            } else {
                // error handling...
            }
            std::cout << "\n";
        }
        Sleep(500);
    }
}

Even better, if you are using C++17 or later, consider using the <filesystem> library instead:

#include <shlobj.h>
#include <filesystem>

using fs = std::filesystem;

fs::path getUserProfilePath() {
    WCHAR szPath[MAX_PATH];
    if (FAILED(SHGetFolderPathW(NULL, CSIDL_PROFILE, NULL, SHGFP_TYPE_CURRENT, szPath))) {
        // error handling...
        throw ...;
    }
    return szPath;

    /*
    PWSTR pszPath;
    if (FAILED(SHGetKnownFolderPath(FOLDERID_Profile, KF_FLAG_DEFAULT, NULL, &pszPath))) {
        // error handling...
        throw ...;
    }
    fs::path pth(pszPath);
    CoTaskMemFree(pszPath);
    return pth;
    */
}

void fileLoop() {
    std::vector<fs::path> fileList = { getUserProfilePath() / L"test.txt" };
    while (!inter) {
        for (auto &pth : fileList) {
            std::error_code ec;
            if (fs::remove(pth, ec)) {
                MessageBoxW(NULL, L"File found, successfully deleted", L"File Deleted", MB_OK | MB_ICONWARNING);
            } else {
                // error handling...
            }
            std::cout << "\n";
        }
        Sleep(500);
    }
}
Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
  • Is `remove()` okay to use? I also used `f.good()` to check if the file exists. By using that and `SHGetFolderPath` it worked! – clouded. Jan 11 '23 at 22:13
  • 1
    "*Is `remove()` okay to use?*" - Why wouldn't it be okay? On Windows, it is just going to call `DeleteFile()` internally for you. So cut out the middle-man. "*I also used `f.good()` to check if the file exists*" - there is no `f` in the code shown. But, in any case, you should not check if a file exists before creating/deleting it. That is a [TOCTOU race condition](https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use). Let the file system handle that for you. Simply create/delete the file unconditionally and let it fail if the file (already|does not) exist(s), respectively. – Remy Lebeau Jan 11 '23 at 22:42