Why does SHFileOperationW create folder instead of move file?

Question

I have created own "Move" function based on SHFileOperationW like this:

int MoveItem(string strSourcePath, string strDestinationDirectory, string strNewName, bool bAllowOverwrite, bool bCreateDestinationDirectoryIfNotExist)
{
    int intPositionOfLastSlash;
    int intResult = 0;

    SHFILEOPSTRUCTW sfosMove;

    string strNewPath;
    string strOldName;

    wchar_t a_wcNewPath[MAX_PATH] = {0};
    wchar_t a_wcSourcePath[MAX_PATH] = {0};

    wstring wsNewPath;
    wstring wsSourcePath;

    if((IsFileExist(strSourcePath) == false) && (IsFolderExist(strSourcePath) == false))
    {
        intResult = 0x7C;
    }

    else
    {
        if(IsFolderExist(strDestinationDirectory) == false)
        {
            if(bCreateDestinationDirectoryIfNotExist == false)
            {
                intResult = 0x7C;
            }
        }
    }

    if(intResult == 0)
    {
        intPositionOfLastSlash = strSourcePath.find_last_of("\\");
        strOldName = strSourcePath.substr(intPositionOfLastSlash + 1);

        if(strNewName == "")
        {
            strNewPath = strDestinationDirectory + "\\" + strOldName;
        }

        else
        {
            strNewPath = strDestinationDirectory + "\\" + strNewName;
        }

        if(bAllowOverwrite == false)
        {
            if(IsFileExist(strNewPath) == true)
            {
                intResult = 0x7E;
            }

            else if(IsFolderExist(strNewPath) == true)
            {
                intResult = 0x80;
            }
        }
    }

    if(intResult == 0)
    {
        /*- Source Path -*/
        wsSourcePath = Utf8StringToWstring(strSourcePath);

        StrCpyW(a_wcSourcePath, wsSourcePath.c_str());

        CopyMemory(a_wcSourcePath + lstrlenW(a_wcSourcePath), L"\0\0", 2);
        /*---------------*/

        /*- New Path -*/
        wsNewPath = Utf8StringToWstring(strNewPath);

        StrCpyW(a_wcNewPath, wsNewPath.c_str());

        CopyMemory(a_wcNewPath + lstrlenW(a_wcNewPath), L"\0\0", 2);
        /*-------------------------*/

        /*- Move Item -*/
        sfosMove.fFlags = FOF_NOCONFIRMATION | FOF_NOERRORUI | FOF_SILENT;
        sfosMove.pFrom = a_wcSourcePath;
        sfosMove.pTo = a_wcNewPath;
        sfosMove.wFunc = FO_MOVE;

        intResult = SHFileOperationW(&sfosMove);
        /*-------------*/
    }

    return intResult;
}

But The problem is when I use this function repeatedly, only first file is moved. The function creates folders with name of files.

For example:

First file: abc.txt (This file is moved.) Second file: def.txt (This file is not moved. The function creates a folder named "def.txt")

Example usage creates the problem:

// "example_folder" is already present in "D:" drive.
int intResult;

intResult = MoveItem("C:\\ProgramData\\abc.txt", "D:\\example_folder", "", true, false);

if(intResult == 0)
{
     intResult = MoveItem("C:\\ProgramData\\def.txt", "D:\\example_folder", "", true, false);     
}   

Answer 1

☠️ Potential buffer overruns in several places here.

1. You should be allocating filename buffers with size [MAX_PATH + 1], because they must have enough room to hold MAX_PATH characters and the terminating double-null.
2. You are only copying two bytes to the end of both your source and your destination paths, when your intent is to copy four. L"\0\0" is two wide characters, which is four bytes.

CopyMemory(a_wcSourcePath + lstrlenW(a_wcSourcePath), L"\0\0", 2);

Will this matter? Practically speaking, probably not, as you did initialize the buffer to all-zeroes. But it's a recipe for potential disaster.

3. You are copying to a buffer that may not have room for the characters you're copying. (It would usually work out, since presumably your source path is not longer than MAX_PATH, but you're dealing with conversions as well, from UTF-8 to UTF-16; for safety's sake, you should use StringCchCopy() or similar.)

    wsSourcePath = Utf8StringToWstring(strSourcePath);

    StrCpyW(a_wcSourcePath, wsSourcePath.c_str());

4. You are not initializing the SHFILEOPSTRUCTW struct, so you have no way of knowing what sort of data is sitting in its various parts. Like this:

SHFILEOPSTRUCTW sfosMove{};

5. Will fixing all of those issues fix your problem? Maybe, but I don't know. (You have several functions in your pasted code that are not defined, for one thing.) When I run this code, more or less as you have it there, with the problems I've noted here fixed, it seems to work fine.

This whole answer might get downgraded because it's not definitively an "answer," but hopefully it saves you from some trouble down the line.

Answer 2

I'd start by using the std::filesystem function to handle path manipulation and such.

I'd probably also use std::filesystem::rename to move the files, but there may be a case to make for using MoveFileEx instead. In particular, I haven't tried to check whether Microsoft's implementation of rename will actually move a file from one drive to another (but I'd guess it probably does).

So, a version using just std::filesystem stuff would be something like this:

bool MoveItem(string sourcePath, string destDirectory, string destName, bool allowOverwrite, bool createDest) {

    namespace fs = std::filesystem;

    fs::path src{sourcePath};
    fs::path dest{destDirectory};

    if (!fs::exists(src))
        return false;

    if (createDest) {
        fs::create_directories(destDirectory); // does nothing if already exists
    } else if (!fs::is_directory(dest)) {
        return false;   // fail if it doesn't exist and we aren't allowed to create it.
    }

    dest = dest / (destName.empty() ? src.filename() : destName);

    if (!allowOverwrite && fs::exists(dest)) {
        return false;
    }

    std::error_code ec;
    fs::rename(src, dest, ec);
    return !ec;
}

If you wanted to use MoveFileEx instead, the last part of the code (after the dest = dest / ... line) would look something like this:

DWORD flags{ MOVEFILE_COPY_ALLOWED };

if (allowOverwrite)
    flags |= MOVEFILE_REPLACE_EXISTING;

return MoveFileEx(src.string().c_str(), dest.string().c_str(), flags) != 0;

I haven't tried to test every possible scenario (different combinations of flags, directory and/or files that already exist, moving between drives, over networks, etc.) but did a quick little test with code like this:

#include <vector>
#include <fstream>

int main() {
    std::vector<string> testNames { "a.test", "b.test", "c.test"};

    // Create a few files to test with:
    for (auto t : testNames) {
        std::ofstream o(t);
    }

    // move them to some innocuous destination:
    for (auto t : testNames) {
        MoveItem(t, "c:/junk", "", false, true);
    }
}