make_permissions is case-inconsistent and requires unnecessary conversions

[daniel.kruegler@…]

There exist two notable issues with the internal function make_permissions in operations.cpp (Windows only):

a) The usage of the macro BOOST_FILESYSTEM_STRICMP implies case-insensitive comparison, but for compilers different from _MSC_VER it actually calls the case-sensitive function std::strcmp.

b) The code uses up to four code-conversions (wchar_t->char) to invoke the comparison function, all of the following form:

BOOST_FILESYSTEM_STRICMP(p.extension().string().c_str(), ".exe")

It seems that there exist a simple and consistent way to solve these problems: Replace the macro BOOST_FILESYSTEM_STRICMP by boost::algorithm::iequals and do not perform code conversion. Essentially the make_permissions code could be changed to the following form - assuming an additional inclusion of

#include "boost/algorithm/string/predicate.hpp"

and removal of the BOOST_FILESYSTEM_STRICMP macro definition (which is no-where else used):

  perms make_permissions(const path& p, DWORD attr)
  {
    perms prms = fs::owner_read | fs::group_read | fs::others_read;
    if  ((attr & FILE_ATTRIBUTE_READONLY) == 0)
      prms |= fs::owner_write | fs::group_write | fs::others_write;
    if (boost::algorithm::iequals(p.extension().c_str(), L".exe")
      || boost::algorithm::iequals(p.extension().c_str(), L".com")
      || boost::algorithm::iequals(p.extension().c_str(), L".bat")
      || boost::algorithm::iequals(p.extension().c_str(), L".cmd"))
      prms |= fs::owner_exe | fs::group_exe | fs::others_exe;
    return prms;
  }

Given that boost::algorithm::iequals allows to provide a std::locale object as additional argument, one could improve that solution even more by providing the same locale as that from path_locale() within path.cpp. Unfortunately there is no access within operations.cpp to that function (which is within an unnamed namespace), so realizing this second improvement would require a larger surgery.

[verybigbadboy@…]

Hello, I have extra note about (b)

I have issues with iteration of folder with a lot of files with chinese filenames.

one of those file has name similar to "123.是打盤子" it break directory iterator increment method :( thank you.

[Daniel Kruegler <daniel.kruegler@…>]

This issue recently became more problematic because of bug ticket #6320: Boost Filesystem really should try to prevent internally any code conversions, unless explicitly required by the user. Due to multi-threading issues with the code conversions (partly related boost, but unfortunately also related to the Standard library associated with the Visual Studio compiler), we try hard to prevent any code conversion when working with boost filesystem. Alas, this doesn't help us becoming victim of the code conversion issue, because the above mentioned make_permissions function does perform implicit code conversions through the usage of the string() member function of path.

[Daniel Kruegler <daniel.kruegler@…>]

Beman, I'm willing to provide a patch, but given the potential race conditions in regard to locales (see #6320), I'm now tending to suggest a less ambitious resolution: Instead of using boost::algorithm::iequals, as originally suggested, I now tend to keep the existing BOOST_FILESYSTEM_STRICMP, with the idea to just replace the function it points to:

// some distributions of mingw as early as GLIBCXX__ 20110325 have _wcsicmp, but the
// offical 4.6.2 release with __GLIBCXX__ 20111026  doesn't. Play it safe for now, and
// only use _wcsicmp if _MSC_VER is defined
#if defined(_MSC_VER) // || (defined(__GLIBCXX__) && __GLIBCXX__ >= 20110325)
#  define BOOST_FILESYSTEM_STRICMP _wcsicmp
#else
#  define BOOST_FILESYSTEM_STRICMP wcscmp
#endif

Given that we can replace make_permissions as follows:

  perms make_permissions(const path& p, DWORD attr)
  {
    perms prms = fs::owner_read | fs::group_read | fs::others_read;
    if  ((attr & FILE_ATTRIBUTE_READONLY) == 0)
      prms |= fs::owner_write | fs::group_write | fs::others_write;
    if (BOOST_FILESYSTEM_STRICMP(p.extension().c_str(), L".exe") == 0
      || BOOST_FILESYSTEM_STRICMP(p.extension().c_str(), L".com") == 0
      || BOOST_FILESYSTEM_STRICMP(p.extension().c_str(), L".bat") == 0
      || BOOST_FILESYSTEM_STRICMP(p.extension().c_str(), L".cmd") == 0)
      prms |= fs::owner_exe | fs::group_exe | fs::others_exe;
    return prms;
  }

How would you feel about that alternative?

[Daniel Kruegler <daniel.kruegler@…>]

Patch suggestion

[Daniel Kruegler <daniel.kruegler@…>]

Please consider the attached patch. In addition to fixing the need of string conversions it considers one complaint pointed out in #9480 about four calls to evaluate the path extension, which is now replaced by a single call.

[daniel.kruegler@…]

Replying to Daniel Kruegler <daniel.kruegler@…>:

Please consider the attached patch. In addition to fixing the need of string conversions it considers one complaint pointed out in #9480 about four calls to evaluate the path extension, which is now replaced by a single call.

Based on that patch suggestion I have created a pull request via the github project:

​https://github.com/boostorg/filesystem/pull/41/commits/755766a0536df599ac0280428defad03dac92cfc

Please consider this request, it also solves the performance problem described in ticket #9480.

[daniel.kruegler@…]

This issue should be marked as resolved by accepting the same pull request as ticket #9480 for Boost version 1.64.