Tcl Source Code

View Ticket
Login
Ticket UUID: 9e6b569963ea497e9e623a79d5a5f92c5681bfdb
Title: file normalize ~user fails on Windows
Type: Bug Version: All
Submitter: fvogel Created on: 2018-05-15 14:04:51
Subsystem: 37. File System Assigned To: fvogel
Priority: 5 Medium Severity: Minor
Status: Closed Last Modified: 2018-07-10 19:07:39
Resolution: Fixed Closed By: sebres
    Closed on: 2018-07-10 19:07:39
Description:

file normalize ~$::tcl_platform(user)

returns:

- in tclsh86tg, normal user : user "fvogel" doesn't exist
- in tclsh86tg run as Admin : C:/Users/l-vogel

The computer is a Win 7 machine connected to a domain controller with an active directory.

This problem was first detected with the Tk test suite test winDialog-5.12.2.

Looking at the code and debugging:

~fvogel looks at the NetUserGetInfo in TclpGetUserHome in tclWinFile.c In the described situation this call returns 2221 (NERR_UserNotFound).

The following patch fixes the issue for me:

Index: win/tclWinFile.c
==================================================================
--- win/tclWinFile.c
+++ win/tclWinFile.c
@@ -1442,10 +1442,11 @@
     int nameLen = -1;
     int badDomain = 0;
     char *domain;
     WCHAR *wName, *wHomeDir, *wDomain, **wDomainPtr = &wDomain;
     WCHAR buf[MAX_PATH];
+    LPCWSTR wServername = NULL;

     Tcl_DStringInit(bufferPtr);
     wDomain = NULL;
     domain = strchr(name, '@');
     if (domain != NULL) {
@@ -1456,11 +1457,12 @@
        nameLen = domain - name;
     }
     if (badDomain == 0) {
        Tcl_DStringInit(&ds);
        wName = Tcl_UtfToUniCharDString(name, nameLen, &ds);
-       if (NetUserGetInfo(wDomain, wName, 1, (LPBYTE *) uiPtrPtr) == 0) {
+        NetGetDCName(NULL, wDomain, (LPBYTE *) &wServername);
+       if (NetUserGetInfo(wServername, wName, 1, (LPBYTE *) uiPtrPtr) == 0) {
            wHomeDir = uiPtr->usri1_home_dir;
            if ((wHomeDir != NULL) && (wHomeDir[0] != L'\0')) {
                Tcl_UniCharToUtfDString(wHomeDir, lstrlenW(wHomeDir),
                        bufferPtr);
            } else {

User Comments: sebres added on 2018-07-10 19:07:14:
as discussed with Francois, the branch bug-9e6b569963 has been closed now.

sebres added on 2018-05-28 14:06:08:

I've merged my branch to 8.5/8.6. @Francois and @Ashok, thanks for testing and review.

Additionally as suggested by Ashok the changes reintegrated back to bug-9e6b569963 (see [c471b6d83a]) to cover the case ~domain\user (TclpGetUserHome only), if someone needs it.

Note I still think it could be too dangerous.
So in this branch "bug-9e6b569963": the call "file normalize ~domain\user" splits the path before it calls TclpGetUserHome, so the syntax ~domain\user is impossible anyway. But "glob ~domain\user" does the tilde-substitution a bit differently, so it is affected by this branch (and IMHO confusion of "~domain\user" with "~user\path" takes place at the moment in this branch).

% file normalize ~$::tcl_platform(user)@$::env(USERDOMAIN)
//hspfil08.my-domain/home/sergey.brester
% file normalize ~$::env(USERDOMAIN)\\$::tcl_platform(user)
user "my-domain" doesn't exist

% glob ~$::tcl_platform(user)@$::env(USERDOMAIN) {//hspfil08.my-domain/home/sergey.brester} % glob ~$::env(USERDOMAIN)\\$::tcl_platform(user) {//hspfil08.my-domain/home/sergey.brester} % glob ~$::tcl_platform(user) {//hspfil08.my-domain/home/sergey.brester}

So the difference to current 8.6:

% # bug-9e6b569963
% glob ~$::env(USERDOMAIN)\\$::tcl_platform(user)
{//hspfil08.my-domain/home/sergey.brester}

% # core-8-6-branch % glob ~$::env(USERDOMAIN)\\$::tcl_platform(user) user "my-domain\sergey.brester" doesn't exist

% # both % file normalize ~$::env(USERDOMAIN)\\$::tcl_platform(user) user "my-domain" doesn't exist

Therefore, I don't recommend to use branch bug-9e6b569963 as is (possibly "DoTildeSubst" should be "fixed" in this or in any case).

Anyway the subject is closed in 8.5/8.6 so the ticket can be closed theoretically. But I'll set it to pending state (because of still existing branch bug-9e6b569963 and lack of clarity with "~domain\user" syntax and the tilde-substitution).


apnadkarni added on 2018-05-26 06:19:19:
@sebres
>Nope, it takes care about all cases excepting 2.

oops, I misspoke. I meant to say it takes care of 1b, not 1a (which is already working in tcl)

fvogel added on 2018-05-25 16:37:51:
Indeed you're right. Failure when disconnected is gone. Well done!

sebres added on 2018-05-25 15:15:45:
I've submitted newest variant with fast withdrawal for the current user home-path.

If user name specified without domain and equals the current user - it tries safest and fastest way to get current user-home path (entirely without usage of netapi).

This should work also without the network for domain-users.

@Francois: can you please test it again.
The filesystem-1.30.1 with {file normalize ~$::tcl_platform(user)} should work for domain-user in any case now (also if disconnected);

sebres added on 2018-05-25 13:11:52:
> [sebres-bug-9e6b569963-8-5-branch] takes care of 1a and 3 but not 2.

Nope, it takes care about all cases excepting 2.
The cases 1a and 1b are both covered with the higher priority for local user
(so backwards-compatible), then try 1a (and if not found try 1b).

sebres added on 2018-05-25 13:06:10:

@Ashok:

> it would be clearer to me at least if the single while loop was broken out explicitly rather than setting domain = -1.

Well, it is "impossible" to break without unfolding of the cycle (because "domain = -1" is just a mark for a post-condition, so read it as "try again but once, leave the cycle later if domain becomes not NULL").

Of course one could rewrite it with unfolding the cycle, but the code-base grows then and IMHO it was clear enough (anyway I gave extra a comment nearby the setting of domain to -1 to explain it).


sebres added on 2018-05-25 12:53:39:

However when disconnected from the network then fileSystem-1.30.1 fails.

This could have 2 reasons:

  • either the PDC cannot be accessed and not cached (so user-home could not be resolved / NetGetDCName fails);
  • or the user-home is a network UNC-path (so user-home could not be accessed by NetUserGetInfo);

I don't see any possibility to overcome that weakness, with the exception of the current user (I mean if the user-name is the name of the current user, then it may be possible using environment variables HOMESHARE resp. HOMEDRIVE/HOMEPATH).


apnadkarni added on 2018-05-25 12:50:26:
For my own understanding, I would like to summarize the situation
*specifically with respect to the function TclpGetUserHome*. The
"name" parameter passed to this function may be one of the forms

1. plain name with no domain (joe). Further,
1a. joe may be a local computer system account, or
1b. joe may be an account in the default domain
2. name in SAM format (ACME\joe) 
3. name in dns format ([email protected]) 

My understanding is that 8.6 (and I assume 8.5) only handled 1a
correctly and was broken for 1b, 2 and 3.

If I read the source correctly, Francois's [adbe7e0e07] changes take
care of 2 and 3 as well but not 1b. [sebres-bug-9e6b569963-8-5-branch]
takes care of 1a and 3 but not 2.

Now when can Case 2 happen? As discussed, the ~domain\user case cannot
be distinguished from ~user\path so we have to forget about worrying
about that case. So the only possible path to that case seems to be if
"~" by itself got mapped to a user account name of the form
domain\user. Now it turns out tracing back to the code that this never
happens. A ~ by itself always resolves to env(HOME). env(HOME) in turn
is picked up from the environment and if it it does not exist, it is
constructed from env(HOMEDRIVE) and env(HOMEPATH). If those do not
exist either, it is set to the root of the system drive.

Thus I believe TclpGetUserHome will never see Case 2.

HOWEVER...

The current behaviour of constructing a "fake" HOME environment
variable if it does not exist seems just plain wrong to me. What Tcl
*should* be doing if HOME is not defined is call TclpGetUserHome with
the current user name retrieved via GetUserNameEx. I think this change
should be made for some future release, 8.7 or 9.

This for future proofing, I would suggest picking up the sebres*
branch with the addition of the check for domain\user from Francois.

On a stylistic issue (not bug), it would be clearer to me at least if
the single while loop was broken out explicitly rather than setting
domain = -1.

fvogel added on 2018-05-25 11:51:18:
In branch "sebres-bug-9e6b569963-8-5-branch":

Works for me on win7 when connected to the network. All tests from fileSystem.test pass.

However when disconnected from the network then fileSystem-1.30.1 fails.

(Same thing in branch "bug-9e6b569963")

sebres added on 2018-05-25 10:14:23:
Regarding the branch "sebres-bug-9e6b569963-8-5-branch":
  - the test-cases are merged/reintegrated from "bug-9e6b569963" now;
  - all tests passed;

I would like to merge, if someone find the time to review resp. to test this and no objections follow.
I myself have tested on two VM's with and without PDC (with remote as well as with local users).

fvogel added on 2018-05-24 21:23:14:

Which merge do you mean?

I mean, there is a bugfix branch "bug-9e6b569963" that I created to deal with the present ticket. It gathers the fixes and one or two new test cases I found useful to add to guard against possible regressions in the future. Usually I let the fix mature in a bugfix branch and only when fully satisfied I merge to core-8-6-branch and forward.

In the present case, if we want to keep what's in the bugfix branch we should first merge core-8-6-branch into the bugfix branch, refine the fix as needed, add test cases and so on, and when finished merge back to core-8-6-branch and forward. On the other hand, if you believe your fix in "sebres-bug-9e6b569963-8-5-branch" is better than what's in the bugfix branch (and I tend to believe so), that's fine for me, just close the bugfix branch "bug-9e6b569963" without merging (but after cherrypicking the commits with the new tests perhaps).

As regards the case "~domain\user", I see what you mean and I'm in agreement with you. Let's just forget about this syntax then (no code change to support it, no backwards compat issue), right?


sebres added on 2018-05-24 21:11:10:

Which merge do you mean?

If current 8-5/8-6, then not - this resolves only normalization bug.

The branch "sebres-bug-9e6b569963-8-5-branch" should resolve the subject, but only for case "no domain specified".

As regards the case "~doman\user", I try to explain it with example:

file normalize ~$A\\$B

What is "$A" here - domain or username? Because dependent on this answer - "$B" will be a user or a path, so this going rather to direction "UB". And by such undefined behaviors at least the priority question is interesting (as well as the backwards-compatibility question also).


fvogel added on 2018-05-24 21:05:00:
Our messages crossed.

sebres, you're obviously much more proficient than I am on these subjects. I'm willing to let you decide which fix is best and commit/merge.

Thanks!

fvogel added on 2018-05-24 20:59:37:

Great, thanks!

Does this merge seamlessly with the present bugfix branch bug-9e6b569963 ?

Besides, a comment about ashok's reply:

> I don't think it is possible to pass DOMAIN\USER as a user name in Tcl's file
> syntax. The moment Tcl sees the backslash when parsing a path beginning with
> ~, it will interpret it as end of the user name.

Well, (the relevant part of) the call chain is:

... -> Tcl_EvalObjEx -> TclEvalObjEx -> TclNRRunCallbacks -> Dispatch -> PathNormalizeCmd -> Tcl_FSGetNormalizedPath -> Tcl_FSConvertToPathType -> SetFsPathFromAny -> TclpGetUserHome

The path string propagated through this chain is "~domain" from PathNormalizeCmd and downwards.

I believe something happens in TclEvalObjEx:

...
    result = TclNREvalObjEx(interp, objPtr, flags, invoker, word);
    return TclNRRunCallbacks(interp, result, rootPtr);
...

There objptr still has file normalize ~domain\\user as a value.

TclNREvalObjEx calls TclCompileObj and something must be happening here that trims ~domain\\user to simply ~domain.

Anyway this does not actually matter a lot. I have removed test filesystem-1.30.3 that I had added previously, which was attempting to check for this domain\user syntax.

Regarding observations from sebres, I can't reproduce on the machine I'm sitting at right now, because it's not connected to a domain controller. In that case I get (and this is expected - I have just replaced the real domain name by xxxxxx in the output below):

% set fn [file normalize ~$::tcl_platform(user)@$::env(USERDOMAIN)]
user "francois@xxxxxxxx" doesn't exist
% file normalize $fn
can't read "fn": no such variable
I'll try on a machine connected to a DC to confirm what's happening there, as soon as I can put my hands on such one.


sebres added on 2018-05-24 20:58:40:

I found another variant to fix the subject, commited for 8.5th as branch "sebres-bug-9e6b569963-8-5-branch" (see [d6a73c524c]), if accepted I could rewrite/merge it for 8.6...

The fix does not affect the case "~doman\user" (which is as already said too dangerous IMHO, because can be confused with "~user\path"). But resolves subject issue, where user specified without domain, so fixes exactly this case:

file normalize ~$::tcl_platform(user)

Only in the case such local user does not exists.


sebres added on 2018-05-24 20:40:51:
normalization bug of user-home path fixed for 8.5/8.6 now

sebres added on 2018-05-24 11:03:58:

BTW. I find another strange behavior by user-home normalization:

% set fn [file normalize ~$::tcl_platform(user)@$::env(USERDOMAIN)]
\\hspfil08.my-domain\home\sergey.brester
% file normalize $fn
\\hspfil08.my-domain\home\sergey.brester
% file normalize [string trim " $fn "]
//hspfil08.my-domain/home/sergey.brester

As you can see the normalization does not work in case of user-home, and file normalize returns native windows-path as is. The second test with "string trim" causes creation of new tcl-object (quasi to reset "normalized" flag), so enforces the normalization again (and this time it works)...


sebres added on 2018-05-24 10:33:04:

I assume also the way with backslash may be a bit dangerous, but second form with "@" char (like "user@domain") should work pretty good:

set my_domain my-domain
file nativename ~$::tcl_platform(user)@$my_domain
\\hspfil08.my-domain\home\sergey.brester


apnadkarni added on 2018-05-24 01:34:46:
I don't think it is possible to pass DOMAIN\USER as a user name in Tcl's file syntax. The moment Tcl sees the backslash when parsing a path beginning with ~, it will interpret it as end of the user name.

The only time I think that code will be hit is if the current user name (~) maps directly to DOMAIN\USER.

/Ashok

fvogel added on 2018-05-23 21:17:56:

Checked in [adbe7e0e07] attempting to deal with the so-called "down-level logon name format", i.e. domain\user.

Something is wrong here: when calling file normalize ~domain\\user , routine TclpGetUserHome() does not receive the expected domain\user string, it receives only domain and I don't see why.

Can you help perhaps?


fvogel added on 2018-05-17 19:31:22:

Oh OK, I see.


apnadkarni added on 2018-05-16 03:41:16:
Certainly an improvement. Just a comment - in addition to the user@domain format, the user name may also be in the domain\user format so that should be checked for as well.

jan.nijtmans added on 2018-05-15 19:53:49:
Well spotted! +1