Callback application should be locked when doing updates from the service

RESOLVED FIXED in mozilla12

Status

()

Toolkit
Application Update
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

unspecified
mozilla12
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
When updates run through the maintenance service, we are no longer passing callback information to the maintenance service (as of a recent security bug).  So when the maintenance service runs the updater elevated it does not have the callback information.

For that reason the callback application is not being locked during update, so a user could start it during an update.  if this happens the user will get an error saying that they should download the complete MAR.  It would be better to give the error we do without the service that the file is in use.

To do this;

> callbackFile = CreateFileW(argv[callbackIndex],
>                            DELETE | GENERIC_WRITE,
>                            // allow delete, rename, and write
>                            FILE_SHARE_DELETE | FILE_SHARE_WRITE,
>                            NULL, OPEN_EXISTING, 0, NULL);

This should be done from the unelevated callback though since the maintenance service does not have information about the callback application.
(Assignee)

Comment 1

6 years ago
Created attachment 582354 [details] [diff] [review]
Patch v1.

This makes it so you can't use firefox while the service is applying the update.
Attachment #582354 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 2

6 years ago
Created attachment 582371 [details] [diff] [review]
Patch v2.

Alternate more safe way of doing locking that won't cause problems as per discussions.
Attachment #582354 - Attachment is obsolete: true
Attachment #582354 - Flags: review?(robert.bugzilla)
Attachment #582371 - Flags: review?(robert.bugzilla)
Comment on attachment 582371 [details] [diff] [review]
Patch v2.

Yes! Some manual testing would be a good thing but I am as certain as can be this is the best way to do this.
Attachment #582371 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 4

6 years ago
Created attachment 582490 [details] [diff] [review]
Patch v3.

Patch v2 was fine but it didn't handle when there was an error for launching the callback. In those cases the callback would be launched as session 0 in addition to the user session. I put the check inside the launch callback app function to catch all past, present and future calls to this function.
Attachment #582371 - Attachment is obsolete: true
Attachment #582490 - Flags: review?(robert.bugzilla)
Comment on attachment 582490 [details] [diff] [review]
Patch v3.

Makes sense. Would be good to go through these different code paths but better to do so when fully awake.
Attachment #582490 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 6

6 years ago
I tested one error condition which was how I seen a firefox in session 0 with the old patch, the new patch fixes it though.  The other error conditions are all the same type of code with launching callback and early return
(Assignee)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.