Closed Bug 115819 Opened 23 years ago Closed 22 years ago

You have chosen to download a file of type: "#1" [#2] from

Categories

(Core Graveyard :: File Handling, defect)

Product:
Core Graveyard
Component:
File Handling
Platform:
DEC
OpenVMS
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.1alpha

People

(Reporter: colin, Assigned: law)

References

(
URL
)

Details

(Keywords: helpwanted, Whiteboard: se-radar)

Attachments

(3 files, 1 obsolete file)

How we got into SetPreferredAction the second time
5.32 KB, text/plain
Details
Dialog box with missing information
21.23 KB, image/jpeg
Details
Patch to do the test only on Windows
855 bytes, patch
law
: review+
Details | Diff | Splinter Review 
There appears to be a race condition that I'm seeing on OpenVMS which is 
preventing helper apps from getting created.

nsExternalAppHandler::OnStopRequest calls ExecuteDesiredAction, 
ExecuteDesiredAction calls OpenWithApplication, and its OpenWithApplication 
which (eventually) results in the helper app getting fired up.

But, ExecuteDesiredAction does NOT call OpenWithApplication unless 
mProgressWindowCreated is set, and  stepping through with the debugger I am 
getting into ExecuteDesiredAction without mProgressWindowCreated being set.

http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/nsExternalHelperApp
Service.cpp#1071

I can't find anywhere else in the code where ExecuteDesiredAction 
or OpenWithApplication is called.

So is this a timing bug that's only hitting OpenVMS, or is 
mProgressWindowCreated ALWAYS meant to be set?

If I manually force mProgressWindowCreated to be set, then the helper app DOES 
get created (and I also get a "Save As" dialog box; this is either another 
problem I have to track down or caused by my jamming mProgressWindowCreated).
[ok, i give up, how do I put long URLs into bugzilla WITHOUT getting them 
wrapped???]
i'm able to open helper apps, at least with 12/17 verif bits on linux.
Keywords: qawanted
either use the url field, or use a browser that is semi ignorant of the evil 
word wrap features.
URL: http://lxr.mozilla.org/seamonkey/sour...
Bill, if you are not the best person to help me debug this, can you re-assign to 
someone who would be? I'd like to try and get to the bottom of this problem, but 
need some help. Thanks, Colin.
Colin, probably myself or Scott MacGregor can help you with this.

I see ExecuteDesiredAction getting called from SetWebProgressListener.  Scott 
has comments there that indicate that code may be dealing with your situation, 
or a related one.  It looks like the code there 
(http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/nsExternalHelperAp
pService.cpp#686) is trying to handle the case where you've gone through 
OnStopRequest before the progress dialog has come up.

I think the strategy is:
2. In OnStartRequest, initiate the download (to temporary file; actually, this 
is already underway).  Unless the user has said "don't ask", open helper app 
dialog and ask user whether to save or open.
3. If user said "don't ask", then OnStartRequest calls either SaveToDisk or 
LaunchWithApp directly.
4. Helper app dialog says one of:
  a. Cancel (no problem)
 Note: Step 3 falls through to one of these next two:
  b. Save (calls SaveToDisk, which opens progress dialog);
  c. Open (calls LaunchWithApplication, which opens progress dialog).
5. If progress dialog was opened, calls SetWebProgressListener.  If 
OnStopRequest has already occurred, that calls ExecuteDesiredAction.
6. Otherwise, when OnStopRequest comes in, ExecuteDesiredAction.

I'm not sure I can see where this breaks down.  Do you?  I'll have to think 
about it more carefully.  Maybe you can insert breakpoints to determine what's 
going on?
I put some print statements into the code. They should be self-explanatory.
Here's what I got.

CRB: Entered ExecuteDesiredAction
CRB:   mProgressWindowCreated=0
CRB:   mCanceled=0
CRB: Leaving ExecuteDesiredAction

Save to dialog appears. I click on SAVE and then:

CRB: Entered SetWebProgressListener
CRB:   mStopRequestIssued=1
CRB:   aWebProgressListener=63699488
CRB: About to call ExecuteDesiredAction
CRB: Entered ExecuteDesiredAction
CRB:   mProgressWindowCreated=1
CRB:   mCanceled=0
CRB:   Inside first if statement
CRB:   action=0 (saveToDisk)
CRB:   after GetPreferredAction, action=0
CRB:   About to call MoveFile
CRB: Leaving ExecuteDesiredAction

So it looks like the "problem" is that the action is saveToDisk, even though
I have defined a helper app for the MIME type I'm downloading (PDF):

If I stop in ExecuteDesiredAction 

DBG> ex *this
*nsExternalAppHandler::ExecuteDesiredAction::this: class nsExternalAppHandler
    inherit nsIStreamListener
        inherit nsIRequestObserver
            inherit nsISupports
                __vptr: 16480920
            __vptr:     16480920
        __vptr: 16480920
    inherit nsIHelperAppLauncher
        inherit nsISupports
            __vptr:     16088408
        __vptr: 16088408
    inherit nsIURIContentListener
        inherit nsISupports
            __vptr:     16096648
        __vptr: 16096648
    inherit nsIInterfaceRequestor
        inherit nsISupports
            __vptr:     16094472
        __vptr: 16094472
    mRefCnt:    3
    _mOwningThread:     73671520
    mTempFile: class nsCOMPtr<nsIFile>
        mRawPtr:        92355696
    mSourceUrl: class nsCOMPtr<nsIURI>
        mRawPtr:        91394112
    mTempFileExtension: class nsCString
        inherit nsAFlatCString
            inherit nsASingleFragmentCString
                inherit nsACString
                    __vptr:     66530552
                __vptr: 66530552
            __vptr:     66530552
        inherit nsStr
            mLength:    4
            mCapacity:  4
            union
                [Displaying union member number 1]
                mStr:   92368832
            mCharSize:  0
            mOwnsBuffer:        1
        __vptr: 66530552
    mMimeInfo: class nsCOMPtr<nsIMIMEInfo>
        mRawPtr:        92354200
    mOutStream: class nsCOMPtr<nsIOutputStream>
        mRawPtr:        0
    mWindowContext: class nsCOMPtr<nsISupports>  (has no non-static data
members)
        inherit nsCOMPtr_base
            mRawPtr:    89878792
    mSuggestedFileName: class nsString
        inherit nsAFlatString
            inherit nsASingleFragmentString
                inherit nsAString
                    __vptr:     66530200
                __vptr: 66530200
            __vptr:     66530200
        inherit nsStr
            mLength:    42
            mCapacity:  42
            union
                [Displaying union member number 1]
                mStr:   92352744
            mCharSize:  1
            mOwnsBuffer:        1
        __vptr: 66530200
    mCanceled:  0
    mReceivedDispostionInfo:    0
    mStopRequestIssued: 1
    mProgressWindowCreated:     0
    mTimeDownloadStarted:       1010503283078830
    mFinalFileDestination: class nsCOMPtr<nsIFile>
        mRawPtr:        0
    mDataBuffer:        84394184
    mLoadCookie: class nsCOMPtr<nsISupports>  (has no non-static data members)
        inherit nsCOMPtr_base
            mRawPtr:    92348992
    mWebProgressListener: class nsCOMPtr<nsIWebProgressListener>
        mRawPtr:        0
    mOriginalChannel: class nsCOMPtr<nsIChannel>
        mRawPtr:        92340680
    mDialog: class nsCOMPtr<nsIHelperAppLauncherDialog>
        mRawPtr:        92471864
    __vptr:     16480920
DBG> 

And inside GetPreferredAction:

DBG> ex *this
*nsMIMEInfoImpl::GetPreferredAction::this: class nsMIMEInfoImpl
    inherit nsIMIMEInfo
        inherit nsISupports
            __vptr:     16343912
        __vptr: 16343912
    mRefCnt:    1
    _mOwningThread:     73671520
    mExtensions: class nsCStringArray
        inherit nsVoidArray
            mImpl:      92367048
            __vptr:     66517144
        __vptr: 66517144
    mDescription: class nsAutoString
        inherit nsString
            inherit nsAFlatString
                inherit nsASingleFragmentString
                    inherit nsAString
                        __vptr: 66515240
                    __vptr:     66515240
                __vptr: 66515240
            inherit nsStr
                mLength:        3
                mCapacity:      63
                union
                    [Displaying union member number 1]
                    mStr:       92354240
                mCharSize:      1
                mOwnsBuffer:    0
            __vptr:     66515240
        mBuffer:        "p"
        __vptr: 66515240
    mURI: class nsCOMPtr<nsIURI>
        mRawPtr:        0
    mMacType:   1415071060
    mMacCreator:        1954051188
    mMIMEType: class nsCString
        inherit nsAFlatCString
            inherit nsASingleFragmentCString
                inherit nsACString
                    __vptr:     66530552
                __vptr: 66530552
            __vptr:     66530552
        inherit nsStr
            mLength:    15
            mCapacity:  15
            union
                [Displaying union member number 1]
                mStr:   92368480
            mCharSize:  0
            mOwnsBuffer:        1
        __vptr: 66530552
    mPreferredApplication: class nsCOMPtr<nsIFile>
        mRawPtr:        92354776
    mPreferredAction:   0
    mPreferredAppDescription: class nsString
        inherit nsAFlatString
            inherit nsASingleFragmentString
                inherit nsAString
                    __vptr:     66530200
                __vptr: 66530200
            __vptr:     66530200
        inherit nsStr
            mLength:    4
            mCapacity:  4
            union
                [Displaying union member number 1]
                mStr:   92368544
            mCharSize:  1
            mOwnsBuffer:        1
        __vptr: 66530200
    __vptr:     16343912
DBG> 

I can see an access() call to the file I've defined as my helper app, so I know 
the MIME stuff is set up correctly.

I have a lunch appointment now, but this afternoon I'll try to track down why I 
don't have an action defined.
I did a bit more debugging, starting from the access() call to verify that my
helper app exists. I stepped through the code and everything seemed to get set
up correctly for the helper app.

BUT, remember I said that ExecuteDesiredAction is entered twice (the first time
before the "save as" dialog appears, and the second time after I dismiss the
"save as"). Well, the first time in GetPreferredAction is 2 (I put an extra call
in to get this value), but the second time in its 0.

So why, after setting up the application correctly, is it getting lost?

Here's the tracing again, with some extra info:

CRB: Entered ExecuteDesiredAction with this=63065888
CRB:   mMimeInfo=63182120
CRB:   mMimeInfo->GetPreferredAction=2
CRB:   mProgressWindowCreated=0
CRB:   mCanceled=0
CRB: Leaving ExecuteDesiredAction

"save as" dialog appears now and is dismissed

CRB: Entered SetWebProgressListener
CRB:   mStopRequestIssued=1
CRB:   aWebProgressListener=63630648
CRB: About to call ExecuteDesiredAction
CRB: Entered ExecuteDesiredAction with this=63065888
CRB:   mMimeInfo=63182120
CRB:   mMimeInfo->GetPreferredAction=0
CRB:   mProgressWindowCreated=1
CRB:   mCanceled=0
CRB:   Inside first if statement
CRB:   action=0 (saveToDisk)
CRB:   after GetPreferredAction, action=0
CRB:   About to call MoveFile
CRB: Leaving ExecuteDesiredAction

If I do this in the second call to ExecuteDesiredAction,

    mMimeInfo->SetPreferredAction(2)

it works (my PDF viewer gets created).
Maybe I'm doing something wrong. I just set up a simple helper app on my Linux 
system for pdf files. Its just a shell script that writes the time to a file. 
Anyway, its not getting invoked.

When I set up the helper app I specified a file type of "pdf" and a MIME type of 
"application/pdf". That's right, right?
Yes (assuming you meant "file extension" rather than "file type").

It looks like the nsIMIMEInfo's "preferredAction" attribute is changing in
between the two calls to ExecuteDesiredAction.

Might the curious code at
http://lxr.mozilla.org/seamonkey/source/embedding/components/ui/helperAppDlg/nsHelperAppDlg.js#358
somehow be involved?

I'll check that out tomorrow, Bill.

Are you able to get a simple helper app working on Linux? Or is it just me?
Bill, this is really sounding like a dup of bug 98084 now. That's good, because 
I hate OpenVMS specific problems!
Colin, my Linux box is out of commision co I can't test this myself.  I suspect
there is some sort of timing issue because I gotta believe helper apps are
working on Linux for most people.  But maybe its one of those things where
people just don't expect it to work so they don't complain when it doesn't...

How does this bug manifest itself, exactly?  You're clicking on a link to the
application/pdf content, presumably.  Do you see the "download dialog" (asking
whether to open with application versus save to disk)?  If so, is your helper
app displayed in that dialog?

Do you see a progress dialog?  Do you get a file-picker dialog?
Bill, I'm not at all sure helper apps are working for a lot of Linux people. 
See bug 98084 for other people's tales of woe. With all due respect, I think you 
need to get your Linux system up and running.

> How does this bug manifest itself, exactly?  You're clicking on a link to
> the application/pdf content, presumably.  

Yes.

> Do you see the "download dialog" (asking whether to open with
> application versus save to disk)?

No. All I get is the "save to disk" dialog.

> Do you see a progress dialog? 

Yes.

> Do you get a file-picker dialog?

Yes, that's the first thing to appear after I click on the PDF link. This is 
what I'm referring to as the "save to disk' dialog.

The second thing to appear, after I dismiss the file picker dialog, is the 
progress dialog.

Those two are the only dialogs I see.
>Bill, I'm not at all sure helper apps are working for a lot of Linux people. 
>See bug 98084 for other people's tales of woe. With all due respect, I think you 
>need to get your Linux system up and running.

Either that, or reassign this bug :-).

My Linux machine is newly refreshed with Redhat 7.2 (thanks to Sarah) and I'll
be building and maybe testing there soon.

But I have one more question:  have you unchecked the "always ask me" box for
your helper app entry for this mime type?

It appears you have (otherwise, you'd see the "Downloading..." dialog asking
whether to open with the helper app or save to disk.  Do you get different
behavior if you check that box?

After examining this code once again, it looks OK.  We go through
OnStartRequest, which calls LaunchWithApplication.  That routine sees we haven't
opened the progress dialog so it calls ShowProgressDialog and returns.

Next, the OnStopRequest for the pdf file comes in.  That triggers a call to
ExecuteDesiredAction, which doesn't do anything because the progress dialog
hasn't come to life yet.

Then, the progress dialog appears, and (immediately before that), calls
SetWebProgressListener.  That code detects that the OnStopRequest has already
fired, so it sends out a OnStateChange call to the dialog and then calls
ExecuteDesiredAction.

I don't see anywhere that the mMIMEInfo.preferredAction attribute can change.

The first thing to do is put a breakpoint in SetPreferredAction to see if that
is getting called.  If it isn't, then somebody's stomping on the object and we
need to set a watch breakpoint on the preferredAction field.

But wait a minute...onStateChange() in nsProgressDlg will call
processEndOfDownload() which likely calls closeWindow() which likeliy calls
window.close() which might trigger release of some xpconnect objects, including
the nsHelperAppLauncher and its member nsMIMEInfo.  *That* could be a problem;
I'm not sure what other references there are to the helperAppLauncher.

How about adding a printf to the nsHelperAppLauncher and/or nsMIMEInfo dtor?




Another idea: Comment out the call to OnStateChange in SetWebProgressListener. 
That will leave the progress dialog hanging, but if your helper app gets called,
then that's a sign that what's happening downstream from there is the cause of
our problem.
> Another idea: Comment out the call to OnStateChange in SetWebProgressListener. 

Made no difference. That is, the helper app still does NOT get invoked.
Is SetPreferredAction getting called to reset that field, and who's calling it 
if so?
SetPreferredAction is called twice, the first time with a 2, and the
second time with a zero. Both calls occur real early on, right as its
starting the download.

Attached are the call frames from the second call (when its passed zero).
Hopefully this will shed some light.
initDialog in nsHelperAppDlg.js is what's calling saveToDisk with the zero 
value (and this is getting called AFTER first call to SetPreferredAction).

Remember, I'm NEVER seeing the "do you want to open it or save it to disk" 
dialog.
It seems that the "what to do with the file" (what gets stored in
mPreferredAction in nsMIMEInfoImpl) is decided before nsHelperAppDlg's
initDialog is called. Yet when initDialog is called is calls saveToDisk(0)
which overwrites the information already stored in mPreferredAction.

Is this just a timing issue?

Are the two calls to SetPreferredAction occuring on different threads and its a
race condition?

To prove to myself that this is indeed what's going on I changed
SetPreferredAction so that it will only write into mPreferredAction if the
contents are zero (the ctor sets this to zero so we should be safe).

Now things are kind of working for me. I click on a PDF link, and I get
the "save to" dialog, I click on OK and I get the progress dialog, and
when the download is complete my helper app is fired up. The only part
I'm not sure about is that I never see the "do you want to open it or
save it" dialog. And yes, I went and clicked on the "reset" button in
the Helper Applications prefs panel.

Here's some tracing. I put a printf in the two nsMIMEInfoImpl ctors and
also in SetPreferredAction. Gee, we create more nsMIMEInfoImpl's than I
expected, and there was a ton of them at Mozilla startup time. Is this
normal?

CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl noargs ctor (setting to 0)
CRB SetPreferredAction called with 2
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB SetPreferredAction called with 0
CRB Already set to 2 so NOT setting to 0
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
CRB nsMIMEInfoImpl with args ctor (setting to 0)
This is making my head hurt.

I can't understand why you don't see the "Download..." dialog asking whether to
open with the helper app vs. save to disk.  The only explanation is that the
file appears to be an executable.

It looks like nsLocalFileUnix simply tests for the executable bit in the file
mode, which is always set.  That seems like it would be a problem on all unixes,
though.  Oh, I know.  I'll bet that implementation returns false if the file
doesn't exist yet, and that's what's different about your environment.

But why is this dialog even being shown?  If you have a helper app registered,
and you've unchecked the "always ask me" box, then it won't.  Maybe you didn't
uncheck that box?

I think that's what's happening.  Something is different on your system so that
the download happens fast enough that the file is created and seems to be
executable.  So we force it to save-to-disk.  Maybe we need a new
"WillBeExecutedDirectlyOrIndirectlyAndIsCapableOfDoingHarmWhenWeCallLaunch"
method (which is what we're really testing for by using IsExecutable).

I'm not sure what to do about this.  Anybody have ideas?  We could just avoid
the IsExecutable check on Unix.

BTW, lots of mime infos are created early on to fill the "cache" for default
types like text/html, xul, images, etc.
> I think that's what's happening.  Something is different on your system
> so that the download happens fast enough that the file is created and
> seems to be executable.  So we force it to save-to-disk. 

BINGO!!

access() is getting called with the temporary PDF file name and X_OK. OpenVMS 
returns 0 (success) and so the launcher code thinks the file is an executable 
and so forces a "save to disk". I hacked my access so that it would return -1 
for the temporary PDF file and then I saw the "do you want to open it or save 
it" box. **

On OpenVMS access(foo,X_OK) means "do I have execute access to foo?" and most 
certainly does NOT mean "is foo an executable file?". On UNIX does X_OK mean 
that the file is an executable? I think this test in the applauncher code is 
WRONG.


** the "open it or save it" box had #1 and #2 all over the place, instead of PDF 
and the name of the helper app. BUT, after I clicked on ADVANCED and came back 
to the dialog, most of the #1 and #2 strings were replaced with the correct 
information. Sounds like another timing problem to me!
This is how my "save it or open it" box looked.
> On UNIX does X_OK mean that the file is an executable?

No, it means "do I have execute permissions", just like on VMS....

The code in question is
http://lxr.mozilla.org/seamonkey/source/embedding/components/ui/helperAppDlg/nsHelperAppDlg.js#185

This calls .isExecutable() on the nsIFile.

Now the IsExecutable impls do the following:

Windows/OS2 -- check whether the file extension is in a list of "executable"
               extensions
MacOS -- check with LaunchServices on OSX or check the file type is in a list of
         executable file types ('APPL' and the like) on Classic
Unix (includes VMS) -- "access(mPath.get(), X_OK) == 0"

So this sounds like a bug in nsLocalFileUnix to me...
It's not a bug. I've encountered this misconception on unix before.

If you don't own the file, then you can't get access information on it. 
If you own the file, you *can* get access info on it.


--pete
BTW: if you are running as root, every file will return X_OK = 0.
Thus causing ::IsExecutable to return true.

--pete
Pete: On what Unix do you see this?

access(2) returns -EACCESS if you don't have the access permissions you're
asking about, but it has nothing to do with file ownership:

: trunk; id
uid=500(shaver) gid=500(shaver) groups=500(shaver)
: trunk; strace -etrace=access access -w /etc/passwd
access("/etc/passwd", W_OK)             = -1 EACCES (Permission denied)
: trunk; strace -etrace=access access -x /etc/passwd
access("/etc/passwd", X_OK)             = -1 EACCES (Permission denied)

Boris: if there's a bug here on nsLocalFileUnix, I don't know how you expect to
fix it.  I don't know of any portable way to test whether a file would execute
if exec(2)d, without actually trying it.  Why are we not allowing "open from
location" on the other platforms, for that matter?  IE does, and as an
occasional IE user I find it handy at times.
Pete: wrong again, I fear:

[root@split root]# id
uid=0(root) gid=0(root)
groups=0(root),1(bin),2(daemon),3(sys),4(adm),6(disk),10(wheel)
[root@split root]# strace -etrace=access access -x /etc/passwd
access("/etc/passwd", X_OK)             = -1 EACCES (Permission denied)

Are there Unix variants that have different access(2) semantics?  I'd be
surprised, I'll admit.
Shaver, I see it on RH Linux and FreeBSD.

access will return X_OK=0 on a file w/ permissions of 644 if you are root.

Try it. I shit you not . . .  

--pete

> I don't know of any portable way to test whether a file would execute
> if exec(2)d, without actually trying it.

Me neither.... otherwise I would have suggested it.

> Why are we not allowing "open from location"

See bug 68279 (that added the code in question).  There is a bug to get rid of
or change this check -- bug 106094.

The question remains.  What exactly _should_ the semantics of isExecutable() be?
#include <stdio.h>
#include <unistd.h>

int
main() {
  char *file = "foo";
  return printf("access = %d\n", access(file, X_OK));

}

Running a.out as root:

Thu Jan 24 
root@tigris
~/CC $ touch foo 
Thu Jan 24 
root@tigris
~/CC $ ls -l foo
-rw-r--r--    1 root     root            0 Jan 24 14:45 foo
Thu Jan 24 
root@tigris
~/CC $ gcc -g -Wall access.c 
Thu Jan 24 
root@tigris
~/CC $ ./a.out 
access = 0


same file as non-root user.

$ su petejc
[petejc@tigris CC]$ ./a.out 
access = -1
[petejc@tigris CC]$ 

The X_OR access test is a test for execute PERMISSION. Where does it say in any 
spec that its a test of whether of not you can exec() it?
> Shaver, I see it on RH Linux and FreeBSD.
> 
> access will return X_OK=0 on a file w/ permissions of 644 if you are root.
> 
> Try it. I shit you not . . .

I _did_ try it, above, but I'll repeat here:

[root@split root]# strace -etrace=access access -x /etc/passwd
access("/etc/passwd", X_OK)             = -1 EACCES (Permission denied)
[root@split root]# ls -l /etc/passwd
-rw-r--r--    1 root     root         1283 Jan  7 09:04 /etc/passwd

What do you get from that sequence, as root?  Ownership of the file doesn't seem
to matter, as well it should not:

[root@split root]# strace -etrace=access access -x /tmp/shaverfile
access("/tmp/shaverfile", X_OK)         = -1 EACCES (Permission denied)
[root@split root]# ls -l /tmp/shaverfile
-rw-r--r--    1 shaver   shaver          0 Jan 24 14:41 /tmp/shaverfile
[root@split root]# 
OK, now things are getting wierd:

[root@split tmp]# ./acc
access = -1
[root@split tmp]# su shaver
: tmp; ./acc
access = -1
: tmp; ls -l foo
-rw-r--r--    1 shaver   shaver          0 Jan 24 14:44 foo

What does strace (or the FreeBSD equivalent) report for you when you run your
little program?

This is on Linux bro . . .

Thu Jan 24 
root@tigris
~/CC $ strace -etrace=access access -x /etc/passwd
access("/etc/passwd", X_OK)             = 0
Thu Jan 24 
root@tigris
~/CC $ 
Thu Jan 24 
root@tigris
~/CC $ ls -l /etc/passwd
-rw-r--r--    1 root     root         1817 Jan 12 16:25 /etc/passwd
Thu Jan 24 
root@tigris
~/CC $ su petejc
[petejc@tigris CC]$ strace -etrace=access access -x /etc/passwd
access("/etc/passwd", X_OK)             = -1 EACCES (Permission denied)
[petejc@tigris CC]$ glib-config --version
1.2.6

Ah, damn.  I forgot about the difference between su and su -.  I get the "0"
behaviour as root too, though of course the file isn't executable (bash: ./foo:
permission denied).

I can picture the exception in the syscall code, too.  Blah.  Sorry about that.

Anyway, I'm still pretty sure that it has nothing to do with file ownership. 
access(2) is clearly fraught with peril, but I don't think there's anything else
appropriate.  
You can have execute permission on a jpg file, but that doesn't mean you can 
exec() it.

ITS THE WRONG TEST!!!!
Yes, Colin, we know.  But you might well be able to execute a JPEG file, if the
system is configured "correctly".  (My laptop currently is, for example.)

What replacement test do you propose, other than actually attempting to execute it?
Why not change the permissions of the temp file to 644 in the js logic?

Then this bug will only happen is you are running mozilla as su.  ;-)

Attempting to execute the file is not a good thing. Imagine testing
isExecutable() from a loop that is reading recursively all the dir entries.  Ouch!

--pete







Why are we even doing this test (beside the fact that it would appear to be 
impossible to test correctly)?

The server's told us the mime type, and we know we have a helper app to handle 
that mime type. So popping up the "open it or save to disk" dialog makes sense. 
Why would we ever want to "force to disk" just because we think we can exec() 
the file? I just don't understand what the problem is that we're trying to solve 
here.
> Why are we even doing this test

Bug 68279. We need to warn the user about running possibly-malicious code.

> Why not change the permissions of the temp file to 644

The file is not created in the js and it's created as 644 (or 600 by now, I
think).  This is why this is _not_ a problem on Unix builds...

> Bug 68279. We need to warn the user about running possibly-malicious code.

But we're not going to RUN it. We're going to feed it in to the helper app as 
data. So what if the file is "executable" (whatever that means)?

Maybe my helper app is hexedit and I want to feed something that's "executable" 
into it?

Maybe my helper app is a virus checker...
Blocks: 122422
bug 122422 is a very similar problem on BSDI....
This patch fixes the problem for root attachments on BSDI.  The nice thing is
that it applies to the javascript source and does not require a recompile.
I just submitted a javascript patch for attachments by root on BSDI.

As for the current isExecutable code that uses access(), it seems access() is
the wrong function call for this.  access() tests permissions more than file
flags.  stat() with S_IXUSR seems the proper test to use.
Comment on attachment 67203 [details] [diff] [review]
Fixes attachment problem for root on BSDI

This patch isn't right.  If I apply this on windows, and click on a .exe file,
I get the helper app dialog.  If I choose to "open with" my virus checker, then
clicking on OK doesn't run the virus checker, it runs the downloaded .exe. 
That's no good.
Attachment #67203 - Flags: needs-work+
Plan is to make the "IsExecutable()" check here (and a similar spot elsewhere) 
only on non-Unix platforms.
Target Milestone: --- → mozilla1.0
*** Bug 122422 has been marked as a duplicate of this bug. ***
Keywords: nsbeta1
IsExecutable() is only part of the problem. I still think there's a timing 
problem here. Here's what I did. 

I hacked access() so that it would always return -1 when passed a mode of X_OK. 
Now my helper app gets created, but NEVER THE FIRST TIME. The first time (of 
each Mozilla session) I always get the dialog box that I posted in attachment 
66276 [details]. But second and subsequent attempts are fine.
nsbeta1-, per Nav triage team, helpwanted.
Keywords: nsbeta1 → helpwanted, nsbeta1-

        Attachment #67203 -
        Attachment is obsolete: true

    
    

    
  
What about Mac?

I think we should do the isExecutable test if both these conditions are met:

1. The platform implements isExecutable() based on the file extension (or
content type, maybe); that is, it can be done before the file is created.
2. Launching the file is potentially harmful (i.e., will execute directly and is
capable of destroying the user's data).

I'm not sure what the deal is on the Mac.  A cursory examiniation of
nsLocalFileMac::IsExecutable and ::Launch (at
http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileMac.cpp) isn't
enough for me to answer those questions.

    
    

    
  
At least on MacOS Classic, the file needs to actually exist (IsExecutable()
calls GetFileType() which will throw NS_ERROR_FILE_NOT_FOUND if the file is not
found).  I can't make sense of the OSX code without knowing more about the
mac-specific functions it uses.

    
    

    
  
See also bug 116938 for a possible alternate solution to this problem....  We
should decide on exactly what we're doing and that bug highlights some security
issues the current code has even on Windows.

    
    

    
  
And the Mac code returns "false," then?  Which is a lie, for some files.  So if
::Launch can run the file, then there might still be a problem in this case.

Does the IsExecutable() check ever pass on Mac?

I'm tempted to OK this patch, since it keeps the same behavior on Windows, is
safe on Linux (because Launch() will never launch the problem directly), and is
safe on Mac, because IsExecutable() always fails, anyway.

Does that sound right?



    
    

    
  
That sums up the current state of things, yes.

    
    

    
  
Comment on attachment 69967 [details] [diff] [review]
Patch to do the test only on Windows

r=law

My new motto: It doesn't have to be perfect, it just has to be better.

        Attachment #69967 -
        Flags: review+

    
    

    
  
Even in M0.9.9 where most of this is now fixed, the first time (of each Mozilla 
session) I always get the dialog box that I posted in attachment 66276 [details]. But 
second and subsequent attempts are fine.

    
    

    
  
Mass-moving all Navigator team 1.0 nsbeta1- bugs to 1.1
Target Milestone: mozilla1.0 → mozilla1.1

    
    

    
  
I can report this bug is fixed for root users of BSD/OS 4.2 with release 0.9.9.

    
    

    
  
Mozilla 0.9.9
Mozilla/5.0 (X11; U; OpenVMS COMPAQ_AlphaServer_DS10_466_MHz; en-US; rv:0.9.9)
Gecko/20020309

I can not get the Adobe PDF helper application to work.  The first time I try, I
get the broken dialog.

On subsequent tries, the actual file is downloaded to my default login
directory.  and then an error message appears sometimes, sometimes just a blank
screen.

The error message popup is: "/tmp/OVMS_73_DEC_TPU_REF.PDF could not be opened,
because an unknown error occured.
Sorry about that, Try saving to disk first and then opening the file."

I have verified that the PDF helper application is registered and that when run
from the DCL command line it will display the PDF file.

I put some diagnostics in the command file, and it appears that it is not being
run at all.

This is also outputted on the terminal session that Mozilla is being run from:

*access("/tmp/rkpi3gli.PDF",1) = -1 (xok fix)
*access("/tmp/c52vc9an.pdf",1) = -1 (xok fix)
*access("/tmp",0) = 0
*access("/tmp/uf45ank9.pdf",1) = -1 (xok fix)
*access("/tmp",0) = 0
*access("/tmp/o52zc5an.pdf",1) = -1 (xok fix)
*access("/tmp",0) = 0
*access("/tmp/kpab49ur.pdf",1) = -1 (xok fix)
*access("/tmp",0) = 0
*access("/tmp/9ab05yrk.pdf",1) = -1 (xok fix)
*access("/tmp",0) = 0
*access("/tmp/4tinspyv.pdf",1) = -1 (xok fix)
*access("/tmp",0) = 0
*access("/tmp/6749incx.pdf",1) = -1 (xok fix)
*access("/tmp",0) = 0
*access("/tmp/ezchybol.PDF",1) = -1 (xok fix)
*access("/tmp",0) = 0
*access("/tmp/gtu3k96v.PDF",1) = -1 (xok fix)
*access("/tmp/0hyf4dqj.PDF",1) = -1 (xok fix)
*access("/tmp",0) = 0
*access("/tmp/bw12bsd6.PDF",1) = -1 (xok fix)
*access("/tmp",0) = 0
*access("/tmp/zwp6fcpm.PDF",1) = -1 (xok fix)
*access("/tmp",0) = 0

    
    

    
  
What is the name of the command file you are using as your helper app? Can you 
search for that in the file MIMETYPES.RDF (in the _MOZILLA tree in your 
SYS$LOGIN). Also search for "pdf". I'd like to see what you have.

    
    

    
  
The remaining problem in this report is that SOMETIMES the downloading dialog 
box doesn't appear correctly - see attachment 66276 [details] - here's the URL in case
http://bugzilla.mozilla.org/attachment.cgi?id=66276&action=view

There is another report of this problem in bug 81190. That bug has been marked 
as a duplicate of bug 77850, but I don't think it is.

I'm convinced there is a real timing problem here.

    
    

    
  
Bug 136023 is reporting the same problem, that sometimes the download dialog box 
doesn't appear correctly, and when this happens, the file is not renamed from 
its temp name.

The x_ok problem was just one problem. There's another (timing, I think) related 
problem still lurking.

    
    

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

    
    

    
  
Changing the summary from "Helper apps don't start up" to "You have chosen to 
download a file of type: "#1" [#2] from" since it more accurately describes the 
remaining problem here which still needs fixing.

Attachment 66276 [details] (Dialog box with missing information) shows how this bug 
presents itself to the user.
Summary: Helper apps don't start up → You have chosen to download a file of type: "#1" [#2] from

    
    

    
  
Please, when someone fix this bug, take a look at bug 95828 . It is essentially
the some bug as this one, but on Linux platform.

    
    

    
  
I think that bug 150145 is another instance of this problem. I will confirm once
I know for sure.

As a workaround, if all you want to do is save a file to disk, shift-click on
the file and this should immediately bring up the "save as" dialog.

    
    

    
  
I don't know if this means anything, but I thought I'd mention it in case it
does mean something to someone. When the infamous 'You have chosen to download a
file of type: "#1" [#2]' dialog appears, its always in the very top left of the
screen, whereas when the correct downloading box appears its in the center of
the screen.

    
    

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

    
    

    
  
Wahoo. I've finally found this damn bug. Its in the OpenVMS "look like UNIX"
code, and not in the Mozilla code itself. For the record, when access() was
called to make the X_OK check, we were returning -1 to say that the file is not
executable, but NOT setting errno to EACCES. isExecutable was then returning
NS_ERROR_FILE_TARGET_DOES_NOT_EXIST to nshelperappdlg.js, and things went
downhill from there.

If anyone wants a fixed image for Mozilla or CSWB V1.0 let me know via mail.
I'll also see if I can get this fixed image up on the OpenVMS web site.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
QA Contact: sairuh → nobody
Whiteboard: se-radar
Keywords: qawanted
Product: Core → Core Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: