453733 - Software update fails if current directory is root directory

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Masahiro YAMADA]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja-KS; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080904035000 Minefield/3.1b1pre

When firefox.exe is executed by start command in batch file,
Software update shows RunAs dialog.
And ,after inputing correct password, updater never executed.

Reproducible: Always

Steps to Reproduce:
1.create batchfile
----------------
@echo off
set MOZ_NO_REMOTE=true
start "Minefield" "C:\Program Files\Minefield\firefox.exe" -no-remote -p Trunk %1 %2
-----------
2.create shortcut of batch file on desktop and set workind directory as "C:\"
3.double click shortcut of batch file
4.Do software update with "Help" - "Check for Update"
2.
3.
Actual Results:  
If update exists and download completed, update dialog is shown.
But, when starting update, "RunAs Dialog" is shown.
But selecting any user and option, updater NEVER start update.


Expected Results:  
Software update should not fail.

When runnning firefox.exe on same user with shortcut (to C:\Program Files\Minefield\firefox.exe -no-remote -p Trunk), update never fails.

Comment 1

[Robert Strong (they/them - no direct email)]

I landed a patch today in bug 453693 that likely fixes this use case.

Can you try again by  first updating to tomorrow's build (09-05) and check if it works with the next update after that (09-06)?

Comment 2

[Masahiro YAMADA]

(In reply to comment #1)
> I landed a patch today in bug 453693 that likely fixes this use case.
> 
> Can you try again by  first updating to tomorrow's build (09-05) and check if
> it works with the next update after that (09-06)?

Okay. I 'll test and report result here.

Comment 3

[Masahiro YAMADA]

(In reply to comment #2)
This issue is still reproduced.

(In reply to comment #2)
In some case, this issue is not reproduced by steps in comment #0
Some pre-steps is needed.

1.uninstall firefox and firefox trunk,
2.remove all profiles and firefox related file on your "Local Settings" folder and "App Config" floder
3.install trunk ,but ,you MUST NOT launch Minefield from installer
4.do step written in Comment #0

Comment 4

[Robert Strong (they/them - no direct email)]

Do you get the run as dialog still?

Comment 5

[Robert Strong (they/them - no direct email)]

Also, when you say, "In some case, this issue is not reproduced by steps in comment #0 Some pre-steps is needed." were you able to reproduce without the additional steps?

Comment 6

[Masahiro YAMADA]

(In reply to comment #4)
> Do you get the run as dialog still?

Yes. I get run asdialog still.

Update related files (update.status, update, updater.ini, updater.exe)
are exists on corret directory:
%USERPROFILE%\Local Settings\Application Data\Mozilla\Firefox\Mozilla Firefox\updates\0

(In reply to comment #5)
> Also, when you say, "In some case, this issue is not reproduced by steps in
> comment #0 Some pre-steps is needed." were you able to reproduce without the
> additional steps?
Once update succeeded update from shortcut made by installer,I cannot reproduce this bug.
To reproduce again, steps in comment #3 is needed.

Comment 7

[Robert Strong (they/them - no direct email)]

We shouldn't be adding the files to 
%USERPROFILE%\Local Settings\Application Data\Mozilla\Firefox\Mozilla
Firefox\updates\0

unless you are running on Vista with UAC turned on.

What does "Update related files (update.status, update, updater.ini, updater.exe)
are exists on corret directory:
%USERPROFILE%\Local Settings\Application Data\Mozilla\Firefox\Mozilla
Firefox\updates\0" mean specifically?

Does it mean that you updated them into this directory or that they were updated into that directory by the update process?

Have you performed any custom configuration of permissions on your system or set the bat file to launch using different permissions, etc?

Comment 8

[Atsushi Sakai]

I can reproduce this.

When Firefox launches updater, the commandline is like
"C:\path\updater.exe" "dir-path" "pid" "C:\" "C:\Program Files\Mozilla Firefox\firefox.exe"
but I guess "C:\" should be "C:\\".
Otherwise, double-quote will be unescaped, and 
argv[3] will be C:" C:\Program
argv[4] will be Files\Mozilla

This is not Application Update issue, but I guess problem is toolkit/xre/nsWindowsRestart.cpp.

Comment 9

[Atsushi Sakai]

(In reply to comment #8)
> This is not Application Update issue, but I guess problem is
> toolkit/xre/nsWindowsRestart.cpp.

Oops, it might be wrong.
It might be in NSPR.

Comment 10

[Masahiro YAMADA]

Updater is launched from 
WinLaunchChild at
http://hg.mozilla.org/mozilla-central/annotate/f5cd1cb9419e/toolkit/xre/nsWindowsRestart.cpp#l264
from ApplyUpdate at
http://hg.mozilla.org/mozilla-central/annotate/9737aa363037/toolkit/xre/nsUpdateDriver.cpp#l477

Comment 11

[Atsushi Sakai]

(In reply to comment #10)
> Updater is launched from 
> WinLaunchChild at

Thanks.
It's not related to NSPR.

Comment 12

[Masahiro YAMADA]

I think, QuoteString at
http://hg.mozilla.org/mozilla-central/annotate/f5cd1cb9419e/toolkit/xre/nsWindowsRestart.cpp#l90
should be fixed because it does care command-line parser of Windows.

See http://msdn.microsoft.com/ja-jp/library/17w5ykft(VS.80).aspx

Comment 13

[Masahiro YAMADA]

proposal of alternative code of QuoteString and QuotedStrLen
(I'ii write patch later)

static int QuoteStrLen(const PRUnichar *s) {
  int i = 2; // initilal and final quote
  int backslashCount = 0;
  while (*s) {
    if (*s == '"') {
        i++;
        i+=(backslashCount*2);
        backslashCount=0;
    } else if (*s == '\\') {
      backslashCount++;
    } else {
      backslashCount=0;
    }
    ++s;
  }
  i+=(backslashCount*2);
}

static PRUnichar* QuoteString(PRUnichar *d, const PRUnichar *s)
{
  *d = '"';
  ++d;
  int backslashCount = 0;
  while (*s) {
    *d = *s;
    if (*s == '"') {
      if (while (backslashCount >0) {
          ++d;
          *d = '\\';
          backslashCount--;
      }
      ++d;
      *d = '"';
    } else if (*s == '\\') {
      backslashCount++;
    } else {
      backslashCount=0;
    }
    ++d; ++s;
  }
  while (backslashCount >0) {
      ++d;
      *d = '\\';
      backslashCount--;
  }
  *d = '"';
  return d;
}

Comment 14

[Masahiro YAMADA]

(In reply to comment #13)
Oops,
 return i
is needed at tail of QuotedStrLen.

Comment 15

[Atsushi Sakai]

FYI:
Similar quoting is already at
http://hg.mozilla.org/mozilla-central/annotate/d7b1a5551e2c/xpcom/threads/nsProcessCommon.cpp#l109

Comment 16

[Masahiro YAMADA]

Attachment: Proposal and test of QuotedStrLen and QuoteString

This sample shows all argv and re-composed commandline string.
MakeCommandLine is same as MakeCommandLine of nsWindowsRestart.cpp.

Comment 17

[Masahiro YAMADA]

(In reply to comment #15)
> FYI:
> Similar quoting is already at
> http://hg.mozilla.org/mozilla-central/annotate/d7b1a5551e2c/xpcom/threads/nsProcessCommon.cpp#l109

Thank you. But I found another problem.
It uses char** for argv.
If argv is Shift_JIS encoded, it may not work correctly.

Comment 18

[Masahiro YAMADA]

Attachment: Proposal and test rev.2

1.Fixed String length mismatch between QuotedStrLen and QuotedString
2.QuotedString does not escape double-quotation correctly.

Comment 19

[Masahiro YAMADA]

Attachment: patch rev.1.0 for Trunk (based on proposal rev.2)

Comment 20

[Robert Strong (they/them - no direct email)]

Comment on attachment 338116 [details] [diff] [review]
patch rev.1.0 for Trunk (based on proposal rev.2)

This seems like it always adds a backslash when it encounters a backslash. This will end up with C:\ -> C:\\ and when it is called again C:\\ -> C:\\\\. There is a pre-existing bug with this code where it always quotes as well.

U've got a fix for the above and am just about done with tests. If you don't mind I can take this bug.

Comment 21

[Robert Strong (they/them - no direct email)]

Notes:
I've never liked how MakeCommandLine always adds quotes to the args and the root cause here is due to C:\ being turned into "C:\". If we pass
"C:\" "C:\Program Files\"
will per the MS spec become
argv[1] C:" C:\Program
argv[2] Files"

Escaping all \ then
"C:\\" "C:\\Program Files\\"
becomes
argv[1] C:\
argv[2] C:\\Program Files\

If we only quote args that contain a space or a tab and escape the last " when it is preceded by a \ then the commandline would be
C:\ "C:\Program Files\"

Comment 22

[Masahiro YAMADA]

I think , quoting algorithm of assembleCmdLine in nsCommonProcess.cpp is better than MakeCommandLine.
But on mulibyte code handling, and safe process executing, nsWindowsRestart.cpp seems to be better (I filed bug 454608 for problems of nsCommonProcess.cpp)

And, I have a question.
Both of them has almostly same function. Shoule we unify them?

Comment 23

[Robert Strong (they/them - no direct email)]

I agree that the quoting algorithm in assembleCmdLine is better than MakeCommandLine but neither are all that good. ;)

nsIProcess is in really bad shape from what I've seen and I think it may be a good idea to unify them after nsIProcess has been improved.

Comment 24

[(not currently active) Ted Mielczarek]

Comment on attachment 338116 [details] [diff] [review]
patch rev.1.0 for Trunk (based on proposal rev.2)

Rob should probably review this. (if he wants this patch)

Comment 25

[Robert Strong (they/them - no direct email)]

Attachment: Tests rev 1

Comment 26

[Robert Strong (they/them - no direct email)]

Comment on attachment 338116 [details] [diff] [review]
patch rev.1.0 for Trunk (based on proposal rev.2)

There are a couple of problems with the existing code this doesn't address. While writing the tests I created a patch that addresses them so I'm going to take this bug.

Comment 27

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Comment 28

[Robert Strong (they/them - no direct email)]

Comment on attachment 339918 [details] [diff] [review]
patch rev1

The change to updater.cpp is to handle the case where an app wants to provide multiple command line arguments in the updater.ini.

Comment 29

[Robert Strong (they/them - no direct email)]

Comment on attachment 339917 [details] [diff] [review]
Tests rev 1

Found a problem so I'm going to hold off for a bit

Comment 30

[Robert Strong (they/them - no direct email)]

Attachment: Tests rev 2

Comment 31

[Robert Strong (they/them - no direct email)]

Comment on attachment 340002 [details] [diff] [review]
Tests rev 2

bah... I forgot to clean up the test's Makefile

Comment 32

[Robert Strong (they/them - no direct email)]

Attachment: Tests rev 3

Comment 33

[(not currently active) Ted Mielczarek]

Comment on attachment 339918 [details] [diff] [review]
patch rev1

+  BOOL hasDuobleQuote = wcspbrk(s, L"\"") != NULL;

Typo in the variable name there. Also, any reason you're using wcspbrk instead of wcschr, given that you're only searching for one specific character?

Looks good. Thanks for the informative comments!

Comment 34

[(not currently active) Ted Mielczarek]

Comment on attachment 340402 [details] [diff] [review]
 Tests rev 3

+  // to the binary being executed and MakeCommandLine only handlkes argv[1] and

typo in the comment.

+    wprintf(L"TEST%sFAIL | %s ARGC Comparison | Test %2d\n", passes ? L"-" : L"-KNOWN-", LOG_PREFIX, testNum);

I should document this on a wiki page somewhere, but the test result values we're using are:

TEST-PASS
TEST-KNOWN-FAIL
TEST-UNEXPECTED-FAIL
TEST-UNEXPECTED-PASS

The first two are not considered errors, the second two are. The first two can also be followed by (EXPECTED RANDOM), but that's only used in reftest currently.

Otherwise, looks great!

Comment 35

[(not currently active) Ted Mielczarek]

Comment on attachment 340402 [details] [diff] [review]
 Tests rev 3

I totally +ed the wrong r?.

Comment 36

[Jim Mathies [:jimm]]

+  int i = wcslen(s);
+  BOOL hasDuobleQuote = wcspbrk(s, L"\"") != NULL;

Shouldn't we have a null check on s before calling these? wcslen will exception on a null pointer.

Comment 37

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2

Updated patch to Ted's and Jim's comments.

Comment 38

[Robert Strong (they/them - no direct email)]

Comment on attachment 340642 [details] [diff] [review]
patch rev2

Carrying forward Ted's r+

Comment 39

[Robert Strong (they/them - no direct email)]

Attachment: Tests rev 4

Addresses Ted's comments... carrying forward r+

Comment 40

[Robert Strong (they/them - no direct email)]

Comment on attachment 340644 [details] [diff] [review]
Tests rev 4

Note: I changed the existing tab in command line test and added a new tab in command line test.

Comment 41

[Robert Strong (they/them - no direct email)]

Comment on attachment 340644 [details] [diff] [review]
Tests rev 4

Jim, I looked at all of the callers and the null pointer check should not necessary. If a caller ever does send a null pointer we should throw an exception.

Comment 42

[Robert Strong (they/them - no direct email)]

Attachment: patch rev3

Benjamin, could you review this patch? Thanks

Comment 43

[Robert Strong (they/them - no direct email)]

Comment on attachment 341155 [details] [diff] [review]
patch rev3

Carrying forward r+ from Ted

Comment 44

[Benjamin Smedberg]

Comment on attachment 341155 [details] [diff] [review]
patch rev3

Boy... unit tests for the arg-quoting routines would be so nice! Any thoughts how we could accomplish that?

Comment 45

[Robert Strong (they/them - no direct email)]

Benjamin, I'm not sure if you are teasing or if there is something I am missing? Attachment #340644 [details] [diff] covers the tests which include quoting arguments, etc.

Comment 46

[Benjamin Smedberg]

I was not joking, but I was only reading the attachment I was reviewing.

Comment 47

[Robert Strong (they/them - no direct email)]

Sorry about that... I didn't realize you hadn't seen the tests.

Comment 48

[Robert Strong (they/them - no direct email)]

Attachment: patch with tests - as checked in

patch with tests as checked in with one minor change to protect against callers that pass in 0 args

Comment 49

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/14db21f85e282a63f2b1cb70fdd9d8df324515a0

Comment 50

[Robert Strong (they/them - no direct email)]

Forgot to include the xre makefile change
http://hg.mozilla.org/mozilla-central/rev/d391ce2f8e81bb8cfa32a16c92daf493c5d51f37

Comment 51

[Jim Jeffery not reading bug-mail 1/2/11]

Could this have caused: 
https://bugzilla.mozilla.org/show_bug.cgi?id=459569 ?

Comment 52

[Robert Strong (they/them - no direct email)]

Backed out the updater.cpp changes due to bug 459569. Filed bug 459615 to clean up the windows' restart code