Make changes according to recommendations by Copilot

- correction spelling of suppress_error
- improved error handling in remove() based on functionality in get_fileattr() and somewhat changed error handling in get_fileattr() itself
- call explicitly LLFile::fopen() to make sure we use the correct file path conversion under Windows

Removing Flawfinder comments since Flawfinder isn't used in the viewer anymore
Adding an option to support symlink detection in getattr()
Adding comments to function implementation to indicate that they are really static functions of the LLFile class

2 files changed, 123 insertions, 77 deletions

diff --git a/indra/llcommon/llfile.cpp b/indra/llcommon/llfile.cpp
index a24524d699..492c7c690b 100644
--- a/indra/llcommon/llfile.cpp
+++ b/indra/llcommon/llfile.cpp
@@ -48,15 +48,18 @@ static std::string empty;
 // variants of strerror() to report errors.
 
 #if LL_WINDOWS
+// For the situations where we directly call into Windows API functions we need to translate
+// the Windows error codes into errno values
 namespace
 {
     struct errentry
     {
-        unsigned long oserr; // OS return value
+        unsigned long oserr; // Windows OS error value
         int errcode;         // System V error code
     };
 }
 
+// translation table between Windows OS error value and System V errno code
 static errentry const errtable[]
 {
     { ERROR_INVALID_FUNCTION,       EINVAL    },  //    1
@@ -106,20 +109,17 @@ static errentry const errtable[]
     { ERROR_NOT_ENOUGH_QUOTA,       ENOMEM    }   // 1816
 };
 
-// Number of elements in the error translation table
-#define ERRTABLECOUNT (sizeof(errtable) / sizeof(errtable[0]))
-
 static int set_errno_from_oserror(unsigned long oserr)
 {
     if (!oserr)
         return 0;
 
-    // Check the table for the OS error code
-    for (unsigned i{0}; i < ERRTABLECOUNT; ++i)
+    // Check the table for the Windows OS error code
+    for (const struct errentry &entry : errtable)
     {
-        if (oserr == errtable[i].oserr)
+        if (oserr == entry.oserr)
         {
-            _set_errno(errtable[i].errcode);
+            _set_errno(entry.errcode);
             return -1;
         }
     }
@@ -156,40 +156,46 @@ static std::wstring utf8path_to_wstring(const std::string& utf8path)
     return ll_convert<std::wstring>(utf8path);
 }
 
-static unsigned short get_fileattr(const std::wstring& utf16path)
+static unsigned short get_fileattr(const std::wstring& utf16path, bool dontFollowSymLink = false)
 {
-    unsigned short st_mode = 0;
-    HANDLE file_handle = CreateFileW(utf16path.c_str(), FILE_READ_ATTRIBUTES,
-                                     FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr,
-                                     OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, nullptr);
-    if (file_handle == INVALID_HANDLE_VALUE)
+    unsigned long  flags = FILE_FLAG_BACKUP_SEMANTICS;
+    if (dontFollowSymLink)
     {
-        set_errno_from_oserror(GetLastError());
-        return 0;
+        flags |= FILE_FLAG_OPEN_REPARSE_POINT;
     }
-
-    FILE_ATTRIBUTE_TAG_INFO attribute_info;
-    if (GetFileInformationByHandleEx(file_handle, FileAttributeTagInfo, &attribute_info, sizeof(attribute_info)))
+    HANDLE file_handle = CreateFileW(utf16path.c_str(), FILE_READ_ATTRIBUTES,
+                                     FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE,
+                                     nullptr, OPEN_EXISTING, flags, nullptr);
+    if (file_handle != INVALID_HANDLE_VALUE)
     {
-        // A volume path alone (only drive letter) is not recognized as directory while it technically is
-        bool is_directory = (attribute_info.FileAttributes & FILE_ATTRIBUTE_DIRECTORY) ||
-                            (iswalpha(utf16path[0]) && utf16path[1] == ':' &&
-                             (!utf16path[2] || (is_slash(utf16path[2]) && !utf16path[3])));
-        st_mode |= is_directory ? S_IFDIR : S_IFREG;
-        st_mode |= (attribute_info.FileAttributes & FILE_ATTRIBUTE_READONLY) ? S_IREAD : S_IREAD | S_IWRITE;
-        // we do not try to guess executable flag
-
-        // propagate user bits to group/other fields:
-        st_mode |= (st_mode & 0700) >> 3;
-        st_mode |= (st_mode & 0700) >> 6;
+        FILE_ATTRIBUTE_TAG_INFO attribute_info;
+        if (GetFileInformationByHandleEx(file_handle, FileAttributeTagInfo, &attribute_info, sizeof(attribute_info)))
+        {
+            // A volume path alone (only drive letter) is not recognized as directory while it technically is
+            bool is_directory = (attribute_info.FileAttributes & FILE_ATTRIBUTE_DIRECTORY) ||
+                                (iswalpha(utf16path[0]) && utf16path[1] == ':' &&
+                                 (!utf16path[2] || (is_slash(utf16path[2]) && !utf16path[3])));
+            unsigned short st_mode = is_directory ? S_IFDIR :
+                                     (attribute_info.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT ? S_IFLNK : S_IFREG);
+            st_mode |= (attribute_info.FileAttributes & FILE_ATTRIBUTE_READONLY) ? S_IREAD : S_IREAD | S_IWRITE;
+            // we do not try to guess executable flag
+
+            // propagate user bits to group/other fields:
+            st_mode |= (st_mode & 0700) >> 3;
+            st_mode |= (st_mode & 0700) >> 6;
+
+            CloseHandle(file_handle);
+            return st_mode;
+        }
     }
-    else
+    // Retrieve last error and set errno before calling CloseHandle()
+    set_errno_from_oserror(GetLastError());
+
+    if (file_handle != INVALID_HANDLE_VALUE)
     {
-        // Retrieve last error before calling CloseHandle()
-        set_errno_from_oserror(GetLastError());
+        CloseHandle(file_handle);
     }
-    CloseHandle(file_handle);
-    return st_mode;
+    return 0;
 }
 
 #else
@@ -241,7 +247,7 @@ std::string strerr(int errn)
 }
 #endif  // ! LL_WINDOWS
 
-int warnif(const std::string& desc, const std::string& filename, int rc, int accept = 0)
+static int warnif(const std::string& desc, const std::string& filename, int rc, int accept = 0)
 {
     if (rc < 0)
     {
@@ -333,7 +339,7 @@ int LLFile::mkdir(const std::string& dirname, int perms)
 }
 
 // static
-int LLFile::rmdir(const std::string& dirname, int supress_error)
+int LLFile::rmdir(const std::string& dirname, int suppress_error)
 {
 #if LL_WINDOWS
     std::wstring utf16dirname = utf8path_to_wstring(dirname);
@@ -341,21 +347,22 @@ int LLFile::rmdir(const std::string& dirname, int supress_error)
 #else
     int rc = ::rmdir(dirname.c_str());
 #endif
-    return warnif("rmdir", dirname, rc, supress_error);
+    return warnif("rmdir", dirname, rc, suppress_error);
 }
 
 // static
-LLFILE* LLFile::fopen(const std::string& filename, const char* mode)    /* Flawfinder: ignore */
+LLFILE* LLFile::fopen(const std::string& filename, const char* mode)
 {
 #if LL_WINDOWS
     std::wstring utf16filename = utf8path_to_wstring(filename);
     std::wstring utf16mode = ll_convert<std::wstring>(std::string(mode));
     return _wfopen(utf16filename.c_str(), utf16mode.c_str());
 #else
-    return ::fopen(filename.c_str(),mode);  /* Flawfinder: ignore */
+    return ::fopen(filename.c_str(),mode);
 #endif
 }
 
+// static
 int LLFile::close(LLFILE * file)
 {
     int ret_value = 0;
@@ -366,9 +373,10 @@ int LLFile::close(LLFILE * file)
     return ret_value;
 }
 
+// static
 std::string LLFile::getContents(const std::string& filename)
 {
-    LLFILE* fp = fopen(filename, "rb"); /* Flawfinder: ignore */
+    LLFILE* fp = LLFile::fopen(filename, "rb");
     if (fp)
     {
         fseek(fp, 0, SEEK_END);
@@ -385,7 +393,8 @@ std::string LLFile::getContents(const std::string& filename)
     return LLStringUtil::null;
 }
 
-int LLFile::remove(const std::string& filename, int supress_error)
+// static
+int LLFile::remove(const std::string& filename, int suppress_error)
 {
 #if LL_WINDOWS
     // Posix remove() works on both files and directories although on Windows
@@ -393,8 +402,7 @@ int LLFile::remove(const std::string& filename, int supress_error)
     // as its siblings unlink() and _wunlink().
     // If we really only want to support files we should instead use
     // unlink() in the non-Windows part below too
-    int rc = 0;
-    ;
+    int rc = -1;
     std::wstring utf16filename = utf8path_to_wstring(filename);
     unsigned short st_mode = get_fileattr(utf16filename);
     if (S_ISDIR(st_mode))
@@ -405,17 +413,25 @@ int LLFile::remove(const std::string& filename, int supress_error)
     {
         rc = _wunlink(utf16filename.c_str());
     }
-    else
+    else if (st_mode)
     {
+        // it is something else than a file or directory
+        // this should not really happen as long as we do not allow for symlink
+        // detection in the optional parameter to get_fileattr()
         rc = set_errno_from_oserror(ERROR_INVALID_PARAMETER);
     }
+    else
+    {
+        // get_filattr() failed and already set errno, preserve it for correct error reporting
+    }
 #else
     int rc = ::remove(filename.c_str());
 #endif
-    return warnif("remove", filename, rc, supress_error);
+    return warnif("remove", filename, rc, suppress_error);
 }
 
-int LLFile::rename(const std::string& filename, const std::string& newname, int supress_error)
+// static
+int LLFile::rename(const std::string& filename, const std::string& newname, int suppress_error)
 {
 #if LL_WINDOWS
     // Posix rename() will gladly overwrite a file at newname if it exists, the Windows
@@ -431,25 +447,26 @@ int LLFile::rename(const std::string& filename, const std::string& newname, int
 #else
     int rc = ::rename(filename.c_str(),newname.c_str());
 #endif
-    return warnif(STRINGIZE("rename to '" << newname << "' from"), filename, rc, supress_error);
+    return warnif(STRINGIZE("rename to '" << newname << "' from"), filename, rc, suppress_error);
 }
 
 // Make this a define rather than using magic numbers multiple times in the code
 #define LLFILE_COPY_BUFFER_SIZE 16384
 
+// static
 bool LLFile::copy(const std::string& from, const std::string& to)
 {
     bool copied = false;
-    LLFILE* in = fopen(from, "rb");     /* Flawfinder: ignore */
+    LLFILE* in = LLFile::fopen(from, "rb");
     if (in)
     {
-        LLFILE* out = fopen(to, "wb");      /* Flawfinder: ignore */
+        LLFILE* out = LLFile::fopen(to, "wb");
         if (out)
         {
-            char buf[LLFILE_COPY_BUFFER_SIZE]; /* Flawfinder: ignore */
+            char buf[LLFILE_COPY_BUFFER_SIZE];
             size_t readbytes;
             bool write_ok = true;
-            while (write_ok && (readbytes = fread(buf, 1, LLFILE_COPY_BUFFER_SIZE, in))) /* Flawfinder: ignore */
+            while (write_ok && (readbytes = fread(buf, 1, LLFILE_COPY_BUFFER_SIZE, in)))
             {
                 if (fwrite(buf, 1, readbytes, out) != readbytes)
                 {
@@ -468,7 +485,8 @@ bool LLFile::copy(const std::string& from, const std::string& to)
     return copied;
 }
 
-int LLFile::stat(const std::string& filename, llstat* filestatus, int supress_error)
+// static
+int LLFile::stat(const std::string& filename, llstat* filestatus, int suppress_error)
 {
 #if LL_WINDOWS
     std::wstring utf16filename = utf8path_to_wstring(filename);
@@ -476,24 +494,25 @@ int LLFile::stat(const std::string& filename, llstat* filestatus, int supress_er
 #else
     int rc = ::stat(filename.c_str(), filestatus);
 #endif
-    return warnif("stat", filename, rc, supress_error);
+    return warnif("stat", filename, rc, suppress_error);
 }
 
-// _wstat() is a bit heavyweight on Windows, use a more lightweight API
-// to just get the attributes
-unsigned short LLFile::getattr(const std::string& filename, int suppress_error)
+// static
+unsigned short LLFile::getattr(const std::string& filename, bool dontFollowSymLink, int suppress_error)
 {
 #if LL_WINDOWS
-    int rc = -1;
+    // _wstat64() is a bit heavyweight on Windows, use a more lightweight API
+    // to just get the attributes
+    int            rc            = -1;
     std::wstring utf16filename = utf8path_to_wstring(filename);
-    unsigned short st_mode = get_fileattr(utf16filename);
+    unsigned short st_mode = get_fileattr(utf16filename, dontFollowSymLink);
     if (st_mode)
     {
         return st_mode;
     }
 #else
     llstat filestatus;
-    int rc = ::stat(filename.c_str(), &filestatus);
+    int rc = dontFollowSymLink ? ::lstat(filename.c_str(), &filestatus) : ::stat(filename.c_str(), &filestatus);
     if (rc == 0)
     {
         return filestatus.st_mode;
@@ -503,18 +522,25 @@ unsigned short LLFile::getattr(const std::string& filename, int suppress_error)
     return 0;
 }
 
+// static
 bool LLFile::isdir(const std::string& filename)
 {
-    // Don't spam the log if the subject pathname doesn't exist.
-    return S_ISDIR(getattr(filename, ENOENT));
+    return S_ISDIR(getattr(filename));
 }
 
+// static
 bool LLFile::isfile(const std::string& filename)
 {
-    // Don't spam the log if the subject pathname doesn't exist.
-    return S_ISREG(getattr(filename, ENOENT));
+    return S_ISREG(getattr(filename));
+}
+
+// static
+bool LLFile::islink(const std::string& filename)
+{
+    return S_ISLNK(getattr(filename, true));
 }
 
+// static
 const char *LLFile::tmpdir()
 {
     static std::string utf8path;
diff --git a/indra/llcommon/llfile.h b/indra/llcommon/llfile.h
index 9747bb67d8..04a2946ac4 100644
--- a/indra/llcommon/llfile.h
+++ b/indra/llcommon/llfile.h
@@ -41,8 +41,9 @@ typedef FILE    LLFILE;
 #include <sys/stat.h>
 
 #if LL_WINDOWS
-// windows version of stat function and stat data structure are called _statxx
-typedef struct _stat64    llstat;
+// The Windows version of stat function and stat data structure are called _stat64
+// We use _stat64 here to support 64-bit st_size and time_t values
+typedef struct _stat64  llstat;
 #else
 typedef struct stat     llstat;
 #include <sys/types.h>
@@ -56,6 +57,16 @@ typedef struct stat     llstat;
 # define S_ISDIR(x) (((x) & S_IFMT) == S_IFDIR)
 #endif
 
+// Windows C runtime library does not define this and does not support symlink detection in the
+// stat functions but we do in our getattr() function
+#ifndef S_IFLNK
+#define S_IFLNK 0xA000 /* symlink */
+#endif
+
+#ifndef S_ISLNK
+#define S_ISLNK(x) (((x) & S_IFMT) == S_IFLNK)
+#endif
+
 #include "llstring.h" // safe char* -> std::string conversion
 
 /// LLFile is a class of static functions operating on paths
@@ -96,39 +107,44 @@ public:
     ///  @returns 0 on success and -1 on failure.
 
     //// remove a directory
-    static  int     rmdir(const std::string& filename, int supress_error = 0);
-    ///< pass ENOENT in the optional 'supress_error' parameter
+    static  int     rmdir(const std::string& filename, int suppress_error = 0);
+    ///< pass ENOENT in the optional 'suppress_error' parameter
     ///  if you don't want a warning in the log when the directory does not exist
     ///  @returns 0 on success and -1 on failure.
 
     /// remove a file or directory
-    static  int     remove(const std::string& filename, int supress_error = 0);
-    ///< pass ENOENT in the optional 'supress_error' parameter
+    static  int     remove(const std::string& filename, int suppress_error = 0);
+    ///< pass ENOENT in the optional 'suppress_error' parameter
     ///  if you don't want a warning in the log when the directory does not exist
     ///  @returns 0 on success and -1 on failure.
 
     /// rename a file
-    static  int     rename(const std::string& filename, const std::string& newname, int supress_error = 0);
+    static  int     rename(const std::string& filename, const std::string& newname, int suppress_error = 0);
     ///< it will silently overwrite newname if it exists without returning an error
+    ///  Posix guarantees that if newname already exists, then there will be no moment
+    ///  in which for other processes newname does not exist. There is no such guarantee
+    ///  under Windows at this time. It may do it in the same way but the used Windows API
+    ///  does not make such guarantees.
     ///  @returns 0 on success and -1 on failure.
 
 
-    // copy the contents of file from 'from' to 'to' filename
+    /// copy the contents of file from 'from' to 'to' filename
     static  bool    copy(const std::string& from, const std::string& to);
     ///< @returns true on success and false on failure.
 
     /// return the file stat structure for filename
-    static  int     stat(const std::string& filename, llstat* file_status, int supress_error = ENOENT);
+    static  int     stat(const std::string& filename, llstat* file_status, int suppress_error = ENOENT);
     ///< for compatibility with existing uses of LL_File::stat() we use ENOENT as default in the
-    ///  optional 'supress_error' parameter to avoid spamming the log with warnings when the API
+    ///  optional 'suppress_error' parameter to avoid spamming the log with warnings when the API
     ///  is used to detect if a file exists
     ///  @returns 0 on success and -1 on failure.
 
     /// get the file or directory attributes for filename
-    static  unsigned short getattr(const std::string& filename, int supress_error = 0);
+    static  unsigned short getattr(const std::string& filename, bool dontFollowSymLink = false, int suppress_error = ENOENT);
     ///< a more lightweight function on Windows to stat, that just returns the file attribute flags
-    ///  pass ENOENT in the optional 'supress_error' parameter if you don't want a warning
-    ///  in the log when the file or directory does not exist
+    ///  dontFollowSymLinks set to true returns the attributes of the symlink if it is one, rather than resolving it
+    ///  we pass by default ENOENT in the optional 'suppress_error' parameter to not spam the log with
+    ///  warnings when the file or directory does not exist
     ///  @returns 0 on failure and a st_mode value with either S_IFDIR or S_IFREG set otherwise
     ///  together with the three access bits which under Windows only the write bit is relevant.
 
@@ -140,6 +156,10 @@ public:
     static  bool    isfile(const std::string& filename);
     ///< @returns true if the path is for an existing file
 
+    /// check if filename is a symlink
+    static  bool    islink(const std::string& filename);
+    ///< @returns true if the path is pointing at a symlink
+
     /// return a path to the temporary directory on the system
     static  const char * tmpdir();
 };