From a40c03917c48cbbbd01a26723e5a294b5821d36b Mon Sep 17 00:00:00 2001 From: Greenfire27 Date: Fri, 7 Aug 2026 15:19:15 -0400 Subject: [PATCH 1/2] Linux: implement Platform::pathCopy and Platform::fileRename Both were stubs returning false on the Unix back-end while Windows and macOS have had real ones all along, and a copy that never happens is not something a caller can see: pathCopy returns a bool that almost nobody checks, so the work simply did not get done and everything downstream blamed the missing files. That reaches further than it sounds. The Project Manager stamps a new project out of a template with a recursive pathCopy, and the editor's new project dialog copies the theme library the same way, so creating a project on Linux produced a folder with nothing in it. AppCore and the Gui Editor both give a theme its own copy of the stock cursor art, which is why the Gui Editor's cursor pane had no art to measure. fileRename is not exposed to script at all, but ModuleManager renames a module definition with it and ZipArchive swaps a rebuilt archive into place, and both were quietly failing. pathCopy takes a file or a whole tree, since both callers exist. Paths are used as given rather than sent through MungePath into the pref directory, matching isFile, isDirectory and fileDelete beside it: every caller builds an absolute path and then asks isFile whether the copy arrived, so a copy routed elsewhere would read as a failure. The directories above a destination are made on that same raw path for the same reason -- Platform::createPath would munge a relative one into the pref directory and leave the open to fail on a parent it had just created somewhere else. Details worth keeping: the mode is carried across so a copied executable is still executable; a copy that fails partway is unlinked rather than left for the next run to mistake for good art; the recursion asks isDirectory rather than trusting dirent::d_type, which is DT_UNKNOWN on filesystems that do not carry the kind; and a tree is refused if the destination is inside it, which would otherwise recurse until the path outgrew MaxPath. Platform::isSubDirectory looks like it would answer that last question and does not -- it matches a bare child name against the parent's entries, not one path inside another. fileRename is rename(2) with a copy-and-delete fallback on EXDEV, because the pref directory and the game directory are not always on one filesystem. dPathCopy goes: it was dead code, and it returned CopyFile's error flag as though it were a success flag, so anyone who had reached for it would have got the answer backwards. Covered by PlatformFileIOTests.PathCopyAndRename, which is also the only way to reach fileRename, having no script binding. The three failures left in PlatformStringTests on Linux -- dStrcatl, dStrrev and dItoa -- are separate pre-existing bugs in x86UNIXStrings.cc and are not touched here. Co-Authored-By: Claude Opus 5 (1M context) --- .../source/platformX86UNIX/x86UNIXFileio.cc | 217 +++++++++++++++++- .../testing/tests/platformFileIoTests.cc | 107 +++++++++ 2 files changed, 318 insertions(+), 6 deletions(-) diff --git a/engine/source/platformX86UNIX/x86UNIXFileio.cc b/engine/source/platformX86UNIX/x86UNIXFileio.cc index bbd470fc3..6fc48d41f 100755 --- a/engine/source/platformX86UNIX/x86UNIXFileio.cc +++ b/engine/source/platformX86UNIX/x86UNIXFileio.cc @@ -137,11 +137,6 @@ return error; } -bool dPathCopy(const char *fromName, const char *toName, bool nooverwrite) -{ - return CopyFile(fromName,toName); -} - //----------------------------------------------------------------------------- static char sgPrefDir[MaxPath]; static bool sgPrefDirInitialized = false; @@ -1249,14 +1244,224 @@ StringTableEntry Platform::osGetTemporaryDirectory() return StringTable->insert("~/"); } +//----------------------------------------------------------------------------- +// Copying, which the editors lean on harder than the name suggests: it is how a +// new project is stamped out of a template, and how a theme is given its own +// copy of the stock cursor art. +// +// Paths are used as handed over, like isFile and isDirectory and fileDelete +// above, rather than routed through MungePath into the pref directory the way +// createPath is. Every caller builds an absolute path first and then asks isFile +// whether the copy arrived, so a copy that landed anywhere else would read as a +// failure -- and MungePath leaves an absolute path alone in any case. +//----------------------------------------------------------------------------- + +static const U32 sCopyBufferSize = 32768; + +// The directories leading up to a file, made on the path exactly as given. +// Platform::createPath is not usable here: it sends a relative path through +// MungePath into the pref directory, which is not where the copy itself would +// then be written, so the open would fail on the parent it had just made +// somewhere else. +static void CreateParentDirectories(const char* path) +{ + char buffer[MaxPath]; + dStrncpy(buffer, path, MaxPath - 1); + buffer[MaxPath - 1] = '\0'; + + for (char* walk = dStrchr(buffer, '/'); walk != NULL; walk = dStrchr(walk + 1, '/')) + { + if (walk == buffer) + continue; // the leading slash of an absolute path + + *walk = '\0'; + mkdir(buffer, 0777); + *walk = '/'; + } +} + +static bool CopyOneFile(const char* fromName, const char* toName, bool nooverwrite) +{ + if (nooverwrite && (Platform::isFile(toName) || Platform::isDirectory(toName))) + return false; + + struct stat fromStat; + if (stat(fromName, &fromStat) < 0) + return false; + + // The destination's folder may not exist yet -- copying a tree creates the + // directories as it walks, but a lone file copied into a new folder does not. + CreateParentDirectories(toName); + + S32 fromFd = open(fromName, O_RDONLY); + if (fromFd < 0) + return false; + + // Carry the mode across, so a copied executable is still executable. + S32 toFd = open(toName, O_WRONLY | O_CREAT | O_TRUNC, fromStat.st_mode & 0777); + if (toFd < 0) + { + close(fromFd); + return false; + } + + char buffer[sCopyBufferSize]; + bool ok = true; + for (;;) + { + const ssize_t got = read(fromFd, buffer, sizeof(buffer)); + if (got == 0) + break; + if (got < 0) + { + if (errno == EINTR) + continue; + ok = false; + break; + } + + ssize_t written = 0; + while (written < got) + { + const ssize_t put = write(toFd, buffer + written, got - written); + if (put < 0) + { + if (errno == EINTR) + continue; + ok = false; + break; + } + written += put; + } + + if (!ok) + break; + } + + close(fromFd); + if (close(toFd) < 0) + ok = false; + + // A half-written file is worse than none: the next run would find it with + // isFile and take it for good art. + if (!ok) + unlink(toName); + + return ok; +} + +static bool CopyOneDirectory(const char* fromName, const char* toName) +{ + DIR* dir = opendir(fromName); + if (dir == NULL) + return false; + + struct stat fromStat; + if (stat(fromName, &fromStat) == 0) + mkdir(toName, fromStat.st_mode & 0777); + else + mkdir(toName, 0700); + + bool ok = true; + struct dirent* entry; + while ((entry = readdir(dir)) != NULL) + { + if (dStrcmp(entry->d_name, ".") == 0 || dStrcmp(entry->d_name, "..") == 0) + continue; + + char fromChild[MaxPath]; + char toChild[MaxPath]; + dSprintf(fromChild, sizeof(fromChild), "%s/%s", fromName, entry->d_name); + dSprintf(toChild, sizeof(toChild), "%s/%s", toName, entry->d_name); + + // Asking the filesystem rather than trusting d_type, which is DT_UNKNOWN + // on filesystems that do not carry the kind in the directory entry. + if (Platform::isDirectory(fromChild)) + { + if (!CopyOneDirectory(fromChild, toChild)) + ok = false; + } + else + { + // Overwriting freely: the caller's nooverwrite was already answered + // against the top of the tree, and stopping here would leave a + // half-copied project behind. + if (!CopyOneFile(fromChild, toChild, false)) + ok = false; + } + } + + closedir(dir); + return ok; +} + bool Platform::pathCopy(const char* source, const char* dest, bool nooverwrite) { + if (source == NULL || dest == NULL || !*source || !*dest) + return false; + + if (Platform::isFile(source)) + return CopyOneFile(source, dest, nooverwrite); + + if (Platform::isDirectory(source)) + { + if (nooverwrite && (Platform::isDirectory(dest) || Platform::isFile(dest))) + return false; + + // Refuse to copy a tree into itself, which would recurse until the path + // outgrew MaxPath. Platform::isSubDirectory is no help here: it matches a + // bare child name against the parent's entries, not one path inside + // another. + dsize_t sourceLen = dStrlen(source); + while (sourceLen > 1 && source[sourceLen - 1] == '/') + sourceLen--; // a trailing slash would put dest past the comparison + + if (dStrncmp(source, dest, sourceLen) == 0 && + (dest[sourceLen] == '/' || dest[sourceLen] == '\0')) + { + Con::errorf("Platform::pathCopy: %s is inside %s", dest, source); + return false; + } + + CreateParentDirectories(dest); + return CopyOneDirectory(source, dest); + } + + Con::errorf("Platform::pathCopy: nothing to copy at %s", source); return false; } bool Platform::fileRename(const char* source, const char* dest) { - return false; + if (source == NULL || dest == NULL || !*source || !*dest) + return false; + + if (!Platform::isFile(source) && !Platform::isDirectory(source)) + { + Con::errorf("Platform::fileRename: no file exists at %s", source); + return false; + } + + if (Platform::isFile(dest) || Platform::isDirectory(dest)) + Con::warnf("Platform::fileRename: overwriting %s", dest); + + CreateParentDirectories(dest); + + if (rename(source, dest) == 0) + return true; + + // rename cannot cross a filesystem, and the pref directory and the game + // directory are not always on the same one. Fall back to moving it by hand. + if (errno != EXDEV) + return false; + + if (!Platform::pathCopy(source, dest, false)) + return false; + + if (Platform::isDirectory(source)) + return Platform::deleteDirectory(source); + + return Platform::fileDelete(source); } bool Platform::fileDelete(const char* name) diff --git a/engine/source/testing/tests/platformFileIoTests.cc b/engine/source/testing/tests/platformFileIoTests.cc index 61b9adaf9..bb05fc173 100755 --- a/engine/source/testing/tests/platformFileIoTests.cc +++ b/engine/source/testing/tests/platformFileIoTests.cc @@ -122,6 +122,113 @@ TEST( PlatformFileIOTests, FileWriteRead ) SUCCEED(); } + +//----------------------------------------------------------------------------- +// Copying and renaming. Both were unimplemented stubs returning false on the +// Unix back-end, which is not something a caller can tell from a copy that +// simply did not happen: the editors stamp a new project out of a template and +// give a theme its own cursor art this way, and both silently produced nothing. +// +// Everything here is done against absolute paths built from the working +// directory, because a relative path handed to Platform::createPath is routed +// into the pref directory while the copy itself is written where it was asked +// for -- so a relative test would be checking two different places. +//----------------------------------------------------------------------------- + +#define PLATFORM_UNITTEST_COPY_ROOT "_unitTestCopy_RemoveMe" + +static void unitTestCopyPath( char* buffer, U32 bufferSize, const char* relative ) +{ + dSprintf( buffer, bufferSize, "%s/%s/%s", + Platform::getCurrentDirectory(), PLATFORM_UNITTEST_COPY_ROOT, relative ); +} + +static bool unitTestWriteFile( const char* path, const char* contents ) +{ + File file; + if ( file.open( path, File::Write ) != File::Ok ) + return false; + + U32 written = 0; + const bool ok = file.write( (U32)dStrlen( contents ), contents, &written ) == File::Ok; + file.close(); + return ok; +} + +TEST( PlatformFileIOTests, PathCopyAndRename ) +{ + char root[1024]; + dSprintf( root, sizeof( root ), "%s/%s", + Platform::getCurrentDirectory(), PLATFORM_UNITTEST_COPY_ROOT ); + + // Anything left by a previous run would make the overwrite checks lie. + // Asked only when there is something there, because deleteDirectory + // complains about a directory it cannot read. + if ( Platform::isDirectory( root ) ) + Platform::deleteDirectory( root ); + + char source[1024], target[1024], nested[1024]; + + // --- A single file, into a folder that does not exist yet. --- + unitTestCopyPath( source, sizeof( source ), "src/one.txt" ); + unitTestCopyPath( target, sizeof( target ), "dst/one.txt" ); + + ASSERT_TRUE( unitTestWriteFile( source, "hello" ) ) << "Could not write the source file."; + ASSERT_TRUE( Platform::isFile( source ) ) << "The source file was not created."; + + ASSERT_TRUE( Platform::pathCopy( source, target, true ) ) << "Copying a file failed."; + ASSERT_TRUE( Platform::isFile( target ) ) << "The copy is not there."; + ASSERT_EQ( Platform::getFileSize( source ), Platform::getFileSize( target ) ) << "The copy is a different size."; + + // --- nooverwrite is honoured in both directions. --- + ASSERT_FALSE( Platform::pathCopy( source, target, true ) ) << "Copying over an existing file should be refused."; + ASSERT_TRUE( Platform::pathCopy( source, target, false ) ) << "Copying over an existing file should be allowed when asked."; + + // --- A missing source fails, and leaves nothing behind. --- + unitTestCopyPath( source, sizeof( source ), "src/nosuch.txt" ); + unitTestCopyPath( target, sizeof( target ), "dst/nosuch.txt" ); + ASSERT_FALSE( Platform::pathCopy( source, target, true ) ) << "Copying a file that does not exist should fail."; + ASSERT_FALSE( Platform::isFile( target ) ) << "A failed copy left a file behind."; + + // --- A whole tree, which is how a project is stamped from a template. --- + unitTestCopyPath( source, sizeof( source ), "tree/top.txt" ); + ASSERT_TRUE( unitTestWriteFile( source, "top" ) ) << "Could not write the tree's top file."; + unitTestCopyPath( nested, sizeof( nested ), "tree/deep/deeper/bottom.txt" ); + ASSERT_TRUE( unitTestWriteFile( nested, "bottom" ) ) << "Could not write the tree's nested file."; + + unitTestCopyPath( source, sizeof( source ), "tree" ); + unitTestCopyPath( target, sizeof( target ), "treeCopy" ); + ASSERT_TRUE( Platform::pathCopy( source, target, true ) ) << "Copying a directory failed."; + + unitTestCopyPath( target, sizeof( target ), "treeCopy/top.txt" ); + ASSERT_TRUE( Platform::isFile( target ) ) << "The tree's top file did not come across."; + unitTestCopyPath( target, sizeof( target ), "treeCopy/deep/deeper/bottom.txt" ); + ASSERT_TRUE( Platform::isFile( target ) ) << "The tree's nested file did not come across."; + unitTestCopyPath( target, sizeof( target ), "treeCopy/deep/deeper" ); + ASSERT_TRUE( Platform::isDirectory( target ) ) << "The tree's nested folder did not come across."; + + // --- A tree copied into itself would recurse until the path ran out. --- + unitTestCopyPath( source, sizeof( source ), "tree" ); + unitTestCopyPath( target, sizeof( target ), "tree/inner" ); + ASSERT_FALSE( Platform::pathCopy( source, target, true ) ) << "A directory should not be copied into itself."; + + // --- Renaming, which has no script binding and so is only reachable here. --- + unitTestCopyPath( source, sizeof( source ), "dst/one.txt" ); + unitTestCopyPath( target, sizeof( target ), "dst/two.txt" ); + ASSERT_TRUE( Platform::fileRename( source, target ) ) << "Renaming a file failed."; + ASSERT_TRUE( Platform::isFile( target ) ) << "The renamed file is not at its new name."; + ASSERT_FALSE( Platform::isFile( source ) ) << "The renamed file is still at its old name."; + + unitTestCopyPath( source, sizeof( source ), "dst/nosuch.txt" ); + unitTestCopyPath( target, sizeof( target ), "dst/three.txt" ); + ASSERT_FALSE( Platform::fileRename( source, target ) ) << "Renaming a file that does not exist should fail."; + + // Tidy up, and check the cleanup itself worked so the next run starts clean. + ASSERT_TRUE( Platform::deleteDirectory( root ) ) << "Could not remove the test directory."; + ASSERT_FALSE( Platform::isDirectory( root ) ) << "The test directory is still there."; + + SUCCEED(); +} //----------------------------------------------------------------------------- #endif // TORQUE_SHIPPING From d2e11c97e06d9472bb664dc1d339ee54d04a1b79 Mon Sep 17 00:00:00 2001 From: Greenfire27 Date: Fri, 7 Aug 2026 15:45:13 -0400 Subject: [PATCH 2/2] Linux: fix dStrcatl, dStrrev and dItoa Three bugs in the Unix string back-end, each failing its own test in PlatformStringTests on every Linux run since the tests were written. dStrcatl walked one byte too far looking for the end of dst. "while (dstSize && *p++)" increments on the iteration that ends the loop too, so p came to rest past the terminator rather than on it and the append landed in the wrong place: "Garage" and "Games" concatenated to "Garage\0Games", which reads back as "Garage". The test and the pointer walk now agree. dStrrev reversed nothing. The loop read "x < 1" where it meant "x < l", so it ran a single iteration and swapped only the first and last character -- "GarageGames" came back as "sarageGameG". The corrected bound also keeps the XOR swap off the middle character of an odd-length string, which it would zero rather than leave alone, and off str[-1] for an empty one. dItoa never terminated the string it had just built. dStrrev measures with dStrlen, so it read past the digits into whatever the caller's buffer happened to hold and reversed that: dItoa(16384) produced "\x7F83614". Terminating before the reverse is what the K&R original this is copied from does. While here, the digits are taken off an unsigned copy, because the old "n = -n" is undefined for S32_MIN -- the one input that could not be printed at all. None of this is exotic; it is the sort of thing that survives because the Windows and macOS back-ends have their own copies of these functions and nobody runs the unit tests on Linux. PlatformStringTests now passes, and with the copy work in the preceding commit the whole suite is green on Linux for the first time: 88 tests, no failures. Co-Authored-By: Claude Opus 5 (1M context) --- .../source/platformX86UNIX/x86UNIXStrings.cc | 36 +++++++++++++------ 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/engine/source/platformX86UNIX/x86UNIXStrings.cc b/engine/source/platformX86UNIX/x86UNIXStrings.cc index 62593a375..6e50f4914 100755 --- a/engine/source/platformX86UNIX/x86UNIXStrings.cc +++ b/engine/source/platformX86UNIX/x86UNIXStrings.cc @@ -133,9 +133,14 @@ char* dStrcatl(char *dst, size_t dstSize, ...) AssertFatal(dstSize > 0, "dStrcatl: destination size is set zero"); dstSize--; // leave room for string termination - // find end of dst - while (dstSize && *p++) - dstSize--; + // find end of dst. Testing *p and stepping separately, because *p++ walks + // past the terminator on the iteration that ends the loop -- which left a + // gap, so "Garage" and "Games" concatenated to "Garage\0Games". + while (dstSize && *p) + { + p++; + dstSize--; + } va_list args; va_start(args, dstSize); @@ -386,7 +391,11 @@ S32 dStrrev(char *str) // Get string length S32 l = dStrlen(str) - 1; - for (int x = 0; x < 1; x++,l--) + // Walking in from both ends until they meet. This read "x < 1", which runs + // a single iteration: "GarageGames" came back as "sarageGameG". Stopping at + // x < l also keeps the XOR swap off the middle character of an odd-length + // string, which it would otherwise zero. + for (int x = 0; x < l; x++,l--) { // triple XOR trick str[x]^=str[l]; @@ -400,19 +409,24 @@ S32 dStrrev(char *str) S32 dItoa(S32 n, char s[]) { - S32 i, sign; + S32 i = 0; + const bool negative = (n < 0); - if ((sign = n) < 0) - n = -n; + // Digits are taken off an unsigned copy: negating S32_MIN does not fit back + // into an S32, and unsigned negation is the one form of it that is defined. + U32 value = negative ? (U32)0 - (U32)n : (U32)n; - i = 0; do { - s[i++] = n % 10 + '0'; - } while((n /= 10) > 0); + s[i++] = (char)(value % 10 + '0'); + } while((value /= 10) > 0); - if (sign < 0) + if (negative) s[i++] = '-'; + // Terminate BEFORE reversing: dStrrev measures with dStrlen, which without + // this ran off the digits into whatever the caller's buffer happened to hold. + s[i] = '\0'; + dStrrev(s); return dStrlen(s);