[Tcl-bugs] [ tcl-Bugs-1193497 ] file writable lies for certain XP directories

[SourceForge.net <no...@so...>]

Bugs item #1193497, was opened at 2005-05-01 23:26
Message generated for change (Comment added) made by fvogelnew1
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=110894&aid=1193497&group_id=10894

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: 36. File System
Group: development: 8.5a4
Status: Open
Resolution: None
>Priority: 9
Submitted By: fvogelnew1 (fvogelnew1)
Assigned to: Vince Darley (vincentdarley)
Summary: file writable lies for certain XP directories

Initial Comment:
Windows XP SP2 french, Tcl/Tk cvs HEAD

file writable "C:/Documents and 
Settings/<account>/Mes Documents/mydir"
returns 1 if mydir does exist, or 0 if not.
This is correct.

file writable "C:/Documents and Settings/"
returns also 1, which is correct.

But:
file writable "C:/Documents and 
Settings/<account>/Mes Documents"
returns 0 but it should be 1

<account> is my account name (with administrator 
rights), this directory of course exists and I'm allowed to 
write files in it.



----------------------------------------------------------------------

>Comment By: fvogelnew1 (fvogelnew1)
Date: 2006-03-07 20:22

Message:
Logged In: YES 
user_id=1245417

Following Don Porter's advice today about 8.4.13:

> Review, testing, and digging out of any remaining bugs
> that need to
> be fixed before release is invited.  Raise prio to 9.

Thanks,
Francois


----------------------------------------------------------------------

Comment By: Jason (jfg118)
Date: 2005-11-01 06:25

Message:
Logged In: YES 
user_id=1266631

Created duplicate bug 1344540

----------------------------------------------------------------------

Comment By: Don Porter (dgp)
Date: 2005-11-01 04:31

Message:
Logged In: YES 
user_id=80530


SF Trackers are fundamentally
broken on this point.  See 490389.

Only workaround is to open a new
Tracker report and attach to that.
Please do so.

And if you want to add your voice
of complaint to 490389, it likely won't
matter, but it might make us complainers
feel better. :)


----------------------------------------------------------------------

Comment By: Jason (jfg118)
Date: 2005-11-01 01:25

Message:
Logged In: YES 
user_id=1266631

I do not have the option to upload an attachment.  Not sure 
why, perhaps because I did not file the bug?  On bugs I file I 
do have the option to attach a file.

----------------------------------------------------------------------

Comment By: Vince Darley (vincentdarley)
Date: 2005-11-01 00:38

Message:
Logged In: YES 
user_id=32170

Can you attach your patch as an _attachment_ rather than
pasting it into the comments field (where it is generally
garbled by wrapping, sadly)?

thanks!


----------------------------------------------------------------------

Comment By: Jason (jfg118)
Date: 2005-10-22 01:24

Message:
Logged In: YES 
user_id=1266631

Below is a patch which implements the unicode call since 
Tcl seems to use unicode for WinNT by default.  It _corrects_ 
the functionality of the AccessCheck and removes the buffer 
on the stack.  The source is based on the Tcl 8.4.11 source 
and encompasses the changes by dkf. The patch should still 
be easily applied on the CVS HEAD.

The patch is generated with diff -u (3 files)

--- ../src/tcl8.4.11/win/tclWinInt.h	2004-05-03 
11:01:36.000000000 -0700
+++ win/tclWinInt.h	2005-10-21 11:20:21.390410900 -0700
@@ -76,6 +76,8 @@
     BOOL (WINAPI *getComputerNameProc)(WCHAR *, 
LPDWORD);
     DWORD (WINAPI *getCurrentDirectoryProc)(DWORD, 
WCHAR *);
     DWORD (WINAPI *getFileAttributesProc)(CONST TCHAR 
*);
+    BOOL (WINAPI *getFileSecurityProc)(CONST TCHAR *, 
SECURITY_INFORMATION, 
+            PSECURITY_DESCRIPTOR, DWORD, 
LPDWORD);
     DWORD (WINAPI *getFullPathNameProc)(CONST 
TCHAR *, DWORD nBufferLength, 
 	    WCHAR *, TCHAR **);
     DWORD (WINAPI *getModuleFileNameProc)(HMODULE, 
WCHAR *, int);
--- ../src/tcl8.4.11/win/tclWin32Dll.c	2005-06-06 
14:04:46.000000000 -0700
+++ win/tclWin32Dll.c	2005-10-21 
11:20:25.075710100 -0700
@@ -89,6 +89,8 @@
     (BOOL (WINAPI *)(WCHAR *, LPDWORD)) 
GetComputerNameA,
     (DWORD (WINAPI *)(DWORD, WCHAR *)) 
GetCurrentDirectoryA,
     (DWORD (WINAPI *)(CONST TCHAR *)) 
GetFileAttributesA,
+    (BOOL (WINAPI *)(CONST TCHAR *, 
SECURITY_INFORMATION, 
+            PSECURITY_DESCRIPTOR, DWORD, 
LPDWORD)) GetFileSecurityA,
     (DWORD (WINAPI *)(CONST TCHAR *, DWORD 
nBufferLength, WCHAR *, 
 	    TCHAR **)) GetFullPathNameA,
     (DWORD (WINAPI *)(HMODULE, WCHAR *, int)) 
GetModuleFileNameA,
@@ -138,6 +140,8 @@
     (BOOL (WINAPI *)(WCHAR *, LPDWORD)) 
GetComputerNameW,
     (DWORD (WINAPI *)(DWORD, WCHAR *)) 
GetCurrentDirectoryW,
     (DWORD (WINAPI *)(CONST TCHAR *)) 
GetFileAttributesW,
+    (BOOL (WINAPI *)(CONST TCHAR *, 
SECURITY_INFORMATION, 
+            PSECURITY_DESCRIPTOR, DWORD, 
LPDWORD)) GetFileSecurityW,
     (DWORD (WINAPI *)(CONST TCHAR *, DWORD 
nBufferLength, WCHAR *, 
 	    TCHAR **)) GetFullPathNameW,
     (DWORD (WINAPI *)(HMODULE, WCHAR *, int)) 
GetModuleFileNameW,
--- ../src/tcl8.4.11/win/tclWinFile.c	2005-06-22 
14:23:32.000000000 -0700
+++ win/tclWinFile.c	2005-10-21 16:06:21.584662400 -0700
@@ -171,6 +171,7 @@
 static int NativeStat(CONST TCHAR *path, Tcl_StatBuf 
*statPtr, int checkLinks);
 static unsigned short NativeStatMode(DWORD attr, int 
checkLinks, int isExec);
 static int NativeIsExec(CONST TCHAR *path);
+static int NativeIsWritable(CONST TCHAR *path);
 static int NativeReadReparse(CONST TCHAR* 
LinkDirectory, 
 			     
REPARSE_DATA_BUFFER* buffer);
 static int NativeWriteReparse(CONST TCHAR* 
LinkDirectory, 
@@ -1333,7 +1334,7 @@
 	return -1;
     }
 
-    if ((mode & W_OK) && (attr & 
FILE_ATTRIBUTE_READONLY)) {
+    if ((mode & W_OK) && !NativeIsWritable(nativePath)) {
 	/*
 	 * File is not writable.
 	 */
@@ -1360,6 +1361,102 @@
     return 0;
 }
 
+ /*
+  *----------------------------------------------------------------------
+  *
+  * NativeIsWritable --
+  *
+  *	Determine if a path is writable. On Windows this 
requires more than a
+  *	simple read only flag check for directories 
because Windows uses this
+  *	as a flag to customize the folder options. [Bug 
1193497]
+  *
+  * Results:
+  *	1 = writable, 0 = not.
+  *
+  * Side effects:
+  *	May smash the last error value (errors are just 
reported by this
+  *	function as non-writable files).
+  *
+  *----------------------------------------------------------------------
+  */
+static int
+NativeIsWritable(
+    CONST TCHAR *nativePath)
+{
+    BYTE *secDescPtr = 0;
+    DWORD secDescLen = 0;
+    HANDLE hToken = NULL;
+    BOOL isWritable = FALSE;
+
+#define FILE_SEC_INFO_BITS \
+	OWNER_SECURITY_INFORMATION \
+	| GROUP_SECURITY_INFORMATION \
+	| DACL_SECURITY_INFORMATION
+
+    /*
+     * Read the security descriptor for the file or directory. 
Note the
+     * first call obtains the size of the security descriptor.
+     */
+
+    if (!tclWinProcs->getFileSecurityProc(nativePath, 
FILE_SEC_INFO_BITS,
+	NULL, 0, &secDescLen)) {
+        if (GetLastError() == 
ERROR_INSUFFICIENT_BUFFER) {
+            DWORD secDescLen2 = 0;
+	    secDescPtr = (BYTE *) ckalloc(secDescLen);
+	    if (!tclWinProcs->getFileSecurityProc
(nativePath, FILE_SEC_INFO_BITS,
+	        secDescPtr, secDescLen, &secDescLen2) || 
(secDescLen < secDescLen2)) {
+	        goto done;
+            }
+        } else {
+	    goto done;
+        }
+    }
+
+    /*
+     * Get the security identity of the process and (if 
successful) use it
+     * with the security descriptor of the file to find out if the 
file really
+     * is writable.
+     */
+
+    if (OpenProcessToken(GetCurrentProcess(), 
TOKEN_DUPLICATE, &hToken)) {
+	HANDLE hIToken = NULL;
+	BOOL status = DuplicateToken(hToken, 
SecurityIdentification, &hIToken);
+
+	CloseHandle(hToken);
+	if (status) {
+	    PRIVILEGE_SET privilegeSet;
+            DWORD privilegeSetLength = sizeof(privilegeSet);
+	    DWORD granted; /* unused */
+            DWORD accessDesired = ACCESS_WRITE;
+
+            /* Initialize generic mapping structure to map all. */
+            GENERIC_MAPPING GenericMapping;
+            memset(&GenericMapping, 0xff, sizeof
(GENERIC_MAPPING));
+            GenericMapping.GenericRead = ACCESS_READ;
+            GenericMapping.GenericWrite = ACCESS_WRITE;
+            GenericMapping.GenericExecute = 0;
+            GenericMapping.GenericAll = ACCESS_READ | 
ACCESS_WRITE;
+            MapGenericMask(&accessDesired, 
&GenericMapping);
+
+	    AccessCheck(secDescPtr, hIToken, 
accessDesired,
+                        &GenericMapping, &privilegeSet, 
&privilegeSetLength, 
+                        &granted, &isWritable);
+	    CloseHandle(hIToken);
+	}
+    }
+
+    /*
+     * Release any temporary space we may be using and 
convert the writability
+     * value into a boolean for return.
+     */
+
+  done:
+    if (secDescPtr) {
+	ckfree(secDescPtr);
+    }
+    return isWritable != 0;
+}
+
 /*
  *----------------------------------------------------------------------
  *


----------------------------------------------------------------------

Comment By: Donal K. Fellows (dkf)
Date: 2005-10-21 10:48

Message:
Logged In: YES 
user_id=79902

Please find attached a patch file that implements the same
changes that Jason suggests, but recut as a UniDiff against
the HEAD (UniDiffs, created with the -u option to cvs diff,
are normally by far the easiest sorts of patches to apply
and review). The changes should apply equally well against
8.4, but I've not checked. I've got the formatting to follow
the core standards, and I've added a few comments to
indicate what *I* think is going on.

I do not know if the patch is correct (I *cannot* review it)
and I wonder whether using 1024 for the statuc buffer size
(as the original patch did) is a bit much. After all, stack
space is comparatively valuable.

----------------------------------------------------------------------

Comment By: Jason (jfg118)
Date: 2005-10-21 01:10

Message:
Logged In: YES 
user_id=1266631

missing lines of the patch in the previous comment :

173a174
> static int NativeIsWritable(CONST TCHAR *path);
1336c1337


----------------------------------------------------------------------

Comment By: Jason (jfg118)
Date: 2005-10-21 01:08

Message:
Logged In: YES 
user_id=1266631

Below is a patch generated against the Tcl 8.4.11 source.
The specific file is win/tclWinFile.c rev 1.44.2.12 .  The
patch implements and "NativeIsWritable" function.  The
function needs to have the wide char implementation added.
The only call which needs to be updated is the
GetFileSecurity function which needs an entry in the
tclWinProcs defnition.  The patch falls back to the old
implementation for wide char paths. 

<     if ((mode & W_OK) && (attr & FILE_ATTRIBUTE_READONLY)) {
---
>     if ((mode & W_OK) && !NativeIsWritable(nativePath)) {
1365a1367,1442
>  * NativeIsWritable --
>  *
>  *	Determines if a path is writable.  On windows this 
>  *	requires more than a simple read only flag check for
directories
>  *      because windows uses this as a flag to customize
the folder
>  *      options.  BUG 1193497
>  *
>  * Results:
>  *	1 = writable, 0 = not.
>  *
> 
*----------------------------------------------------------------------
>  */
> static int
> NativeIsWritable(nativePath)
>     CONST TCHAR *nativePath;
> {
>   char psd_buf[1024], *psd_ptr = psd_buf, *psd_alloc = 0;
>   DWORD sd_rlen;
>   HANDLE hToken = NULL;
>   int retVal = 0;
> 
>   if (tclWinProcs->useWide) {
>     /* not implemented  -- fall back to old implementation */
>     return
((*tclWinProcs->getFileAttributesProc)(nativePath) &
FILE_ATTRIBUTE_READONLY) ? 0 : 1;
>   }
> 
>   if (!GetFileSecurity(nativePath,
>                        OWNER_SECURITY_INFORMATION
>                        | GROUP_SECURITY_INFORMATION
>                        | DACL_SECURITY_INFORMATION,
>                        psd_buf, sizeof(psd_buf), &sd_rlen)) {
>     return 0; /* Failed ... */
>   }
>   if (sizeof(psd_buf) < sd_rlen) {
>     DWORD psd_alloc_rlen;
>     psd_alloc = (char *)ckalloc(sd_rlen);
>     if (!GetFileSecurity(nativePath,
>                          OWNER_SECURITY_INFORMATION
>                          | GROUP_SECURITY_INFORMATION
>                          | DACL_SECURITY_INFORMATION,
>                          psd_alloc, sd_rlen, &psd_alloc_rlen)
>         || sd_rlen  < psd_alloc_rlen) {
>       ckfree(psd_alloc);
>       return 0; /* Failed .... */
>     }
>     psd_ptr = psd_alloc;
>     sd_rlen = psd_alloc_rlen;
>   }
>   if ( OpenProcessToken ( GetCurrentProcess ( ),
TOKEN_DUPLICATE, &hToken ) ) {
>     HANDLE hIToken = NULL;
>     BOOL status = DuplicateToken (hToken,
SecurityIdentification, &hIToken);
>     CloseHandle (hToken);
>     
>     if (status) {
>       GENERIC_MAPPING mapping = { FILE_GENERIC_READ,
>                                   FILE_GENERIC_WRITE,
>                                   FILE_GENERIC_EXECUTE,
>                                   FILE_ALL_ACCESS };
>       char pbuf[sizeof (PRIVILEGE_SET) + 3 * sizeof
(LUID_AND_ATTRIBUTES)];
>       DWORD plength = sizeof(pbuf), granted; 
>       retVal = AccessCheck (psd_ptr, hIToken,
FILE_WRITE_DATA, &mapping,
>                             (PPRIVILEGE_SET) pbuf,
&plength, &granted, &status) ? 1 : 0;
>       
>       CloseHandle(hIToken);
>     }
>   }
> 
>   if (psd_alloc)
>     ckfree(psd_alloc);
> 
>   return retVal;
> }
> /*
> 
*----------------------------------------------------------------------
>  *


----------------------------------------------------------------------

Comment By: fvogelnew1 (fvogelnew1)
Date: 2005-10-05 21:47

Message:
Logged In: YES 
user_id=1245417

I would be very thankful if somebody could derive a proper 
patch from the code below.
I already had a deep look at this code, but I have to admit 
that I'm not proficient enough to do it myself, sorry for that.


----------------------------------------------------------------------

Comment By: Jeffrey Hobbs (hobbs)
Date: 2005-10-05 21:20

Message:
Logged In: YES 
user_id=72656

There is some demo code provided below.  If someone wants to
turn that into a patch for the core, we can try it out.

----------------------------------------------------------------------

Comment By: fvogelnew1 (fvogelnew1)
Date: 2005-10-05 15:00

Message:
Logged In: YES 
user_id=1245417

Hi all,

Anything new wrt this bug?
Is anybody planning to fix it?

I really need [file writable] to work correctly. My scripts are 
currently not portable on Windows because of this bug.

Thanks.


----------------------------------------------------------------------

Comment By: Don Porter (dgp)
Date: 2005-08-23 23:10

Message:
Logged In: YES 
user_id=80530

dgp	visitor is upset about [file writable]'s lies on windows.
[17:05]	dgp	1193497, I think
[17:06]	dgp	he tracked it down to a fundamental difference
in meaning of permission bits on the two systems.
[17:07]	dgp	On unix, when a user lacks write permission to a
directory, the user cannot create/delete files in that
directory.
[17:07]	dgp	On Windows, when a users lacks write permission
to a directory, the user cannot delete the directory itself,
but may create/delete files within it as much as they like.

I would prefer to have [file writable] give
consistent information cross-platform, so
I can write portable scripts with it.  Wipe
out OS Differences!

----------------------------------------------------------------------

Comment By: Vince Darley (vincentdarley)
Date: 2005-06-09 18:28

Message:
Logged In: YES 
user_id=32170

Can you suggest and test a patch to tclWinFile.c which
resolves this for you?


----------------------------------------------------------------------

Comment By: Jason (jfg118)
Date: 2005-05-04 01:09

Message:
Logged In: YES 
user_id=1266631

Something like this would check the ACL list for writable
permission, written in C++ though.

bool is_writable(const char *path) {

    char psd_buf[1024], *psd_ptr = psd_buf, *psd_alloc = 0;
    DWORD sd_rlen;

    if (!GetFileSecurity (path,
                          OWNER_SECURITY_INFORMATION
                          | GROUP_SECURITY_INFORMATION
                          | DACL_SECURITY_INFORMATION,
                          psd_buf, sizeof(psd_buf), &sd_rlen)) {
        return false; // Failed ...
    }
    if (sizeof(psd_buf) < sd_rlen) {
        psd_alloc = new char[sd_rlen];
        DWORD psd_alloc_rlen;
        if (!GetFileSecurity (path,
                              OWNER_SECURITY_INFORMATION
                              | GROUP_SECURITY_INFORMATION
                              | DACL_SECURITY_INFORMATION,
                              psd_alloc, sd_rlen,
&psd_alloc_rlen)
            || sd_rlen  < psd_alloc_rlen) {
            delete [] psd_alloc;
            return false; // Failed ....
        }
        psd_ptr = psd_alloc;
        sd_rlen = psd_alloc_rlen;
    }

    HANDLE hToken = NULL, hIToken = NULL;
    if ( !OpenProcessToken ( GetCurrentProcess ( ),
TOKEN_DUPLICATE, &hToken ) ) {
        if (psd_alloc)
            delete [] psd_alloc;
        return false;
    }
    BOOL status = DuplicateToken (hToken,
SecurityIdentification, &hIToken);
    CloseHandle (hToken);

    if (! status) {
        if (psd_alloc)
            delete [] psd_alloc;
        return false;
    }

    GENERIC_MAPPING mapping = { FILE_GENERIC_READ,
                                FILE_GENERIC_WRITE,
                                FILE_GENERIC_EXECUTE,
                                FILE_ALL_ACCESS };
    char pbuf[sizeof (PRIVILEGE_SET) + 3 * sizeof
(LUID_AND_ATTRIBUTES)];
    DWORD plength = sizeof(pbuf), granted;
    if (AccessCheck (psd_ptr, hIToken, FILE_WRITE_DATA,
&mapping,
                     (PPRIVILEGE_SET) pbuf, &plength,
&granted, &status))
        retVal = true;

    CloseHandle(hIToken);

    if (psd_alloc)
        delete [] psd_alloc;

    return true;
}


----------------------------------------------------------------------

Comment By: Jason (jfg118)
Date: 2005-05-04 01:05

Message:
Logged In: YES 
user_id=1266631

Clearly from the MSDN document it states that the "Read-only
attribute" does not mean that the directory is not writable,
but instead means it is a "special customized folder". The
writeability is determined by the ACL list.

From MSDN : CAUSE
Unlike the Read-only attribute for a file, the Read-only
attribute for a folder is typically ignored by Windows,
Windows components and accessories, and other programs. For
example, you can delete, rename, and change a folder with
the Read-only attribute by using Windows Explorer. The
Read-only and System attributes is only used by Windows
Explorer to determine whether the folder is a special
folder, such as a system folder that has its view customized
by Windows (for example, My Documents, Favorites, Fonts,
Downloaded Program Files), or a folder that you customized
by using the Customize tab of the folder's Properties dialog
box.

----------------------------------------------------------------------

Comment By: fvogelnew1 (fvogelnew1)
Date: 2005-05-03 00:11

Message:
Logged In: YES 
user_id=1245417

The fact that the read only box cannot be unchecked using 
the Explorer is a Windows by-design item, yes.

But all the directories of my report have this attribute set. 
Nevertheless, [file writable] returns 0 or 1 depending on which 
directory it checks. Moreover, I *can* write files in all of them.

IMO this means that the way tcl implements the file writable 
command should be revised to return 1 for all of them.

Otherwise, how can I check if a directory is writable or not?

In other words, there should be more/other checks than the 
read only attribute (I know there is already more, see bug 
749876, but this is apparently not enough).


----------------------------------------------------------------------

Comment By: Jeffrey Hobbs (hobbs)
Date: 2005-05-02 23:55

Message:
Logged In: YES 
user_id=72656

Which would imply that this is a Windows by-design item.

----------------------------------------------------------------------

Comment By: fvogelnew1 (fvogelnew1)
Date: 2005-05-02 22:52

Message:
Logged In: YES 
user_id=1245417

And apparently this is by design of Windows. See 
http://support.microsoft.com/kb/326549/en-us


----------------------------------------------------------------------

Comment By: fvogelnew1 (fvogelnew1)
Date: 2005-05-02 22:46

Message:
Logged In: YES 
user_id=1245417

And I can't remove the read only attribute using Explorer. It 
let me do it, but if I reopen the Properties box, it is here again.


----------------------------------------------------------------------

Comment By: fvogelnew1 (fvogelnew1)
Date: 2005-05-02 22:25

Message:
Logged In: YES 
user_id=1245417

All the directories I mention in my report show the read only 
attribute if I look at their properties using Explorer, 
even "mydir" which I created manually in "Mes Documents".


----------------------------------------------------------------------

Comment By: Jeffrey Hobbs (hobbs)
Date: 2005-05-02 22:07

Message:
Logged In: YES 
user_id=72656

While you can actually write to that directory, I think
Windows handles it specially.  If I do through Explorer and
do Properties on my "My Documents" dir, it does clearly have
the "Read only" box checked, which is correctly reported by
Tcl as well.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=110894&aid=1193497&group_id=10894