Closed Bug 58371 Opened 22 years ago Closed 21 years ago

if /tmp/preferences.js exists and is not writable by the user, we fail to migrate

Categories

(Core Graveyard :: Profile: Migration, defect, P1)

x86
Linux
defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.8

People

(Reporter: sspitzer, Assigned: dbragg)

Details

(Whiteboard: [rtm-] relnote-user)

Attachments

(6 files)

I found this out the hard way.

on linux, everyone shares /tmp.  I was logged in as user1 to work on a migration
bug.  (the bug was we were failing to migrate).  when we failed to migrate, we
left behind the users 4.x preferences.js file.  in the migration code, we copied
it to /tmp.

then I logged in as user2 and tried to migrate.  because /tmp/preferences.js
existed, I failed to migrate.  as soon as I removed /tmp/preferences.js (owned
by user1) I could migrate user2.

should be easy to fix.  could happen on multi users systems.
Good catch Seth.  Not being a Unix expert, I didn't realize that the /tmp
directory was shared amongst all users.  I would think this problem would be
fairly pervasive on Unix.  What is the normal way that programs deal with this?
Do they create unique directories off of /tmp?

This would be a problem even if the migration doesn't error out.  If two users
were trying to migrate at the same time they'd run into this problem.
we use MakeUnique() or CreateUnique() or whatever to make sure we get a unique
file name if we are in /tmp.

we do this for mail when creating temporary files when sending email.

are there other files we write to /tmp during migration?

dbragg, you want to take this from me?

Yeah. I'll take it.  I think this is more serious than I thought.  The file is
NOT deleted at the end of a successful migration.  The file is ALWAYS left
behind after a migration.  This is not a problem on Windows (all platforms) and
Mac but on Unix it's a problem.

Investigating.
Assignee: sspitzer → dbragg
Ignore my last comment. The file IS deleted upon successful migration so this
bug will only happen if there's an error or if two people are migrating at the
same time.
Status: NEW → ASSIGNED
Unfortunately this isn't as easy as it might seem.  The profile migration code
is still using nsFileSpec and nsFileSpec has no method for copying a file to a
new name.  All it has is CopyToDir.

The fix would be to convert the PrefFile4x FileSpec to an IFile then do a
MakeUnique on the target and use nsIFile's CopyTo function to rename the file in
mid-copy.  Working on a patch.

sspitzer, what do you think the likelihood (and thus priority) of this should be?
Ok, I think I've got it.  Had to do the nsIFile dance I mentioned in the
previous comment.  I tested the following diff on Windows and Unix.  (My Mac is
out of commission for now)  I don't have the multiple account test scenario of
sspitzer so sspitzer, if you could review (and hopefully test) the attached
patch I'd be most grateful.
Attaching new diff.  The leaf name char* systemTempLeaf has memory allocated in
the GetLeafName call.  Need to free that up using nsMemory::Free(systemTempLeaf);

Attaching new diff.
Upping the priority and severity and adding rtm keyword for PDT review.
Severity: normal → critical
Keywords: rtm
Priority: P3 → P1
Whiteboard: [rtm need info] need reviews
I've already requested reviews and super reviews for this patch.  sspitzer has
suggested this might not be serious enough for rtm inclusion.  sspitzer, please
comment.
After a review with dveditz I changed the char* systemTempLeaf to an
nsXPIDLCString so that we don't have to worry about leaking it if one of the
later function calls fails.  Adding new diff and dveditz to the CC list.
r=dveditz for the 11/01 patch
this fix isn't quite right.  if I had multiple user migrating they could still
encounter this bug.  (it would just be harder.)

process 1					process 2
MakeUnique()
				.
.
					MakeUnique()	
CopyTo()
				.
.
					CopyTo()

it's good we delete the prefs file, but the creation of the uniq prefs file in
/tmp is not atomic.

this has come up before.  I think the only CreateUnique() (on nsIFile) is atomic
(thanks to robert).

cc'ing robert and brendan.

dbragg, sorry for the delay on reviewing this.

Adding myself to the cc list so I can give the final patch a super review
Dan and I both felt that the chances of two people running at exactly the same
time and running into that condition was pretty remote plus we didn't know about
CreateUnique.

Ok, this changes things.  Working on new patch using CreateUnique.  
Time is running out and this looks important.  Is the patch ready yet?

A smaller patch that just ensures the file is always deleted may be safer at
this point...
I could put a delete call in the destructor.
New patch using CreateUnique.  Due to the limitation (on windows anyway) of
CopyTo not being able to overwrite a file that already exists, I used
CreateUnique to create a unique directory in the system temp dir.  Then the
prefs.js file is simply copied to this unique directory.
comments

can you use 700 as the permission?  since it contains the users prefs.js, we
should try to keep other users for getting at it (even briefly during migration.)

can you name the directory "migrate" (when uniqified we'd get
"migrate-1","migrate-2", etc) instead of PREF_FILE_NAME_IN_4x?

having /tmp/migrate/prefs.js makes more sense to me than /tmp/prefs.js/prefs.js
Sure.  Do I need to post another patch?
the new patch works for me.  I've tested it out.

dbragg, thanks for putting up with the delays and comments.

r=sspitzer

mscott, sr=?
Whiteboard: [rtm need info] need reviews → [rtm need info] r=sspitzer, sr=?
No, thank YOU for taking the time to make such a thorough inspection.  Muy
importante at this stage!
Whiteboard: [rtm need info] r=sspitzer, sr=? → [rtm need info] need reviews
Didn't know about CreateUnique(), must have been added since we converted 
XPInstall over to nsIFile.  Don, maybe we should revisit our xpinstall 
MakeUnique() calls for any that might be made safer using CreateUnique() 
instead.
You must be reading my mind Dan.  The only problem with CreateUnique is that you
can't really use it for copying.  Not because of CreateUnique itself but because
CopyTo won't allow you to copy over an existing file.  A cool new feature might
be CopyUnique (although I'm not volunteering ;-)
sr=mscott. Thanks guys!
Better to fix CopyTo() to overwrite existing files.
It's not really CopyTo, it's the actual Windows call that won't overwrite.
Anyway, that's another discussion eh?

Marking rtm+ to put on PDT radar.
Whiteboard: [rtm need info] need reviews → [rtm+]
Per discussion with the PDT team this is an rtm-.  Thanks to everyone who help
on this bug!

We will be release noting this.  I'll add another comment for release not purposes.
Whiteboard: [rtm+] → [rtm-]
Adding relnoteRTM and CC:ing verah.

For the release notes:
On Linux systems if you are unable to migrate a Communicator profile check the
/tmp directory and remove any prefs.js that is found.
Keywords: relnoteRTM
note:  it would be named "preferences.js" and it could be in $TMPDIR if that
environment variable set, otherwise it would be in /tmp.

Whiteboard: [rtm-] → [rtm-] relnote-user
Using /tmp is a bad idea altogether. Unfortunately it looks rather common
with a quick grep over the mozilla tree.
I've filed bug 59808 on that.
why is /tmp bad?  would /tmp/<uniq> be any better?  please explain.

adding patch keyword.
Keywords: patch
Now that we're past N6 shipment I'd like to check this in.  I've merged changes
to the trunk with Seth's last patch.  The only difference is the change made to
fix bug 1.125 that Seth checked in on 10/14.  I guess the file I was originally
using was pulled between 1.124 and 1.125.

Posting the patch.
oops.  I didn't mean "fix for bug 1.125" I meant revision number 1.125.  Sorry.
Removing myself from list of cc's. This was release noted.
Keywords: nsbeta1
What's the deal with this bug? it has apparently had a patch since 11/27 but no 
reviews. I want it off my nsbeta1 radar one way or another: please check in, 
mark nsbeta1-, or assign to someone else to do the deed.
Target Milestone: --- → mozilla0.8
r=racham.
sr=mscott. I hate to see uses of nsfileSpec but I'm not going to hold your fix
hostage as most of this file uses the old nsFileSpec. 
I completely agree Scott but as you said, this isn't the time to rewrite the
migration code to use nsIFile.  Thanks for the reviews all!

Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
verified on trunk build 2001020508- Linux
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.