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

Status: VERIFIED FIXED

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

Description

[(not reading, please use [e-mail] instead)]

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.

Comment 1

[dbragg]

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.

Comment 2

[(not reading, please use [e-mail] instead)]

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?

Comment 3

[dbragg]

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.

Comment 4

[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.

Comment 5

[dbragg]

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?

Comment 6

[dbragg]

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.

Comment 7

[dbragg]

Attachment: Diff for bug 58371

Comment 8

[dbragg]

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.

Comment 9

[dbragg]

Attachment: New diff with nsMemory::Free

Comment 10

[dbragg]

Upping the priority and severity and adding rtm keyword for PDT review.

Comment 11

[dbragg]

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.

Comment 12

[dbragg]

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.

Comment 13

[dbragg]

Attachment: New diff using nsXPIDLCString.

Comment 14

[Daniel Veditz [:dveditz]]

r=dveditz for the 11/01 patch

Comment 15

[(not reading, please use [e-mail] instead)]

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.

Comment 16

[Scott MacGregor]

Adding myself to the cc list so I can give the final patch a super review

Comment 17

[dbragg]

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.  

Comment 18

[selmer (gone)]

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...

Comment 19

[dbragg]

I could put a delete call in the destructor.

Comment 20

[dbragg]

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.

Comment 21

[dbragg]

Attachment: New diff using CreateUnique

Comment 22

[(not reading, please use [e-mail] instead)]

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

Comment 23

[dbragg]

Sure.  Do I need to post another patch?

Comment 24

[(not reading, please use [e-mail] instead)]

Attachment: dbragg's patch with the changes I mentioned.

Comment 25

[(not reading, please use [e-mail] instead)]

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=?

Comment 26

[dbragg]

No, thank YOU for taking the time to make such a thorough inspection.  Muy
importante at this stage!

Comment 27

[Daniel Veditz [:dveditz]]

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.

Comment 28

[dbragg]

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 ;-)

Comment 29

[Scott MacGregor]

sr=mscott. Thanks guys!

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Better to fix CopyTo() to overwrite existing files.

Comment 31

[dbragg]

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.

Comment 32

[dbragg]

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.

Comment 33

[dbragg]

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.

Comment 34

[(not reading, please use [e-mail] instead)]

note:  it would be named "preferences.js" and it could be in $TMPDIR if that
environment variable set, otherwise it would be in /tmp.

Comment 35

[olaf]

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.

Comment 36

[(not reading, please use [e-mail] instead)]

why is /tmp bad?  would /tmp/<uniq> be any better?  please explain.

adding patch keyword.

Comment 37

[dbragg]

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.

Comment 38

[dbragg]

Attachment: Patch with latest Trunk changes (as of 11/27) merged in.

Comment 39

[dbragg]

oops.  I didn't mean "fix for bug 1.125" I meant revision number 1.125.  Sorry.

Comment 40

[verah (gone)]

Removing myself from list of cc's. This was release noted.

Comment 41

[Daniel Veditz [:dveditz]]

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.

Comment 42

[racham]

r=racham.

Comment 43

[Scott MacGregor]

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. 

Comment 44

[dbragg]

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.

Comment 45

[Grace Bush]

verified on trunk build 2001020508- Linux