Software update fails if current directory is root directory

RESOLVED FIXED in mozilla1.9.1b2

Status

()

Toolkit
Application Update
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: Masahiro YAMADA, Assigned: rstrong)

Tracking

unspecified
mozilla1.9.1b2
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 8 obsolete attachments)

16.22 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
6.42 KB, patch
Benjamin Smedberg
: review+
rstrong
: review+
Details | Diff | Splinter Review
22.94 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
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.
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)?
(Reporter)

Comment 2

10 years ago
(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.
(Reporter)

Comment 3

10 years ago
(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
Do you get the run as dialog still?
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?
(Reporter)

Comment 6

10 years ago
(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.
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

10 years ago
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

10 years ago
(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 11

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

Thanks.
It's not related to NSPR.
(Reporter)

Comment 12

10 years ago
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
(Reporter)

Comment 13

10 years ago
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;
}
(Reporter)

Comment 14

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

Comment 16

10 years ago
Created attachment 337851 [details]
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.

Updated

10 years ago
Assignee: nobody → masa141421356
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 17

10 years ago
(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.
(Reporter)

Comment 18

10 years ago
Created attachment 337885 [details]
Proposal and test rev.2

1.Fixed String length mismatch between QuotedStrLen and QuotedString
2.QuotedString does not escape double-quotation correctly.
Attachment #337851 - Attachment is obsolete: true
(Reporter)

Updated

10 years ago
Summary: Software update fails if executed from start command in batfile → Software update fails if current directory is root directory
(Reporter)

Comment 19

10 years ago
Created attachment 338116 [details] [diff] [review]
patch rev.1.0 for Trunk (based on proposal rev.2)
Attachment #338116 - Flags: review?(ted.mielczarek)
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.
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\"
(Reporter)

Comment 22

10 years ago
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?
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.
Attachment #338116 - Flags: review?(ted.mielczarek) → review?(robert.bugzilla)
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)
Created attachment 339917 [details] [diff] [review]
Tests rev 1
Assignee: masa141421356 → robert.bugzilla
Attachment #339917 - Flags: review?(ted.mielczarek)
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.
Attachment #338116 - Flags: review?(robert.bugzilla) → review-
Attachment #339917 - Flags: review?(jmathies)
Created attachment 339918 [details] [diff] [review]
patch rev1
Attachment #337885 - Attachment is obsolete: true
Attachment #338116 - Attachment is obsolete: true
Attachment #339918 - Flags: review?(ted.mielczarek)
Attachment #339918 - Flags: review?(jmathies)
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.
Attachment #339917 - Flags: review?(ted.mielczarek)
Attachment #339917 - Flags: review?(jmathies)
Comment on attachment 339917 [details] [diff] [review]
Tests rev 1

Found a problem so I'm going to hold off for a bit
Created attachment 340002 [details] [diff] [review]
Tests rev 2
Attachment #339917 - Attachment is obsolete: true
Attachment #340002 - Flags: review?(jmathies)
Attachment #340002 - Flags: review?(ted.mielczarek)
Attachment #340002 - Attachment is obsolete: true
Attachment #340002 - Flags: review?(ted.mielczarek)
Attachment #340002 - Flags: review?(jmathies)
Comment on attachment 340002 [details] [diff] [review]
Tests rev 2

bah... I forgot to clean up the test's Makefile
Created attachment 340402 [details] [diff] [review]
 Tests rev 3
Attachment #340402 - Flags: review?(jmathies)
Attachment #340402 - Flags: review?(ted.mielczarek)
Attachment #339918 - Flags: review?(ted.mielczarek) → review+
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 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!
Attachment #340402 - Flags: review?(jmathies) → review+
Attachment #340402 - Flags: review?(ted.mielczarek) → review?(jmathies)
Comment on attachment 340402 [details] [diff] [review]
 Tests rev 3

I totally +ed the wrong r?.

Comment 36

10 years ago
+  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.

Updated

10 years ago
Attachment #340402 - Flags: review?(jmathies) → review+
Created attachment 340642 [details] [diff] [review]
patch rev2

Updated patch to Ted's and Jim's comments.
Attachment #339918 - Attachment is obsolete: true
Attachment #340642 - Flags: review?(jmathies)
Attachment #339918 - Flags: review?(jmathies)
Comment on attachment 340642 [details] [diff] [review]
patch rev2

Carrying forward Ted's r+
Attachment #340642 - Flags: review+
Created attachment 340644 [details] [diff] [review]
Tests rev 4

Addresses Ted's comments... carrying forward r+
Attachment #340402 - Attachment is obsolete: true
Attachment #340644 - Flags: review+
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 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.
Created attachment 341155 [details] [diff] [review]
patch rev3

Benjamin, could you review this patch? Thanks
Attachment #340642 - Attachment is obsolete: true
Attachment #341155 - Flags: review?(benjamin)
Attachment #340642 - Flags: review?(jmathies)
Comment on attachment 341155 [details] [diff] [review]
patch rev3

Carrying forward r+ from Ted
Attachment #341155 - Flags: review+

Comment 44

10 years ago
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?
Attachment #341155 - Flags: review?(benjamin) → review+
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

10 years ago
I was not joking, but I was only reading the attachment I was reviewing.
Sorry about that... I didn't realize you hadn't seen the tests.
Created attachment 342666 [details] [diff] [review]
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
Attachment #342666 - Flags: review+
Pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/14db21f85e282a63f2b1cb70fdd9d8df324515a0
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
Backed out the updater.cpp changes due to bug 459569. Filed bug 459615 to clean up the windows' restart code
You need to log in before you can comment on or make changes to this bug.