Last Comment Bug 351950 - nsLocalFileUnix has "access"(system call) problem on Solaris
: nsLocalFileUnix has "access"(system call) problem on Solaris
Status: VERIFIED FIXED
[need testcase]
: verified1.8.1.2
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: Sun Solaris
: -- normal (vote)
: ---
Assigned To: Alfred Peng
:
:
Mentors:
Depends on: 542738
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-09 09:13 PDT by Alfred Peng
Modified: 2010-01-28 02:38 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (1.04 KB, patch)
2006-09-09 09:17 PDT, Alfred Peng
doug.turner: review+
doug.turner: superreview+
jaymoz: approval1.8.1.2+
Details | Diff | Splinter Review

Description Alfred Peng 2006-09-09 09:13:20 PDT
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 2006-09-09 09:17:40 PDT
Created attachment 237500 [details] [diff] [review]
Patch v1
Comment 2 Doug Turner (:dougt) 2006-09-11 09:10:01 PDT
brendan, why access() not stat()?
Comment 3 Brendan Eich [:brendan] 2006-09-11 10:10:46 PDT
Because access is faster -- no stat buf to copy out of kernel space to userland.

/be
Comment 4 Alfred Peng 2006-09-12 04:40:51 PDT
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 2006-09-17 01:06:56 PDT
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 2006-12-05 04:37:52 PST
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) 2006-12-05 08:03:06 PST
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 2006-12-05 19:59:49 PST
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 2006-12-06 00:08:54 PST
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] 2006-12-27 14:57:30 PST
Comment on attachment 237500 [details] [diff] [review]
Patch v1

Approved for 1.8 branch, a=jay for drivers.
Comment 11 Alfred Peng 2006-12-27 21:20:32 PST
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] 2007-02-07 15:36:49 PST
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 2007-02-07 21:36:02 PST
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 2007-02-07 21:37:33 PST
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?

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