Closed Bug 711505 Opened 9 years ago Closed 9 years ago

Callback application should be locked when doing updates from the service

Categories

(Toolkit :: Application Update, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Patch v1. (obsolete) — Splinter Review
This makes it so you can't use firefox while the service is applying the update.
Attachment #582354 - Flags: review?(robert.bugzilla)
Attached patch Patch v2. (obsolete) — Splinter Review
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+
Attached patch Patch v3.Splinter Review
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+
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
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.