Windows File Handling: International characters break file handling, permissions don't translate

Checking the operating system when you really have to make assumptions about the file system is not ideal, but unfortunately I didn't find a way to check the file system directly. This may be the best you can do.

I'm a bit rusty on bit-wise operations, but I hope that this change only relaxes the constraints. NTFS may be the default, but you can also use FAT and even (with added drivers) EXT2 on Windows. The check should still pass in that case.

(There's also the case of using an NTFS volume on Linux, which could be possible on dual-booting machines. But I think we don't need to cover all the edge cases.)

Seems sane to me. I don't have a Windows box to test this on though. And I really do like the change from 511 to 0777... not sure why I did the decimal version.

meant to bump the status as well.

Other than perhaps in some book somewhere, I've never in my life seen the |= operator, nor the >> operator. We need way, way more comments here to explain what is going on. And a summary would be nice as well for people who don't care about the details. Like, "This forces the normal Unix permissions to Windows's read/write equivalents" or "This drops the group and other bits from the permission, as the user is the only important thing" or whatever it's doing.

The comments atm tell me some of what, but not enough, and not any of the why. :)

I do like the consistency of using 0777 though. So +1 to that!

ah, yeah that's a lot more readable. if we can get this tested on a windows box it looks rtbc.

+++ modules/simpletest/tests/file.test	10 Jul 2009 20:23:46 -0000
@@ -99,19 +99,32 @@ class FileTestCase extends DrupalWebTest
     // Mask out all but the last three octets.
-    $actual_mode = fileperms($filepath) & 511;
+    $actual_mode = fileperms($filepath) & 0777;

The comment looks outdated now?

I'm on Windows, I can test.

I'm on crack. Are you, too?

sun, you are on crack... that or you need to review the documentation for integers ;)

0777 == 511. the leading 0 causes php to interpret the number as octal, just like a leading 0x causes it to be interpreted as hexadecimal.

Marked #315520: assertFilePermissions() not working on Windows as a duplicate of this one.

Marked #753250: Simpletest file permission exceptions on Windows test platform as a duplicate (does contain some additional info)

Raising to critical, as file handling is severely broken on Windows + IIS.

Here is a reroll, with a few additions:

• On IIS 7.x, the FastCGI implementation (which is the preferred way of deploying PHP on Windows) mangles the REQUEST_URI variable, and we have to use UNENCODED_URL if available
• The rewriting rule in web.config was not consistent with the new one we have in .htaccess.
• By default, IIS doesn't serve files without an extension (and we are using a lot of them in the test), so add a "." (meaning: "no extension") mime mapping in web.config.
• IIS recognizes "." as the extension separator, so we cannot test for it.
• By default the "Request filtering" handler blocks requests that are double-encoded. It is a pretty much useless security mechanism, so disable it.

This makes the File tests (and related tests) pass on IIS. At last. We really need a test environment for Windows + IIS.

Needs work anyway because:

- The idea of storing UTF-8 data over another encoding is just a hack (and this "other encoding" is not necessarily Windows-1252, it can be any encoding depending of the server configuration).
- The current patch provides no migration path for existing files.
- Changing the external URLs of files to match our lame way of storing them is just... not elegant.

The root of the problem is known: PHP on Windows doesn't use the proper API to access the files. It uses the legacy one instead of the Unicode version. I'm currently investigating if it would be possible to workaround that using a custom stream wrapper.

@c960657: the problem is not Windows, it's PHP. Windows stores filenames in UTF-16 (on NTFS), but PHP uses the old (locale-aware) API to access the filesystem, instead of the newer Unicode API. That's where the actual encoding conversion takes place.

Given that there is no clear way forward for the support of Unicode files on Windows, I suggest we first focus on fixing the issue regarding the clean-up of directories containing read-only files. This issue is simple to fix, and make a lot of unrelated tests fail (for example: "Import feeds from OPML functionality" in the test suite of the Aggregator module).

This (somewhat simple) patch should already make the situation a lot better. Let's get it in.

The same patch minus the changes to web.config, that I don't want there.

#26: 443286-step-1.patch queued for re-testing.

Rerolling #26

+++ includes/file.inc	25 Jun 2010 12:36:50 -0000
@@ -1920,6 +1955,37 @@ function drupal_mkdir($uri, $mode = NULL
+  if ((!$scheme || !file_stream_wrapper_valid_scheme($scheme))
+    && (substr(PHP_OS, 0, 3) == 'WIN')) {
+    chmod($uri, 0700);
+  }

There's no reason I understand to do an "if Windows" in the delete cases. The logic is the same. Why can't we just chmod whether or not we're on Windows and simplify the code?

The comments need a link to the PHP issue.

We need a test for this so that it doesn't come back to haunt us (assuming we get testing going on Win). I think the big risk here is that we'll rip this out later because we just don't understand it.

It's very hard to like this at all. It's an API change. It's intrusive. It's hard to let people know about. It's hard to maintain. We're changing core code and adding an API to support an obscure Windows PHP issue that nobody will understand down the line.

Changing the title, as this is *not* critical if this is just about tests on Windows. It probably *is* critical if it's about all file handling on Windows. But if it's just about tests, let's downgrade it.

Powered by Dreditor.

I just took a quick look at the issues marked as dups in #17, 18, and 19.

We need a clear statement of what we're going to address in this issue, and the rest needs to be put in another issue. So many things have been marked as dups and they'll be lost if we don't get this straightened out. I see at least:

• Filenames with UTF filenames fail: (#238299-15: file_create_url should return valid URLs and #701500: Cannot download files with 8bit filenames on Windows)
• Permissions don't translate correctly to Unix ugo octets, causing:
  • directory deletion failure
  • test failure as a result

  If we're going to try to address a list of issues, let's get them listed carefully, with an example of how to demonstrate each so we can do decent tests.

Just clarifying a little bit more:

• There is a directory deletion failure due to the fact that is_writable(), chmod() and unlink() are not consistent with one another; we can and probably should fix that (there is probably an easier fix then mine)
• There is the Unicode file issue, this one we cannot fix. It goes deep into how PHP uses the filesystem API on Windows. There is a fix in the works on the PHP side (including an extension that will be usable on previous versions of PHP), but it will not be available before the end of the year. Until that, I would recommend not using the standard public/private files on Windows.

looking at #29

+++ includes/file.inc	25 Jun 2010 12:36:50 -0000
@@ -913,6 +913,10 @@ function file_create_filename($basename,
+  if (substr(PHP_OS, 0, 3) == 'WIN') {
+    // These characters are not allowed in Windows filenames
+    $basename = str_replace(array(':', '*', '?', '"', '<', '>', '|'), '_', $basename);
+  }

We might want to have a link to this inside the code comments?
http://en.wikipedia.org/wiki/Filename#Reserved_characters_and_words
I have a similar function in the boost module so I don't try to create bad filenames on windows. If this was in core, it would probably be a nice thing; otherwise we will be shipping something I can not rely on as a module maintainer and will thus have to create my own version of the same function.

+++ modules/simpletest/tests/file.test	25 Jun 2010 12:36:51 -0000
@@ -108,7 +108,20 @@ class FileTestCase extends DrupalWebTest
+    // PHP on Windows has limited support for file permissions. Usually each of
+    // "user", "group" and "other" use one octal digit (3 bits) to represent the
+    // read/write/execute bits. On Windows, chmod() ignores the "group" and ¶
+    // "other" bits, and fileperms() returns the "user" bits in all three
+    // positions. $expected_mode is updated to reflect this.
+    if (substr(PHP_OS, 0, 3) == 'WIN') {
+      // Reset the "group" and "other" bits.
+      $expected_mode = $expected_mode & 0700;
+      // Shift the "user" bits to the "group" and "other" positions also.
+      $expected_mode = $expected_mode | $expected_mode >> 3 | $expected_mode >> 6;
+    }

@@ -130,7 +143,20 @@ class FileTestCase extends DrupalWebTest
+    // PHP on Windows has limited support for file permissions. Usually each of
+    // "user", "group" and "other" use one octal digit (3 bits) to represent the
+    // read/write/execute bits. On Windows, chmod() ignores the "group" and ¶
+    // "other" bits, and fileperms() returns the "user" bits in all three
+    // positions. $expected_mode is updated to reflect this.
+    if (substr(PHP_OS, 0, 3) == 'WIN') {
+      // Reset the "group" and "other" bits.
+      $expected_mode = $expected_mode & 0700;
+      // Shift the "user" bits to the "group" and "other" positions also.
+      $expected_mode = $expected_mode | $expected_mode >> 3 | $expected_mode >> 6;
+    }

make this into a function since it is used in more then 1 place

Powered by Dreditor.

@rfay

There's no reason I understand to do an "if Windows" in the delete cases. The logic is the same. Why can't we just chmod whether or not we're on Windows and simplify the code?

This would be for performance reasons, if we can avoid hitting the filesystem then that is a major plus. Always chmod-ing a file before deletion will make it slower. Taking the idea from #818818: Race Condition when using file_save_data FILE_EXISTS_REPLACE we could attempt the delete and if it fails then chmod and attempt the delete again; like this

function drupal_unlink($uri, $context = NULL) {
  $scheme = file_uri_scheme($uri);
  
  if ($context) {
    $success = @unlink($uri, $context);
  }
  else {
    $success = @unlink($uri);
  }
  if (!$success && (!$scheme || !file_stream_wrapper_valid_scheme($scheme))) {
    chmod($uri, 0600);
    if ($context) {
      $success = @unlink($uri, $context);
    }
    else {
      $success = @unlink($uri);
    }
  }
  return $success;
}

Is there anyway we can get rid of the if ($context) in this function? Makes the code harder to read. http://php.net/unlink

For windows only 0200 mask has meaning (readable) so chmod(0200, $file) is enough to make it writable

About unicode - I have to use transliteration module on every site where users can upload any files because russian letters 'Ё' 'ё' 'я' are not supported on linux. So only possibles way to work with user files are urlencode() or transliterate them.

Related issue about permissions #857300: Make drupal_install_mkdir should use octal numbers

#29: 443286-step-1_0-reroll.patch queued for re-testing.

This patch doesn't apply cleanly to the latest HEAD revision.

Re-roll attached. I am a big fan of the addition of drupal_unlink - It introduces lower level file handling to the API for modules that may want to make use of unlink instead of file_delete.

I can also confirm this does, in fact, work on IIS7 (Windows 7, 64-Bit) thought it would be nice to get some older builds of IIS in the mix here.

This wasn't reviewed / RTBC yet, as far as I can see.

@sun, Thanks. You are right. Still needs review. My tests pass but others need to have a peek.

I hate the fact that we need to work around OS-difference. This is a PHP bug, IMO.

I don't have a Windows machine so I can't really test it but webkenny (who is sitting next to me) ran the tests on Windows.

As far as I can tell this is ready. Painful but ready. I won't commit it yet so we leave some more time for reviews.

+++ includes/file.inc	17 Aug 2010 14:53:58 -0000
@@ -1893,6 +1898,37 @@ function drupal_chmod($uri, $mode = NULL
+  if ((!$scheme || !file_stream_wrapper_valid_scheme($scheme))
+    && (substr(PHP_OS, 0, 3) == 'WIN')) {

@@ -2002,6 +2037,37 @@ function drupal_mkdir($uri, $mode = NULL
+  if ((!$scheme || !file_stream_wrapper_valid_scheme($scheme))
+    && (substr(PHP_OS, 0, 3) == 'WIN')) {

Strange wrapping. Can we write this on one line, please?

+++ modules/simpletest/tests/file.test	17 Aug 2010 14:53:59 -0000
@@ -108,7 +108,20 @@ class FileTestCase extends DrupalWebTest
+    // read/write/execute bits. On Windows, chmod() ignores the "group" and ¶

@@ -130,7 +143,20 @@ class FileTestCase extends DrupalWebTest
+    // read/write/execute bits. On Windows, chmod() ignores the "group" and ¶

Trailing white-space.

+++ modules/simpletest/tests/file.test	17 Aug 2010 14:53:59 -0000
@@ -866,19 +892,26 @@ class FileDirectoryTest extends FileTest
+      // Make directory read only.
+      @chmod($directory, 0444);

Should be drupal_chmod(), no?

+++ modules/system/system.tar.inc	17 Aug 2010 14:54:00 -0000
@@ -210,7 +210,7 @@ class Archive_Tar // extends PEAR
-            @unlink($this->_temp_tarname);
+            @drupal_unlink($this->_temp_tarname);

Do we have to alter the Archive_Tar library?

Powered by Dreditor.

As I pointed above ONLY 0200 (readable) make sense for WIN, 0400 useless because actually no changes happen in filesystem

rerolling with changes suggested in #45 except:

Do we have to alter the Archive_Tar library?

We would not have to if it had it's own work around for WIN. What do you suggest instead, no WIN support? Per #870204: Revert coding style changes to system.tar.inc & other externally developed files, this would fall under "Only keep changes required for Drupal integration.". This should be fixed upstream in the tar library.

Committed to CVS HEAD. Great! :)

Actually, we might want to revert that. Because both rmdir() and unlink() are in the stream wrapper interface, we can do much better then this.

This patch (untested) might allow us to remove drupal_unlink() and drupal_rmdir().

there is also the filesystem permission to address in #44 which remains to be addressed

Not sure the proposed alternative is 'much better'. It certainly is something we want to evaluate more (and in the mean time, the critical bug has been addressed).

This approach is better because we don't have to rely on everyone explicitely calling drupal_unlink() and drupal_rmdir(). If you think this is a hack, it certainly is. But it seems to work alright.

Also, with this framework we can fix other crazy issues with filesystem on Windows, e.g.: #874172: is_writable() cannot be use to determine if a directory is writable (on Windows).

Yep, I'm on board with all of that. As I wrote: let's continue to explore your option.

I'm demoting this issue because it is no longer critical. The actual bug was fixed, the proposal in #48 is further clean-up. :-)

+

Demoting to normal. Might still be nice to that follow-up clean-up, but as of now, there doesn't seem to be a real problem in HEAD.

Lnked issue #278425: Using basename() is not locale safe