303599 - Many problems if NtfsDisable8dot3NameCreation set to 1

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[jsnod]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050805 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050805 Firefox/1.0+

Deer Park has problems if it insallled with NtfsDisable8dot3NameCreation set to
1 in the Windows registry.

With NtfsDisable8dot3NameCreation = 1, when DP is installed, a 8dot3 shortname
such as DEERPA~1 is not created.  It seems that DP uses 8dot3 in many places, as
I ran into a couple of problems.  Software update would silently fail.  I would
receive numerous error pages on the first start of a new install (Bug 296916).

The only reason this bug made itself apparent is because DP installs into a
directory with spaces in the name ("Deer Park Alpha 2" for example).  These
problems will also appear if a Firefox user installed Firefox into a folder with
spaces in the name.

While it may not be a bug, many many people have NtfsDisable8dot3NameCreation
set to 1 for performance reasons.  If this is determined not to be a bug, DP
should at LEAST give error messages explaining what the problem is, because this
took literally months to figure out.

Reproducible: Always

Steps to Reproduce:
1. Set HKEY_LOCAL_MACHINE\System\CurrentControlSet\Control\FileSystem set
NtfsDisable8dot3NameCreation to 1
2. Reboot
3. Remove C:\Program Files\Deer Park Alpha 2
4. Do a clean install

Actual Results:  
Bug 303598 and Bug 296916

Expected Results:  
Not be dependant on 8dot3 notation, or at least print error messages if and
8dot3 file cannot be found.

Info on the regkey:
http://www.microsoft.com/resources/documentation/Windows/2000/server/reskit/en-us/Default.asp?url=/resources/documentation/Windows/2000/server/reskit/en-us/regentry/28231.asp

Run "dir /x" in C:\Program Files to see if an 8dot3 notation exists for a folder.

Comment 1

[Darin Fisher]

-> bsmedberg

Comment 2

[Benjamin Smedberg]

Asa, this should be relnoted, since it's unlikely we're going to be able to fix
it for 1.5.

Comment 3

[Benjamin Smedberg]

Attachment: Quote all arguments, don't use shortnames, rev. 1

Comment 4

[Darin Fisher]

Comment on attachment 194431 [details] [diff] [review]
Quote all arguments, don't use shortnames, rev. 1

>Index: toolkit/xre/nsAppRunner.cpp

It sort of feels like some of this code could just go into a header file
to be included by both nsAppRunner.cpp and updater.cpp.  That way we don't
actually have to keep duplicate copies of the code.


>+ * Copy string "s" to string "d", quoting and escaping all double quotes.

it would probably be useful to the reader to explain what escaping all
double quotes means.  point out that this escaping is designed to match
the unescaping performed by the MS CRT.  heck, maybe even reference the
CRT source ;-)


>+char*
>+MakeCommandLine(int argc, char **argv)
>+{
>+  int i;
>+  int len = 0;
>+
>+  // skip argv[0]
>+  for (i = 1; i < argc; ++i)
>+    len += QuotedStrLen(argv[i]) + 1;
>+
>+  char *s = (char*) malloc(len);
>+  if (!s)
>+    return nsnull;
>+
>+  char *c = s;
>+  for (i = 0; i < argc; ++i) {
>+    c = QuoteString(c, argv[i]);
>+    *c = ' ';
>+  }

maybe i missed something, but why are you skipping argv[0]
in the length calculation?  it would seem that you should
also skip argv[0] then in the QuoteString loop.  makes me
very nervous seeing all this buffer munging code without
bounds checking :(


>+  STARTUPINFO si;
>+  PROCESS_INFORMATION pi;
>+  ZeroMemory(&si, sizeof(si));
>+  si.cb = sizeof(si);
>+  ZeroMemory(&pi, sizeof(pi));
> 
>+  BOOL ok = CreateProcess(exePath.get(),
>+                          cl,
>+                          NULL,  // no special security attributes
>+                          NULL,  // no special thread attributes
>+                          FALSE, // don't inherit filehandles
>+                          0,     // No special process creation flags
>+                          NULL,  // inherit my environment
>+                          NULL,  // use my current directory
>+                          &si,
>+                          &pi);
>+  if (!ok)
>     return NS_ERROR_FAILURE;
>+
>+  CloseHandle(pi.hProcess);
>+  CloseHandle(pi.hThread);
>+  free(cl);

how about moving the |free| call up before the line that checks |ok|?
then you will be sure to free the memory even if CreateProcess fails,
though that probably isn't a big deal to leak.

i think you could also put this CreateProcess code in a helper function
defined in a header file that could be shared by all three callsites.

Comment 5

[Benjamin Smedberg]

Attachment: Quote all arguments, don't use shortnames, rev. 2

The first patch was wrong in a lot of ways... turns out that you are supposed
to put the exectuable path at the beginning of the command line passed to
CreateProcess().

Comment 6

[Darin Fisher]

Comment on attachment 194445 [details] [diff] [review]
Quote all arguments, don't use shortnames, rev. 2

>Index: toolkit/xre/nsWindowsRestart.cpp

>+    *c = ' ';
>+    ++c;
>+  }
>+
>+  *c = '\0';

So, it looks like this adds an extra space at the end of the
command line.  I guess that is benign, right?


>+  STARTUPINFO si;
>+  PROCESS_INFORMATION pi;
>+  ZeroMemory(&si, sizeof(si));
>+  si.cb = sizeof(si);
>+  ZeroMemory(&pi, sizeof(pi));

FWIW, you can also replace ZeroMemory calls with the following:

    STARTUPINFO si = {0};
    PROCESS_INFORMATION pi = {0};


r=darin

Comment 7

[Benjamin Smedberg]

> So, it looks like this adds an extra space at the end of the
> command line.  I guess that is benign, right?

Right.

>     STARTUPINFO si = {0};
>     PROCESS_INFORMATION pi = {0};

done, with the further enhancement STARTUPINFO si = {sizeof(si), 0}; I would
like to let this bake for a while and get verification from the reporter that
this actually fixes the problem, but this would be very good for the branch.

Comment 8

[Benjamin Smedberg]

*** Bug 296916 has been marked as a duplicate of this bug. ***

Comment 9

[Asa Dotzler [:asa]]

Darin and Ben, we're interested in taking this but we need to understand the
risk better. We're running out of time on beta 1 and if this is scary, we'd
probably want to understand better how widespread is the risk.

Comment 10

[Darin Fisher]

I think we can do beta 1 without this patch.  Now that it's on the trunk, we can
take some time to ensure that it does everything we expect, and then land it on
the branch for beta 2.  I think a week or so of trunk testing should be
sufficient to give us high confidence.  (We usually find out right away if
something goes wrong in the Firefox restart logic.)

Comment 11

[Benjamin Smedberg]

Fixed on 1.8 branch.

Comment 12

[Wayne Mery (:wsmwk)]

did this fixe Bug 303598?