Closed Bug 60182 Opened 24 years ago Closed 23 years ago

Deleting profile does not delete parent directory

Categories

(Core Graveyard :: Profile: BackEnd, defect, P3)

Product:
Core Graveyard
Component:
Profile: BackEnd
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.1

People

(Reporter: agracebush, Assigned: ccarlen)

References

Details

Attachments

(2 files)

patch to delete parent
4.86 KB, patch
Details | Diff | Splinter Review
updated patch
6.18 KB, patch
Details | Diff | Splinter Review 
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..
reassigning to racham.
Assignee: putterman → racham
Summary: Deleting profile does not delete parent directory → Deleting profile does not delete parent directory
*** Bug 60799 has been marked as a duplicate of this bug. ***
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
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
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
*** 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
-> 0.9.1
Target Milestone: mozilla0.9 → mozilla0.9.1
*** Bug 78387 has been marked as a duplicate of this bug. ***
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.

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
Keywords: patch
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.

CC'ing waterson for sr.
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.
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() ?

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


CC'ing Johnny for string iterator review. Can you read the questions Waterson
had about searching in strings?
|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
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.

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?
see bug 76946
Fix checked in. Sorry for the delay - I was away for a few days.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Jonathan, please test this for I18n.  If you see some problems for I18n, please
log the other bug.
Status: RESOLVED → VERIFIED
build 2001052306
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: