Bug 44132 – Implement virtual path utilities for FileSystem API

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kinuko Yasuda]

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.

[Kinuko Yasuda]

Created attachment 64643 [details]
Patch

[Kinuko Yasuda]

Created attachment 64767 [details]
Patch

Simplified the patch.

[Kinuko Yasuda]

Comment on attachment 64767 [details]
Patch

(seems like it'll need more logic)

[Kinuko Yasuda]

Created attachment 65145 [details]
Patch

[Eric U.]

Comment on attachment 65145 [details]
Patch

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.

[Kinuko Yasuda]

Created attachment 65211 [details]
Patch

[Kinuko Yasuda]

Thanks for your careful review!

(In reply to comment #5)
> (From update of attachment 65145 [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.

[Kinuko Yasuda]

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

[Eric U.]

Comment on attachment 65211 [details]
Patch

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.

[Kinuko Yasuda]

Created attachment 65298 [details]
Patch

[Kinuko Yasuda]

(In reply to comment #9)
> (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.

Right, fixed.

[Eric U.]

(In reply to comment #11)
> (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.

OK, LGTM.  Now you need a reviewer ;'>.

[Dumitru Daniliuc]

Comment on attachment 65298 [details]
Patch

r=me. LGTM + i trust eric's review.

[Kinuko Yasuda]

Committed r65975: <http://trac.webkit.org/changeset/65975>