Bug 44132 - Implement virtual path utilities for FileSystem API
: Implement virtual path utilities for FileSystem API
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
: 42903
  Show dependency treegraph
 
Reported: 2010-08-17 14:43 PST by
Modified: 2010-08-24 21:31 PST (History)


Attachments
Patch (26.08 KB, patch)
2010-08-17 15:11 PST, Kinuko Yasuda
no flags Review Patch | Details | Formatted Diff | Diff
Patch (8.24 KB, patch)
2010-08-18 13:50 PST, Kinuko Yasuda
no flags Review Patch | Details | Formatted Diff | Diff
Patch (17.19 KB, patch)
2010-08-23 11:49 PST, Kinuko Yasuda
no flags Review Patch | Details | Formatted Diff | Diff
Patch (17.39 KB, patch)
2010-08-23 23:08 PST, Kinuko Yasuda
no flags Review Patch | Details | Formatted Diff | Diff
Patch (17.40 KB, patch)
2010-08-24 11:12 PST, Kinuko Yasuda
dumi: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-08-17 14:43:12 PST
Add file path utilities for HTML5 FileSystem API.
We need some utility class that takes care of path sanitization (cleaning up relative references), concatenation and etc for virtual filesystem paths.
------- Comment #1 From 2010-08-17 15:11:43 PST -------
Created an attachment (id=64643) [details]
Patch
------- Comment #2 From 2010-08-18 13:50:28 PST -------
Created an attachment (id=64767) [details]
Patch

Simplified the patch.
------- Comment #3 From 2010-08-18 22:54:39 PST -------
(From update of attachment 64767 [details])
(seems like it'll need more logic)
------- Comment #4 From 2010-08-23 11:49:24 PST -------
Created an attachment (id=65145) [details]
Patch
------- Comment #5 From 2010-08-23 19:52:45 PST -------
(From update of attachment 65145 [details])
WebCore/storage/DOMFilePath.cpp:43
 +  const char* DOMFilePath::root = "/";
const char DOMFilePath::root[] = "/"; ?

WebCore/storage/DOMFilePath.cpp:73
 +      return path;
This seems odd, when compared to getName.  Is returning path really the right thing to do in each case?

WebCore/storage/DOMFilePath.cpp:78
 +      return mayBeChild.startsWith(path);
/foo/bar is not the parent of /foo/barrister.

WebCore/storage/DOMFilePath.cpp:115
 +      const char* p = utf8String.data();
What happens here if the UTF-8 representation of a character in the path doesn't fit in a char?

WebCore/storage/DOMFilePath.cpp:125
 +      if (unallowedNamesRegExp1.match(path) >= 0)
Is this match case-sensitive?  It needs to be case-insensitive on all of these.

WebCore/storage/DOMFilePath.cpp:122
 +      DEFINE_STATIC_LOCAL(RegularExpression, unallowedNamesRegExp1, ("/(CON|PRN|AUX|NUL)([\\./]|$)", TextCaseInsensitive));
I've never used RegularExpressions from WebKit internally before.  Is that last '/' in the right place?

WebCore/storage/DOMFilePath.cpp:131
 +      DEFINE_STATIC_LOCAL(RegularExpression, endingRegExp, ("[\\. ](/|$)", TextCaseInsensitive));
What about other kinds of whitespace [tab, vtab, newline, cr, ...]

WebCore/storage/DOMFilePath.cpp:138
 +      DEFINE_STATIC_LOCAL(RegularExpression, unallowedCharsRegExp, ("[\\\\<><>:\\?\\*\"|]", TextCaseInsensitive));
You've got "<>" in there twice.

WebCore/storage/DOMFilePath.cpp:108
 +  bool DOMFilePath::isValid(const String& path)
Perhaps "isValidPath" instead of just "isValid"?  One might call this by accident for a name and let something containing a directory separator slip through.

WebCore/storage/DOMFilePath.h:44
 +      static const char* root;
How about static const char root[]?

WebCore/storage/DOMFilePath.cpp:74
 +  }
I think it might be better to return an empty string, or perhaps make sure not to be called in that case.  What's the directory of "foo"?  I suppose it could be ".".

WebCore/storage/DOMFilePath.h:52
 +      // Checks if a given path is a parent of mayBeChild. This method assumes given paths are absolute and do not have extra parent references.
Explain what a parent reference is.  Do you mean "../"?

WebCore/storage/DOMFilePath.h:55
 +      // Appends the separator at the end of the path.
...if it's not there already.

WebCore/storage/DOMFilePath.h:71
 +      // Removes extra parent ("..") references. This doesn't take all the references if it goes beyond the root directory.
It does more than that.  How about something like this?

// Evaluates all "../" and "./" segments.  Note that "/../" expands to "/", so you can't ever refer to anything above the root directory.
------- Comment #6 From 2010-08-23 23:08:25 PST -------
Created an attachment (id=65211) [details]
Patch
------- Comment #7 From 2010-08-23 23:11:38 PST -------
Thanks for your careful review!

(In reply to comment #5)
> (From update of attachment 65145 [details] [details])
> WebCore/storage/DOMFilePath.cpp:43
>  +  const char* DOMFilePath::root = "/";
> const char DOMFilePath::root[] = "/"; ?

Fixed.

> WebCore/storage/DOMFilePath.cpp:73
>  +      return path;
> This seems odd, when compared to getName.  Is returning path really the right thing to do in each case?

I wasn't sure what to return, but I changed this to return an empty string.  As you say probably "." or an empty string would be the expected value.

> WebCore/storage/DOMFilePath.cpp:78
>  +      return mayBeChild.startsWith(path);
> /foo/bar is not the parent of /foo/barrister.

Oh of course not... fixed.

> WebCore/storage/DOMFilePath.cpp:115
>  +      const char* p = utf8String.data();
> What happens here if the UTF-8 representation of a character in the path doesn't fit in a char?

Fixed.

> WebCore/storage/DOMFilePath.cpp:125
>  +      if (unallowedNamesRegExp1.match(path) >= 0)
> Is this match case-sensitive?  It needs to be case-insensitive on all of these.

All of RegExps are case-insensitive (CaseInsensitive).

> WebCore/storage/DOMFilePath.cpp:122
>  +      DEFINE_STATIC_LOCAL(RegularExpression, unallowedNamesRegExp1, ("/(CON|PRN|AUX|NUL)([\\./]|$)", TextCaseInsensitive));
> I've never used RegularExpressions from WebKit internally before.  Is that last '/' in the right place?

It's in the place I intended and my intension was: it should match if CON, PRN, AUX or NUL is followed by a period (\\.), slash (/) or the end of string ($).  Does that sound right?

> WebCore/storage/DOMFilePath.cpp:131
>  +      DEFINE_STATIC_LOCAL(RegularExpression, endingRegExp, ("[\\. ](/|$)", TextCaseInsensitive));
> What about other kinds of whitespace [tab, vtab, newline, cr, ...]

Replaced ' ' with '\s'.

> WebCore/storage/DOMFilePath.cpp:138
>  +      DEFINE_STATIC_LOCAL(RegularExpression, unallowedCharsRegExp, ("[\\\\<><>:\\?\\*\"|]", TextCaseInsensitive));
> You've got "<>" in there twice.

Fixed.

> WebCore/storage/DOMFilePath.cpp:108
>  +  bool DOMFilePath::isValid(const String& path)
> Perhaps "isValidPath" instead of just "isValid"?  One might call this by accident for a name and let something containing a directory separator slip through.

Fixed.

> WebCore/storage/DOMFilePath.h:44
>  +      static const char* root;
> How about static const char root[]?

Fixed.

> WebCore/storage/DOMFilePath.h:52
>  +      // Checks if a given path is a parent of mayBeChild. This method assumes given paths are absolute and do not have extra parent references.
> Explain what a parent reference is.  Do you mean "../"?

Yes I meant a reference to a parent, i.e. "..".  Changed the expression a bit.

> WebCore/storage/DOMFilePath.h:55
>  +      // Appends the separator at the end of the path.
> ...if it's not there already.

Fixed.

> WebCore/storage/DOMFilePath.h:71
>  +      // Removes extra parent ("..") references. This doesn't take all the references if it goes beyond the root directory.
> It does more than that.  How about something like this?
>
> // Evaluates all "../" and "./" segments.  Note that "/../" expands to "/", so you can't ever refer to anything above the root directory.

Changed the comment (and its behavior) as suggested.
------- Comment #8 From 2010-08-23 23:44:06 PST -------
> > WebCore/storage/DOMFilePath.cpp:125
> >  +      if (unallowedNamesRegExp1.match(path) >= 0)
> > Is this match case-sensitive?  It needs to be case-insensitive on all of these.
> 
> All of RegExps are case-insensitive (CaseInsensitive).

I meant... all of the regexps used here are declared with CaseInsensitive for case-insensitive match.
------- Comment #9 From 2010-08-24 11:02:40 PST -------
(From update of attachment 65211 [details])
WebCore/storage/DOMFilePath.cpp:124
 +      DEFINE_STATIC_LOCAL(RegularExpression, unallowedNamesRegExp1, ("/(CON|PRN|AUX|NUL)([\\./]|$)", TextCaseInsensitive));
Ah, I think I see how this is supposed to work.  However, you're requiring a leading '/' before matching for e.g. "CON".  If this isn't an absolute path, that's not safe.  "CON" and "CON/safe/rest/of/path" will pass the test.  Or you could assert and document that the input path must be absolute.
------- Comment #10 From 2010-08-24 11:12:33 PST -------
Created an attachment (id=65298) [details]
Patch
------- Comment #11 From 2010-08-24 11:13:46 PST -------
(In reply to comment #9)
> (From update of attachment 65211 [details] [details])
> WebCore/storage/DOMFilePath.cpp:124
>  +      DEFINE_STATIC_LOCAL(RegularExpression, unallowedNamesRegExp1, ("/(CON|PRN|AUX|NUL)([\\./]|$)", TextCaseInsensitive));
> Ah, I think I see how this is supposed to work.  However, you're requiring a leading '/' before matching for e.g. "CON".  If this isn't an absolute path, that's not safe.  "CON" and "CON/safe/rest/of/path" will pass the test.  Or you could assert and document that the input path must be absolute.

Right, fixed.
------- Comment #12 From 2010-08-24 16:25:03 PST -------
(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 65211 [details] [details] [details])
> > WebCore/storage/DOMFilePath.cpp:124
> >  +      DEFINE_STATIC_LOCAL(RegularExpression, unallowedNamesRegExp1, ("/(CON|PRN|AUX|NUL)([\\./]|$)", TextCaseInsensitive));
> > Ah, I think I see how this is supposed to work.  However, you're requiring a leading '/' before matching for e.g. "CON".  If this isn't an absolute path, that's not safe.  "CON" and "CON/safe/rest/of/path" will pass the test.  Or you could assert and document that the input path must be absolute.
> 
> Right, fixed.

OK, LGTM.  Now you need a reviewer ;'>.
------- Comment #13 From 2010-08-24 16:32:57 PST -------
(From update of attachment 65298 [details])
r=me. LGTM + i trust eric's review.
------- Comment #14 From 2010-08-24 21:31:38 PST -------
Committed r65975: <http://trac.webkit.org/changeset/65975>