Closed Bug 118216 Opened 23 years ago Closed 22 years ago

Can crash in engine installing to folder without sufficient permissions

Categories

(Core Graveyard :: Installer: XPInstall Engine, defect)

x86
Linux
defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.0

People

(Reporter: slogan, Assigned: dprice)

Details

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

Attachments

(1 file, 3 obsolete files)

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.
Severity: normal → major
Keywords: nsbeta1+
Target Milestone: --- → mozilla0.9.9
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.9 → mozilla1.0
adt2 per adt traige
Whiteboard: [adt3]
Attachment #63552 - Flags: review+
--> dprice to get sr=, and adt approval for this crasher
Assignee: dveditz → dprice
Status: ASSIGNED → NEW
Attached patch updated patch (obsolete) — Splinter Review
there was some drifting in the code.  This patch is a unified diff that's up to
date.
Attachment #63552 - Attachment is obsolete: true
Please get a module owner level review then ping me again for a sr. Thanks!
Comment on attachment 78866 [details] [diff] [review]
updated patch

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

NS_FAILED(rv)
Attached patch patch 2 (obsolete) — Splinter Review
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+
Attached patch patch 3Splinter Review
Adding additional checks
Attachment #79389 - Attachment is obsolete: true
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+
Keywords: adt1.0.0
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]
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.
Whiteboard: [adt3] [ETA Needed] → [adt3] [ETA 4-23]
landed on the trunk friday, marking fixed
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
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
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.0adt1.0.0+, approval
Whiteboard: [adt3] [ETA 4-23] → [adt2] [ETA 4-23]
Comment on attachment 79710 [details] [diff] [review]
patch 3

a=scc for checkin to the mozilla 1.0 branch
Attachment #79710 - Flags: approval+
Keywords: fixed1.0.0
Build: 2002-04-25-08-1.0.0(LINUX)

Looks good on BRANCH as well.  Adding keyword, verified1.0.0.
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: