351950 - nsLocalFileUnix has "access"(system call) problem on Solaris

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[Alfred Peng]

The behavior of system call access on Solaris is different with Linux. Here is a snippet from the manual of access:
If the process has appropriate privileges, an implementation may indicate  success for X_OK even if none of the execute file permission bits are set.

So for a root account, the return value for access on Solaris is different with Linux. Replace access with stat to solve the problem.

Another issue hasn't been addressed: comment 5 for bug 169506:
Why not just replace access with stat universally?

Comment 1

[Alfred Peng]

Attachment: Patch v1

Comment 2

[Doug Turner (:dougt)]

brendan, why access() not stat()?

Comment 3

[Brendan Eich [:brendan]]

Because access is faster -- no stat buf to copy out of kernel space to userland.

/be

Comment 4

[Alfred Peng]

The difference behaviors influence the file opening dialog for Firefox under Solaris. With root account or account having root privilege, the dialog disables the "open with" option.

Is there any reference for the access system call to make the implementations coherent?

Comment 5

[Alfred Peng]

dougt, to provide the same functionality with Linux, I made this patch. What's your opinion on it?

Besides that, I'm also a little confused by the evaluation for "isexecutable" here:
http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in#430

Since we always make a temp file with permission 600 before we really open it:
http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1610,
is that redundant for us to evaluate the executable permission again?

Comment 6

[Alfred Peng]

dougt, any advice on the patch? It changes the code for Solaris and makes the output of nsLocalFile::Exists consistent with Linux.

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 237500 [details] [diff] [review]
Patch v1

This is fine.  Feel free to request which ever branches you want to land this on.

Comment 8

[Alfred Peng]

Thanks.
Checking in xpcom/io/nsLocalFileUnix.cpp;
/cvsroot/mozilla/xpcom/io/nsLocalFileUnix.cpp,v  <--  nsLocalFileUnix.cpp
new revision: 1.135; previous revision: 1.134

Comment 9

[Alfred Peng]

Comment on attachment 237500 [details] [diff] [review]
Patch v1

This patch makes the behaviour of nsLocalFile::Exists on Solaris consistent with Linux. It only affects the Solaris platform and with low risk. Ask for the approval for 1.8 branch.

Comment 10

[Jay Patel [:jay]]

Comment on attachment 237500 [details] [diff] [review]
Patch v1

Approved for 1.8 branch, a=jay for drivers.

Comment 11

[Alfred Peng]

Checking in xpcom/io/nsLocalFileUnix.cpp;
/cvsroot/mozilla/xpcom/io/nsLocalFileUnix.cpp,v  <--  nsLocalFileUnix.cpp
new revision: 1.126.8.2; previous revision: 1.126.8.1
done

Comment 12

[Tony Chung [:tchung]]

hi alfred, since this is solaris based fix, could you test it and verify for us on the 1.8 branch?  If the fix is valid, please change the keywords field from "fixed1.8.1.2" to "verified1.8.1.2"

Thanks!

Comment 13

[Alfred Peng]

Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.8.1.2pre) Gecko/20070131 BonEcho/2.0.0.2pre.

=>Verified.

Comment 14

[Alfred Peng]

I just put a binary file and a .xls file on our internal web. It works fine with root user. Do we still need a test case for that?