Deleting profile does not delete parent directory

VERIFIED FIXED in mozilla0.9.1

Status

Core Graveyard
Profile: BackEnd
P3
normal
VERIFIED FIXED
17 years ago
2 years ago

People

(Reporter: Grace Bush, Assigned: Conrad Carlen (not reading bugmail))

Tracking

Trunk
mozilla0.9.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

17 years ago
If you create directory 'test', directory/file structure is created as follows:
c:\winnt\profiles\username\application data\mozilla\users50\test\'saltedname'
If 'test' is deleted, saltedname directory and all files in it are deleted but 
'test' remains.


These are one of the side-effects of fixing bug 56002 (adding a salted
  directory). It is not harmful and extra-directory sticks around.
  If there is a salted directory, we   may have to go one level up and delete 
it's parent folder also..

Comment 1

17 years ago
reassigning to racham.
Assignee: putterman → racham
Summary: Deleting profile does not delete parent directory → Deleting profile does not delete parent directory
(Reporter)

Comment 2

17 years ago
*** Bug 60799 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 3

17 years ago
comments from bug 60799
having recently gotten back on the trunk, i've noticed that new profiles, rather
than containing the files directly under the <profile_name>/ directory, now
store files in a <profile_name>/<random>.slt/ subdirectory. i spoke with grace,
who sez this a new feature to increase profile info security.

true --but this seems rather superficial. one could just cd into in that
<random>.slt directory to get at the files.

my suggestion here is to get rid of this extra level of obfuscation (i could
understand that it's there to confuse, but it doesn't seem terribly effective
--unless there's another reason ;). instead, why not implement enhancements such
as those suggested in bug 16489, bug 19184 and bug 59120?

thx!
adding sairuh to cc list

Comment 4

17 years ago
Doing a mass reassign to Conrad Carlen.

Conrad is going address all current and upcoming profile manager issues. Please
do not hesitate to involve me in any of the issues.
Assignee: racham → ccarlen
(Assignee)

Comment 5

17 years ago
When deleting the profile dir, we need to check its parent dir for the .slt
suffix and delete from there.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9
(Assignee)

Comment 6

17 years ago
*** Bug 61993 has been marked as a duplicate of this bug. ***
*** Bug 68326 has been marked as a duplicate of this bug. ***
nominating.
Keywords: nsbeta1
OS: Windows NT → All
Hardware: PC → All
(Assignee)

Comment 9

17 years ago
-> 0.9.1
Target Milestone: mozilla0.9 → mozilla0.9.1
(Assignee)

Comment 10

17 years ago
*** Bug 78387 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 11

17 years ago
Created attachment 32775 [details] [diff] [review]
patch to delete parent
(Assignee)

Comment 12

17 years ago
The patch fixes this in a safe manner. When deleting a profile dir, its parent
is deleted only if:
(1) the profile dir ends in ".slt"
(2) the profile dir is the only child of its parent (in case somebody moved
their salted profile dir to the root of their hard drive - whoops!)

CC'ing Bhuvan & Seth for review.

Comment 13

17 years ago
Conrad, patch looks good to me. I would like to request you to try out some test
cases and confirm the results are good before check in as we are talking about
deleting the data/folders here and we were in deep troubles with this operation
in the past. Sorry for the overload.

r=bhuvan

Updated

17 years ago
Keywords: patch
(Assignee)

Comment 14

17 years ago
I've tested it. Since it gets the parent dir of the dir it's about to delete
anyway and checks that the parent has only one child, that should be safe. What
it does now is this:

(1) given the profile dir
(2) gets its parent
(3) ensure that the parent has only one child

For an extra measure of safety, it could:
(4) ensure that the one child == the profile dir

That would even insure against nsIFile::GetParent() doing something wacky or
(gasp!) having a bug.
(Assignee)

Comment 15

17 years ago
CC'ing waterson for sr.

Comment 16

17 years ago
I'm not hep to the latest string jive, so tell me if this will match 
``foo.slt.bar''? (In other words, does it really test that the string _ends_ 
with ``.slt''? (Maybe that's what end.size_forward() does?)

+    nsLiteralCString leafNameStr(leafName.get());
+    nsReadingIterator<char> start, end;
+    leafNameStr.BeginReading(start);
+    leafNameStr.EndReading(end);
+    FindInReadable(NS_LITERAL_CSTRING(SALT_EXTENSION), start, end);
+    if ((start == end) || end.size_forward())
+        return NS_OK;

I'd have called the routine ``IsProfileDirSaltedAndUnique'' or or something, 
just to indicate that you're also checking that condition. Maybe not, take it or 
leave it.
(Assignee)

Comment 17

17 years ago
It shouldn't match 'foo.slt.bar' Since we salt dir names by adding .slt, if it
doesn't end in that, the user has changed it and I'd say we shouldn't delete it.

As far as the routine name, how about ShouldDeleteProfileParentDir() ?

Comment 18

17 years ago
> It shouldn't match 'foo.slt.bar' 

I agree that it *shouldn't*, but I'm not convinced that it *doesn't*. Certainly, 
if given the string ``foo.slt.bar'', you'll end up with...

   foo.slt.bar
      ^   ^
      |   end
      start

So |start != end|. What will |end.size_forward()| return in this case? (I guess 
I'm wishing we had an ``EndsWith'' string helper routine right now!)

> As far as the routine name, how about ShouldDeleteProfileParentDir() ?

Sounds good to me!
(Assignee)

Comment 19

17 years ago
 foo.slt.bar
      ^   ^
      |   end
      start

So |start != end|. What will |end.size_forward()| return in this case? (I guess 
I'm wishing we had an ``EndsWith'' string helper routine right now!)

That's the truth!

If start == end, the string was not found.
In this case size_forward() should return 4 and fail the test.

Time to call in the string guy. Scott, is this right?

(Assignee)

Comment 20

17 years ago
CC'ing Johnny for string iterator review. Can you read the questions Waterson
had about searching in strings?

Comment 21

17 years ago
|end.size_forward()| is not a good test of anything except whether there is more
data in the hunk of string pointed to by |end| ... there may be more string in a
later hunk.  In "ReadableUtils.h" where it is declared, |FindInReadable| has
documentation.  I quote:

  Returns |PR_TRUE| if a match was found, and adjusts |aSearchStart| and
  |aSearchEnd| to point to the match. If no match was found, returns |PR_FALSE|
  and makes |aSearchStart == aSearchEnd|

So just collect the function result for your test, e.g.,

  PRBool matchFound = FindInReadable(NS_LITERAL_CSTRING(SALT_EXTENSION), start,
end);

But now you also want to know if that match was found at the very end of the
string?  No problem, test like this:

  nsLiteralCString leafNameStr(leafName.get());

  nsReadingIterator<char> stringEnd
  leafNameStr.EndReading(stringEnd);
  nsReadingIterator<char> matchEnd = stringEnd, matchStart;
  leafNameStr.BeginReading(matchStart);

  PRBool endWithSalt =
    FindInReadable(NS_LITERAL_CSTRING(SALT_EXTENSION), matchStart, matchEnd)
    && (matchEnd == stringEnd);

Of course, since you're not even interested in finding a match anywhere else in
the string, all you really care about is if the last |n| characters of the
string match your pattern.  If this is the case, just extract the appropriate
range for comparison first, e.g.,

  PRBool endsWithSalt = PR_FALSE;
  NS_NAMED_LITERAL_CSTRING(saltPattern, ".slt");
  if ( leafNameString.Length() >= saltPattern.Length() )
    {
      nsReadingIterator<char> stringEnd;
      leafNameString.EndReading(stringEnd);

      nsReadingIterator<char> stringStart = stringEnd;
      stringStart.advance( -saltPattern.Length() );

      endsWithSalt = saltPattern.Equals(Substring(stringStart, stringEnd));
        // or |==|, or |Compare|, or whatever makes you feel happiest
    }

Also, I would caution against applying the macro |NS_LITERAL_CSTRING| to another
macro as in your original example.  There have been problems with this using
|NS_LITERAL_STRING|, it just seems less safe.  Prefer defining the macro from
the start, e.g.,

  #define SALT_EXTENSION_CSTRING NS_LITERAL_CSTRING(".slt")

or whatever.

Hope this helps
(Assignee)

Comment 22

17 years ago
Created attachment 33675 [details] [diff] [review]
updated patch
(Assignee)

Comment 23

17 years ago
The new patch uses Scott's last suggestion for determining whether the name ends
in ".slt" It also adds a check for whether the profile dir in question exists
before all this work. It would have safely failed later but better to fail sooner.
(Assignee)

Comment 24

17 years ago
Alright, now with Scott's string advice taken and the method name changed, can I
get sr?
sr=sspitzer

the code you needed to write to do "ends with" seems like a lot of work.

a while back I added some code to nsPrefMigration.cpp, and I'm sure there are
other "ends with" utilitity functions out there.

scc, seems like this could be added the string library?

Comment 26

17 years ago
see bug 76946
(Assignee)

Comment 27

17 years ago
Fix checked in. Sorry for the delay - I was away for a few days.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 28

17 years ago
Jonathan, please test this for I18n.  If you see some problems for I18n, please
log the other bug.
(Reporter)

Updated

17 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Comment 29

17 years ago
build 2001052306
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.