Boost C++ Libraries: Ticket #10731: make_permissions is case-inconsistent and requires unnecessary conversions

verybigbadboy@… — Sun, 08 May 2016 19:11:42 GMT

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.

cc set

raad@… — Mon, 27 Jun 2016 13:50:37 GMT

cc raad@… added

Fri, 09 Sep 2016 18:32:24 GMT

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.

Fri, 09 Sep 2016 19:07:08 GMT

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?

attachment set

Sat, 10 Sep 2016 11:59:08 GMT

attachment → filesystem_10731.patch

Patch suggestion

Sat, 10 Sep 2016 12:03:35 GMT

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@… — Tue, 14 Mar 2017 20:51:18 GMT

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@… — Sun, 30 Apr 2017 17:21:15 GMT

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