Can crash in engine installing to folder without sufficient permissions

VERIFIED FIXED in mozilla1.0

Status

Core Graveyard
Installer: XPInstall Engine
--
major
VERIFIED FIXED
16 years ago
2 years ago

People

(Reporter: Syd Logan, Assigned: dprice (gone))

Tracking

Trunk
mozilla1.0
x86
Linux

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2] [ETA 4-23])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

16 years ago
A patch on a separate bug http://bugzilla.mozilla.org/show_bug.cgi?id=89436
keeps the Linux/Unix stub and blob installers from running into this, but we
still need to address it in the backend in case other applications using XPI
don't account for it.
(Reporter)

Comment 1

16 years ago
Created attachment 63552 [details] [diff] [review]
patch based on Dan's input, over to Dan for refinement as necessary
(Reporter)

Updated

16 years ago
Severity: normal → major
Keywords: nsbeta1+
Target Milestone: --- → mozilla0.9.9
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.9 → mozilla1.0
(Reporter)

Comment 2

16 years ago
adt2 per adt traige
Whiteboard: [adt3]
(Reporter)

Updated

16 years ago
Attachment #63552 - Flags: review+
(Reporter)

Comment 3

16 years ago
--> dprice to get sr=, and adt approval for this crasher
Assignee: dveditz → dprice
Status: ASSIGNED → NEW
(Assignee)

Comment 4

16 years ago
Created attachment 78866 [details] [diff] [review]
updated patch

there was some drifting in the code.  This patch is a unified diff that's up to
date.
Attachment #63552 - Attachment is obsolete: true

Comment 5

16 years ago
Please get a module owner level review then ping me again for a sr. Thanks!

Comment 6

16 years ago
Comment on attachment 78866 [details] [diff] [review]
updated patch

instead of !NS_SUCCEEDED(rv), I think you should use

NS_FAILED(rv)
(Assignee)

Comment 7

16 years ago
Created attachment 79389 [details] [diff] [review]
patch 2 

Changed to using NS_FAILED()
Attachment #78866 - Attachment is obsolete: true
Comment on attachment 79389 [details] [diff] [review]
patch 2 

>-    if (bTryProfileDir)
>+    if (bTryProfileDir && !nsSoftwareUpdate::GetProgramDirectory())

The !nsSoftwareUpdate::GetProgramDirectory() is confusing and needs
commenting that it basically means "not in the install wizard".

>-        if (!dirSvc) return NS_ERROR_FAILURE;
>+        if (!dirSvc || NS_FAILED(rv)) return NS_ERROR_FAILURE;
>         dirSvc->Get(NS_APP_USER_PROFILE_50_DIR, NS_GET_IID(nsIFile),
>                     getter_AddRefs(iFile));

Seems strange to me that we'd check the return value of do_GetService()
in addition to dirSvc not being null (which many argue is a redundant
check) and then not check the status of the Get() call which *might* fail
if some embedding app didn't define that target.

>         if (!nsSoftwareUpdate::GetLogName())
>             rv = iFile->Append(INSTALL_LOG);
>         else
>             rv = iFile->Append(nsSoftwareUpdate::GetLogName());

and if it does fail, then here we go boom.
Attachment #79389 - Flags: needs-work+
(Assignee)

Comment 9

16 years ago
Created attachment 79710 [details] [diff] [review]
patch 3

Adding additional checks
Attachment #79389 - Attachment is obsolete: true
(Reporter)

Comment 10

16 years ago
Comment on attachment 79710 [details] [diff] [review]
patch 3

r=syd
Attachment #79710 - Flags: review+
Comment on attachment 79710 [details] [diff] [review]
patch 3

>-    if (bTryProfileDir)
>-    {
>+    if (bTryProfileDir && !nsSoftwareUpdate::GetProgramDirectory())
>+    {   
>+        // Not in the stub installer
>         // failed to create the log file in the application directory
>         // so try to create the log file in the user's profile directory

That's not that great a comment, it won't be clear to anyone that
it has anything to do with the cryptic !nsSoftwareUpdate::blah().
Short of adding an in-line nsSoftwareUpdate::InWizard() for readability
that evaluates to the above (since we use this idiom everywhere we
should do that eventually) your added comment would make more sense
*after* the existing comments:
	  // but only if we're not in the stub installer

sr=dveditz if you make the comment change.
Attachment #79710 - Flags: superreview+
(Assignee)

Updated

16 years ago
Keywords: adt1.0.0

Comment 12

16 years ago
Pls land this on the trunk today. Once Jimmy has verified it on the trunk, we
will consider for inclusion on the 1.0 branch.
Whiteboard: [adt3] → [adt3] [ETA Needed]
(Reporter)

Comment 13

16 years ago
Once landed in the trunk, please mark "FIXED" so that testing on the trunk is
commenced, and track this bug for verified, nominate adt1.0.0 once you get the
verified.
(Assignee)

Updated

16 years ago
Whiteboard: [adt3] [ETA Needed] → [adt3] [ETA 4-23]
(Assignee)

Comment 14

16 years ago
landed on the trunk friday, marking fixed
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 15

16 years ago
Build: 2002-04-22-10-trunk(LINUX)

Engine does not crash when installing to folder without sufficient permissions.
 As expeected, file(s)/folder(s) are not created in folder without sufficient
permissions.  As expected, Install.log is created in .mozilla directory.  Error
-202 (ACCESS_DENIED) is reported from Install.log which appears to be accurate.

Marking Verified for TRUNK!
Status: RESOLVED → VERIFIED

Comment 16

16 years ago
adding adt1.0.0+.  Please check this into the branch as soon as possible and add
the fixed1.0.0 keyword after getting drivers approval.
Keywords: adt1.0.0 → adt1.0.0+, approval
Whiteboard: [adt3] [ETA 4-23] → [adt2] [ETA 4-23]

Comment 17

16 years ago
Comment on attachment 79710 [details] [diff] [review]
patch 3

a=scc for checkin to the mozilla 1.0 branch
Attachment #79710 - Flags: approval+
(Assignee)

Updated

16 years ago
Keywords: fixed1.0.0

Comment 18

16 years ago
Build: 2002-04-25-08-1.0.0(LINUX)

Looks good on BRANCH as well.  Adding keyword, verified1.0.0.
Keywords: fixed1.0.0 → verified1.0.0
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.