[webkit-reviews] review denied: [Bug 53784] Application Cache should save audio/ and video/ mime types as flat files : [Attachment 81701] [2/3] Store media files as flat files within the Application Cache area.

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has denied Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 53784: Application Cache should save audio/ and video/ mime types as flat
files
https://bugs.webkit.org/show_bug.cgi?id=53784

Attachment 81701: [2/3] Store media files as flat files within the Application
Cache area.
https://bugs.webkit.org/attachment.cgi?id=81701&action=review

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=81701&action=review

Overall comment: where does "path" come from in the first place? Is it based on
the URL of the resource? If so, we need to make sure to sanitize it to avoid
dangerous characters like / or null. I see that you try to avoid escaping the
flat file directory, but I don't see that in store().

I would like to see path sanitization code or an explanation of why it's not
needed. The style comments are purely optional, but I think this part is
required to fix, so r-.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:564
> +    // Allow the normal flat-file cleanup process to execute before clearing
all the tables:

I suggest removing this comment or add an inline wrapper for empty() called
cleanUpFlatFiles() if it's important to state the purpose.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:648
> +    // When a cache resource is deleted, if it contains a non-empty path,
that path should
> +    // be added to the DeletedCacheResources table so the flat file at that
path can
> +    // be deleted at a later time.
> +    executeSQLCommand("CREATE TRIGGER IF NOT EXISTS CacheResourceDataDeleted
AFTER DELETE ON CacheResourceData"
> +			 " FOR EACH ROW"
> +			 " WHEN OLD.path NOT NULL BEGIN"
> +			 "  INSERT INTO DeletedCacheResources (path) values
(OLD.path);"
> +			 " END");

I suggest instead of the comment you factor this out as a helper function:
recordResourcePathForLaterDeletion() or something like that.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:786
> +    if (resource->response().mimeType().startsWith("audio/", false) ||
resource->response().mimeType().startsWith("video/", false)) {

The chunk of code added in this if statement is pretty big to bury in an if
statement. I suggest factoring it out into a separate function.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1076
> +	   String path = cacheStatement.getColumnText(6);

Consider making a named constant instead of using 6 directly in code.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:1383
> +	   ASSERT(directoryName(fullPath) == flatFileDirectory);

I don't think it's right to ASSERT for something based on a file on disk. You
should always assume that files on disk could be corrupted or even malicious. I
suggest you leve it at just the if check below.