Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12703012:
Have media gallery (through native media file util) use MIME sniffer (Closed)
|
Created:
7 years, 9 months ago by Kevin Bailey Modified:
7 years, 7 months ago CC:
chromium-reviews, tzik+watch_chromium.org, kinuko+watch, darin-cc_chromium.org, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionHave media gallery (through native media file util) use MIME sniffer
to verify all media files before allowing user to open them.
BUG=171753
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199587
Patch Set 1 #
Total comments: 22
Patch Set 2 : Code review response. #
Total comments: 12
Patch Set 3 : More code review response, added device_media_async_file_util. #
Total comments: 7
Patch Set 4 : Minor formatting responses. #
Total comments: 6
Patch Set 5 : Correct method ordering. #Patch Set 6 : More minor formatting. #Patch Set 7 : Fixed "which thread" errors. #
Total comments: 2
Patch Set 8 : Previous code review response, plus trying to get new unit test passing. #
Total comments: 11
Patch Set 9 : Unit test working. #
Total comments: 2
Patch Set 10 : Unit test code review tweaks #
Total comments: 2
Patch Set 11 : Correct style in test. #
Total comments: 3
Patch Set 12 : Fixed ScopedPlatformFile. #
Total comments: 3
Patch Set 13 : Check invalid handle. #Patch Set 14 : Add new MIME types without affecting current MIME sniffer behavior #Patch Set 15 : Oops, forgot, I can now add Flash #
Total comments: 6
Patch Set 16 : Merged massive media files change. #Patch Set 17 : Addressed comments about MIME sniffer. #
Total comments: 13
Patch Set 18 : Code review: More ShareableFileReference changes. #Patch Set 19 : More minor changes. #
Total comments: 6
Patch Set 20 : Minor code review changes. #
Total comments: 13
Patch Set 21 : Name change. #Patch Set 22 : Fixed unit test plus mime_sniffer review #
Total comments: 2
Patch Set 23 : Indentation fix. #Messages
Total messages: 56 (0 generated)
Looks good with just a few clean ups. This actually turned out to be less code than I expected. https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... File webkit/fileapi/media/native_media_file_util.cc (right): https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:35: bool IsOnIOThread(fileapi::FileSystemOperationContext* context) { nit: There's a thread called the IO thread, so this isn't a good name. Maybe OnTaskRunnerThread ? https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:250: != base::PLATFORM_FILE_OK) { To improve formatting, assign the result of CreateOrOpen to a variable and check that. https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:254: new base::PlatformFile(file_handle)); nit: will fit on the previous line. https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:259: // Let the SniffMimeType() function decide what to do Actually, I think we can pretty safely say that a zero byte file is not a media file. https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:264: std::string result; nit: result -> mime_type ? https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:265: if (!net::SniffMimeType(buffer, net::kMaxBytesToSniff, I think you should pass len here. https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:266: GURL(path.value()), nit: This will fit on the previous line. https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:266: GURL(path.value()), Should we prefix "file://" to the path first ? https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:281: DCHECK(policy); Can we change lines 281-288 to just IsolatedFileUtil::CreateSnapshotFile(...); if (error != base::PLATFORM_FILE_OK) return error; ?
Responses to second round of comments. One thing to discuss about possibly improving. https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... File webkit/fileapi/media/native_media_file_util.cc (right): https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:35: bool IsOnIOThread(fileapi::FileSystemOperationContext* context) { On 2013/03/20 00:18:27, vandebo wrote: > nit: There's a thread called the IO thread, so this isn't a good name. Maybe > OnTaskRunnerThread ? And that's what I get for plagiarizing. This was copied from device_media_async_file_util. I meant to ask if you thought a common location would be better, but I couldn't spot one. It needs about 5 different class definitions, so there are lots of possibilities, from file_system_operation_context to file_system_task_runners. Or I could just leave it here. https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:250: != base::PLATFORM_FILE_OK) { On 2013/03/20 00:18:27, vandebo wrote: > To improve formatting, assign the result of CreateOrOpen to a variable and check > that. Done. https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:254: new base::PlatformFile(file_handle)); On 2013/03/20 00:18:27, vandebo wrote: > nit: will fit on the previous line. Ok, but one stray rename... https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:259: // Let the SniffMimeType() function decide what to do On 2013/03/20 00:18:27, vandebo wrote: > Actually, I think we can pretty safely say that a zero byte file is not a media > file. Fair enough, 1) I wanted the policy decisions in one place (there may very well be valid zero-byte media files, however I will grant the user probably isn't hurt by not opening them) and 2) I can't imagine how one could implement an attack through a zero-byte file. https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:264: std::string result; On 2013/03/20 00:18:27, vandebo wrote: > nit: result -> mime_type ? Done. https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:265: if (!net::SniffMimeType(buffer, net::kMaxBytesToSniff, On 2013/03/20 00:18:27, vandebo wrote: > I think you should pass len here. Good catch. https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:266: GURL(path.value()), On 2013/03/20 00:18:27, vandebo wrote: > Should we prefix "file://" to the path first ? It doesn't seem to need it, but I'm all for sanitizing input. https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:266: GURL(path.value()), On 2013/03/20 00:18:27, vandebo wrote: > nit: This will fit on the previous line. Done. https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:281: DCHECK(policy); On 2013/03/20 00:18:27, vandebo wrote: > Can we change lines 281-288 to just IsolatedFileUtil::CreateSnapshotFile(...); > if (error != base::PLATFORM_FILE_OK) > return error; > ? Done. Moved the DCHECK too, since I imagine we wanted to test before the snapshot, not so much before the media check.
lgtm https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... File webkit/fileapi/media/native_media_file_util.cc (right): https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:35: bool IsOnIOThread(fileapi::FileSystemOperationContext* context) { On 2013/03/20 13:53:23, Kevin Bailey wrote: > On 2013/03/20 00:18:27, vandebo wrote: > > nit: There's a thread called the IO thread, so this isn't a good name. Maybe > > OnTaskRunnerThread ? > > And that's what I get for plagiarizing. This was copied from > device_media_async_file_util. I meant to ask if you thought a common location > would be better, but I couldn't spot one. It needs about 5 different class > definitions, so there are lots of possibilities, from > file_system_operation_context to file_system_task_runners. Or I could just leave > it here. Hmm, I'm not completely familiar with the different task runners. The one in device_media_async_file_util.cc just does context->task_runner()->RunsTasksOnCurrentThread(). Is there a reason to specifically pull out the io_task_runner? I think it's fine as a stand alone function, or even fine inline. https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:259: // Let the SniffMimeType() function decide what to do On 2013/03/20 13:53:23, Kevin Bailey wrote: > On 2013/03/20 00:18:27, vandebo wrote: > > Actually, I think we can pretty safely say that a zero byte file is not a > media > > file. > > Fair enough, 1) I wanted the policy decisions in one place (there may very well > be valid zero-byte media files, however I will grant the user probably isn't > hurt by not opening them) and 2) I can't imagine how one could implement an > attack through a zero-byte file. I don't feel strongly, if you'd prefer to leave it as only < 0, that's fine as well. In practice I don't think it'll make a difference because a 0 byte media file simply can't be validated with mime type sniffing, there's nothing to validate. https://codereview.chromium.org/12703012/diff/4001/webkit/fileapi/media/nativ... File webkit/fileapi/media/native_media_file_util.cc (right): https://codereview.chromium.org/12703012/diff/4001/webkit/fileapi/media/nativ... webkit/fileapi/media/native_media_file_util.cc:248: base::PLATFORM_FILE_OPEN | nit: technically line 248 and 249 are a single argument, so they should go on one line if possible. This is probably the most readable indention: base::PlatformFileError error = NativeFileUtil::CreateOrOpen( path, base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ, &file_handle, &created); This would also be acceptable. base::PlatformFileError error = NativeFileUtil::CreateOrOpen( path, base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ, &file_handle, &created); https://codereview.chromium.org/12703012/diff/4001/webkit/fileapi/media/nativ... webkit/fileapi/media/native_media_file_util.cc:251: if (error != base::PLATFORM_FILE_OK) { nit: Single line if's with single line bodies may omit the {}'s This file seems to do that, so for consistency they should be omitted. https://codereview.chromium.org/12703012/diff/4001/webkit/fileapi/media/nativ... webkit/fileapi/media/native_media_file_util.cc:251: if (error != base::PLATFORM_FILE_OK) { Hmm, I guess this function can fail for a reason other than not a media file. Maybe it should return base::PlatformFileError isntead of a bool. Thoughts? https://codereview.chromium.org/12703012/diff/4001/webkit/fileapi/media/nativ... webkit/fileapi/media/native_media_file_util.cc:259: if (len <= 0) { And here https://codereview.chromium.org/12703012/diff/4001/webkit/fileapi/media/nativ... webkit/fileapi/media/native_media_file_util.cc:278: DCHECK(policy); nit: this method doesn't actually care if policy for file_info are non-NULL. Generally we use DCHECKs for assumptions that the following code makes. So just leave it up to LocalFileUtil::CreateSnapshotFile to DCHECK those two parameters. https://codereview.chromium.org/12703012/diff/4001/webkit/fileapi/media/nativ... File webkit/fileapi/media/native_media_file_util.h (right): https://codereview.chromium.org/12703012/diff/4001/webkit/fileapi/media/nativ... webkit/fileapi/media/native_media_file_util.h:91: // Trojan horse.) Actually, mime type sniffing is about preventing exposure of non media files that have a media extension.
Oops, didn't mean to click the LGTM button. I was going to say, sorry for all the nits. You'll need an owners reviewer as well. I'll add one after I send this so your next mail will ping them.
Also added changes to device_media_async_file_util, but haven't tested the latest since it doesn't appear that I've picked up Lei's crash fix. https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... File webkit/fileapi/media/native_media_file_util.cc (right): https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:35: bool IsOnIOThread(fileapi::FileSystemOperationContext* context) { On 2013/03/20 20:03:48, vandebo wrote: > > Hmm, I'm not completely familiar with the different task runners. The one in > device_media_async_file_util.cc just does > context->task_runner()->RunsTasksOnCurrentThread(). Is there a reason to > specifically pull out the io_task_runner? It's identical to what's in device_media_async_file_util.cc https://codereview.chromium.org/12703012/diff/1/webkit/fileapi/media/native_m... webkit/fileapi/media/native_media_file_util.cc:259: // Let the SniffMimeType() function decide what to do On 2013/03/20 20:03:48, vandebo wrote: > On 2013/03/20 13:53:23, Kevin Bailey wrote: > > On 2013/03/20 00:18:27, vandebo wrote: > > > Actually, I think we can pretty safely say that a zero byte file is not a > > media > > > file. > > > > Fair enough, 1) I wanted the policy decisions in one place (there may very > well > > be valid zero-byte media files, however I will grant the user probably isn't > > hurt by not opening them) and 2) I can't imagine how one could implement an > > attack through a zero-byte file. > > I don't feel strongly, if you'd prefer to leave it as only < 0, that's fine as > well. In practice I don't think it'll make a difference because a 0 byte media > file simply can't be validated with mime type sniffing, there's nothing to > validate. Ack. https://codereview.chromium.org/12703012/diff/4001/webkit/fileapi/media/nativ... File webkit/fileapi/media/native_media_file_util.cc (right): https://codereview.chromium.org/12703012/diff/4001/webkit/fileapi/media/nativ... webkit/fileapi/media/native_media_file_util.cc:248: base::PLATFORM_FILE_OPEN | On 2013/03/20 20:03:49, vandebo wrote: > nit: technically line 248 and 249 are a single argument, so they should go on > one line if possible. This is probably the most readable indention: > base::PlatformFileError error = NativeFileUtil::CreateOrOpen( > path, > base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ, > &file_handle, > &created); > > This would also be acceptable. > base::PlatformFileError error = NativeFileUtil::CreateOrOpen( > path, base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_READ, &file_handle, > &created); Done. https://codereview.chromium.org/12703012/diff/4001/webkit/fileapi/media/nativ... webkit/fileapi/media/native_media_file_util.cc:251: if (error != base::PLATFORM_FILE_OK) { On 2013/03/20 20:03:49, vandebo wrote: > nit: Single line if's with single line bodies may omit the {}'s This file seems > to do that, so for consistency they should be omitted. I glanced up to line 234 to see what style the file liked, but it lied. Done. https://codereview.chromium.org/12703012/diff/4001/webkit/fileapi/media/nativ... webkit/fileapi/media/native_media_file_util.cc:251: if (error != base::PLATFORM_FILE_OK) { On 2013/03/20 20:03:49, vandebo wrote: > Hmm, I guess this function can fail for a reason other than not a media file. > Maybe it should return base::PlatformFileError isntead of a bool. Thoughts? Reason #27 why exceptions are better than return codes. This will work fine until the function needs to return something else. As it is, the callers are a bit simpler, at the expense of some extra conditions here. https://codereview.chromium.org/12703012/diff/4001/webkit/fileapi/media/nativ... webkit/fileapi/media/native_media_file_util.cc:259: if (len <= 0) { On 2013/03/20 20:03:49, vandebo wrote: > And here Done. https://codereview.chromium.org/12703012/diff/4001/webkit/fileapi/media/nativ... webkit/fileapi/media/native_media_file_util.cc:278: DCHECK(policy); On 2013/03/20 20:03:49, vandebo wrote: > nit: this method doesn't actually care if policy for file_info are non-NULL. > Generally we use DCHECKs for assumptions that the following code makes. So just > leave it up to LocalFileUtil::CreateSnapshotFile to DCHECK those two parameters. Done. https://codereview.chromium.org/12703012/diff/4001/webkit/fileapi/media/nativ... File webkit/fileapi/media/native_media_file_util.h (right): https://codereview.chromium.org/12703012/diff/4001/webkit/fileapi/media/nativ... webkit/fileapi/media/native_media_file_util.h:91: // Trojan horse.) On 2013/03/20 20:03:49, vandebo wrote: > Actually, mime type sniffing is about preventing exposure of non media files > that have a media extension. Done, but moved (possibly temporarily.)
Kinuko is busy trying to get something done for Monday, so if you have cycles to spend on this, you could start another CL that adds the other mime types we'll need. https://codereview.chromium.org/12703012/diff/11001/webkit/fileapi/media/devi... File webkit/fileapi/media/device_media_async_file_util.cc (right): https://codereview.chromium.org/12703012/diff/11001/webkit/fileapi/media/devi... webkit/fileapi/media/device_media_async_file_util.cc:318: NativeMediaFileUtil::IsMediaFile(platform_path); nit: two more spaces. https://codereview.chromium.org/12703012/diff/11001/webkit/fileapi/media/nati... File webkit/fileapi/media/native_media_file_util.cc (right): https://codereview.chromium.org/12703012/diff/11001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util.cc:260: else if (len == 0) nit: Convention is to make this just an if since the above body is a return. https://codereview.chromium.org/12703012/diff/11001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util.cc:266: if (StartsWithASCII(mime_type, "image/", true) || nit: Since the if is multi line, it and the else has to have {}'s
lgtm https://codereview.chromium.org/12703012/diff/11001/webkit/fileapi/media/nati... File webkit/fileapi/media/native_media_file_util.cc (right): https://codereview.chromium.org/12703012/diff/11001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util.cc:243: base::PlatformFileError NativeMediaFileUtil::IsMediaFile( nit: can you match the method order in .cc and in .h?
Lets get a unit test worked out and then this can go in.
https://codereview.chromium.org/12703012/diff/11001/webkit/fileapi/media/nati... File webkit/fileapi/media/native_media_file_util.cc (right): https://codereview.chromium.org/12703012/diff/11001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util.cc:243: base::PlatformFileError NativeMediaFileUtil::IsMediaFile( On 2013/03/21 16:59:28, kinuko wrote: > nit: can you match the method order in .cc and in .h? Done. https://codereview.chromium.org/12703012/diff/11001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util.cc:260: else if (len == 0) On 2013/03/21 16:51:12, vandebo wrote: > nit: Convention is to make this just an if since the above body is a return. Done. https://codereview.chromium.org/12703012/diff/11001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util.cc:266: if (StartsWithASCII(mime_type, "image/", true) || On 2013/03/21 16:51:12, vandebo wrote: > nit: Since the if is multi line, it and the else has to have {}'s Done.
https://codereview.chromium.org/12703012/diff/21001/webkit/fileapi/media/devi... File webkit/fileapi/media/device_media_async_file_util.cc (right): https://codereview.chromium.org/12703012/diff/21001/webkit/fileapi/media/devi... webkit/fileapi/media/device_media_async_file_util.cc:316: if (!callback.is_null()) { After looking at this a bit more, I'm not sure it's correct. OnSnapshotFileCreatedRunTask is what points to this function and it is passed as a success callback. There's something else passed as a failure callback. So I think the way the code is structured currently assumes that once we're to this step there's no way to fail anymore. With this change that's not the case anymore. It would be ok to roll this part back and approach it in a different CL. https://codereview.chromium.org/12703012/diff/21001/webkit/fileapi/media/nati... File webkit/fileapi/media/native_media_file_util.cc (right): https://codereview.chromium.org/12703012/diff/21001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util.cc:263: if (!net::SniffMimeType(buffer, len, GURL("file://" + path.value()), nit: this is a multi-line if, so it should use braces. https://codereview.chromium.org/12703012/diff/21001/webkit/fileapi/media/nati... File webkit/fileapi/media/native_media_file_util.h (right): https://codereview.chromium.org/12703012/diff/21001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util.h:64: static base::PlatformFileError IsMediaFile(const base::FilePath& path); All the othe rmethods are implementations of IsolatedFileUtil, so they should stay together. Put IsMediaFile before or after the block of overriding functions and correspondly move the implementation.
https://codereview.chromium.org/12703012/diff/30001/webkit/fileapi/media/devi... File webkit/fileapi/media/device_media_async_file_util.cc (right): https://codereview.chromium.org/12703012/diff/30001/webkit/fileapi/media/devi... webkit/fileapi/media/device_media_async_file_util.cc:68: base::PlatformFileError* p_error) { nit: error, not p_error in all your change.
Somehow missed some old draft messages. https://codereview.chromium.org/12703012/diff/21001/webkit/fileapi/media/devi... File webkit/fileapi/media/device_media_async_file_util.cc (right): https://codereview.chromium.org/12703012/diff/21001/webkit/fileapi/media/devi... webkit/fileapi/media/device_media_async_file_util.cc:316: if (!callback.is_null()) { Chatted with vandebo and it looks ok to us, but I will step through it and figure out who ultimately cares if the create snapshot succeeds or not. There is a concern that the entity responsible for cleaning up the snapshot file may think it was never created. https://codereview.chromium.org/12703012/diff/21001/webkit/fileapi/media/nati... File webkit/fileapi/media/native_media_file_util.cc (right): https://codereview.chromium.org/12703012/diff/21001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util.cc:263: if (!net::SniffMimeType(buffer, len, GURL("file://" + path.value()), On 2013/03/25 19:02:11, vandebo wrote: > nit: this is a multi-line if, so it should use braces. Done. https://codereview.chromium.org/12703012/diff/21001/webkit/fileapi/media/nati... File webkit/fileapi/media/native_media_file_util.h (right): https://codereview.chromium.org/12703012/diff/21001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util.h:64: static base::PlatformFileError IsMediaFile(const base::FilePath& path); On 2013/03/25 19:02:11, vandebo wrote: > All the othe rmethods are implementations of IsolatedFileUtil, so they should > stay together. Put IsMediaFile before or after the block of overriding > functions and correspondly move the implementation. Done. https://codereview.chromium.org/12703012/diff/30001/webkit/fileapi/media/devi... File webkit/fileapi/media/device_media_async_file_util.cc (right): https://codereview.chromium.org/12703012/diff/30001/webkit/fileapi/media/devi... webkit/fileapi/media/device_media_async_file_util.cc:68: base::PlatformFileError* p_error) { On 2013/03/29 21:53:20, vandebo wrote: > nit: error, not p_error in all your change. Done.
Like Lei said, it looks like maybe the issue is not running the message loop. https://codereview.chromium.org/12703012/diff/37001/webkit/fileapi/media/nati... File webkit/fileapi/media/native_media_file_util_unittest.cc (right): https://codereview.chromium.org/12703012/diff/37001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util_unittest.cc:97: NativeFileUtil::CreateOrOpen( It might be easier to use WriteFile() from base/file_util.h here. https://codereview.chromium.org/12703012/diff/37001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util_unittest.cc:630: if (!kFilteringTestCases[i].is_directory && nit: reverse the case of the if and use continue. https://codereview.chromium.org/12703012/diff/37001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util_unittest.cc:635: if (kFilteringTestCases[i].media_file) { nit: use an "expected" variable and assign it based on this condition. That will let you pull the duplicated code out of the if/else. https://codereview.chromium.org/12703012/diff/37001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util_unittest.cc:637: operation->CreateSnapshotFile(url, After op->CrateSnapshotFile, you should call MessageLoop::current()->RunUntilIdle() to get all the async calls to run. Did you already try that? It looks like you were trying to make things synchronous for debugging.
https://codereview.chromium.org/12703012/diff/37001/webkit/fileapi/media/nati... File webkit/fileapi/media/native_media_file_util_unittest.cc (right): https://codereview.chromium.org/12703012/diff/37001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util_unittest.cc:97: NativeFileUtil::CreateOrOpen( On 2013/04/03 18:54:14, vandebo wrote: > It might be easier to use WriteFile() from base/file_util.h here. I agree, but that would remove a test ("created"). Is that ok ? https://codereview.chromium.org/12703012/diff/37001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util_unittest.cc:630: if (!kFilteringTestCases[i].is_directory && On 2013/04/03 18:54:14, vandebo wrote: > nit: reverse the case of the if and use continue. Done. https://codereview.chromium.org/12703012/diff/37001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util_unittest.cc:630: if (!kFilteringTestCases[i].is_directory && On 2013/04/03 18:54:14, vandebo wrote: > nit: reverse the case of the if and use continue. Done. https://codereview.chromium.org/12703012/diff/37001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util_unittest.cc:635: if (kFilteringTestCases[i].media_file) { On 2013/04/03 18:54:14, vandebo wrote: > nit: use an "expected" variable and assign it based on this condition. That > will let you pull the duplicated code out of the if/else. Done. https://codereview.chromium.org/12703012/diff/37001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util_unittest.cc:637: operation->CreateSnapshotFile(url, On 2013/04/03 18:54:14, vandebo wrote: > After op->CrateSnapshotFile, you should call > MessageLoop::current()->RunUntilIdle() to get all the async calls to run. Did > you already try that? It looks like you were trying to make things synchronous > for debugging. Done.
LGTM with one required nit. https://codereview.chromium.org/12703012/diff/37001/webkit/fileapi/media/nati... File webkit/fileapi/media/native_media_file_util_unittest.cc (right): https://codereview.chromium.org/12703012/diff/37001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util_unittest.cc:97: NativeFileUtil::CreateOrOpen( On 2013/04/04 16:07:28, Kevin Bailey wrote: > On 2013/04/03 18:54:14, vandebo wrote: > > It might be easier to use WriteFile() from base/file_util.h here. > > I agree, but that would remove a test ("created"). Is that ok ? I think it's ok to remove the assertion that we just created it. We should still assert that it exists though. https://codereview.chromium.org/12703012/diff/49001/webkit/fileapi/media/nati... File webkit/fileapi/media/native_media_file_util_unittest.cc (right): https://codereview.chromium.org/12703012/diff/49001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util_unittest.cc:43: // Directory should always be visible. Should init media_file and content for all the entries.
https://codereview.chromium.org/12703012/diff/37001/webkit/fileapi/media/nati... File webkit/fileapi/media/native_media_file_util_unittest.cc (right): https://codereview.chromium.org/12703012/diff/37001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util_unittest.cc:97: NativeFileUtil::CreateOrOpen( Is this what you had in mind ? https://codereview.chromium.org/12703012/diff/49001/webkit/fileapi/media/nati... File webkit/fileapi/media/native_media_file_util_unittest.cc (right): https://codereview.chromium.org/12703012/diff/49001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util_unittest.cc:43: // Directory should always be visible. On 2013/04/04 20:24:20, vandebo wrote: > Should init media_file and content for all the entries. I didn't want to clutter it with unnecessary init. Done.
LGTM with nit. Should we commit this now, or hold off until you're able to add mime types for the other supported types? https://codereview.chromium.org/12703012/diff/55001/webkit/fileapi/media/nati... File webkit/fileapi/media/native_media_file_util_unittest.cc (right): https://codereview.chromium.org/12703012/diff/55001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util_unittest.cc:99: ASSERT_NE((const char*)NULL, test_cases[i].content); nit: we don't use C style casts and iirc, the assert macro doesn't deal with the mixed types very well, so you can just ASSERT(test_cases[i].content != NULL);
vandebo@chromium.org wrote: > Should we commit this now, or hold off until you're able > to add mime types for the other supported types? That was my assumption. Otherwise people won't be able to load certain files. Alternatively, if we're concerned about the code drifting, I could check in a more permissive version of IsMediaFile(). https://codereview.chromium.org/12703012/diff/55001/webkit/fileapi/media/nati... File webkit/fileapi/media/native_media_file_util_unittest.cc (right): https://codereview.chromium.org/12703012/diff/55001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util_unittest.cc:99: ASSERT_NE((const char*)NULL, test_cases[i].content); Right, the cast was to make it happy. Done.
On 2013/04/05 20:39:51, Kevin Bailey wrote: > mailto:vandebo@chromium.org wrote: > > Should we commit this now, or hold off until you're able > > to add mime types for the other supported types? > > That was my assumption. Otherwise people won't be able to > load certain files. Alternatively, if we're concerned about > the code drifting, I could check in a more permissive > version of IsMediaFile(). I don't expect the code to change much, let's just hold off until we can get at least the most common mime types supported. Have you made a list of the missing mime types?
On Fri, Apr 5, 2013 at 1:42 PM, <vandebo@chromium.org> wrote: > > I don't expect the code to change much, let's just hold off until we can > get at > least the most common mime types supported. Have you made a list of the > missing > mime types? > Almost. The *most* common are already supported, mostly image files, e.g. jpeg, png. I'm concerned about some others though. For example, the code can't handle the complex pattern match of mpeg. Worse, it intentionally avoids matching, for example, .flv. I'll create a definitive list next.
https://codereview.chromium.org/12703012/diff/60001/webkit/fileapi/media/nati... File webkit/fileapi/media/native_media_file_util.cc (right): https://codereview.chromium.org/12703012/diff/60001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util.cc:24: class ScopedPlatformFileClose { I was looking at this because I was going to copy it into some code that I'm writing and I realized that it actually leaks memory. At the call site you do new base::PlatformFile, but don't free it here. You could, but I think using ScopedGenericObj is better - you won't have to create new PlatformFile and can just close the file like you have here.
https://codereview.chromium.org/12703012/diff/60001/webkit/fileapi/media/nati... File webkit/fileapi/media/native_media_file_util.cc (right): https://codereview.chromium.org/12703012/diff/60001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util.cc:24: class ScopedPlatformFileClose { Ah, my deleter is instead of, not in addition to. Will fix as soon as this new Webkit finishes sync'ing.
https://codereview.chromium.org/12703012/diff/60001/webkit/fileapi/media/nati... File webkit/fileapi/media/native_media_file_util.cc (right): https://codereview.chromium.org/12703012/diff/60001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util.cc:24: class ScopedPlatformFileClose { On 2013/04/12 18:14:23, Kevin Bailey wrote: > Ah, my deleter is instead of, not in addition to. Will fix as soon as this new > Webkit finishes sync'ing. Done.
https://codereview.chromium.org/12703012/diff/67001/webkit/fileapi/media/nati... File webkit/fileapi/media/native_media_file_util.cc (right): https://codereview.chromium.org/12703012/diff/67001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util.cc:28: base::ClosePlatformFile(file); I think you need to check that the file handle is valid before closing it. For example, see https://code.google.com/p/chromium/codesearch/#chromium/src/base/platform_fil...
https://codereview.chromium.org/12703012/diff/67001/webkit/fileapi/media/nati... File webkit/fileapi/media/native_media_file_util.cc (right): https://codereview.chromium.org/12703012/diff/67001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util.cc:28: base::ClosePlatformFile(file); I can of course make that tiny change, but realize it won't do anything about the real danger: Creating a ScopedPlatformFile with an uninitialized handle, or worse, one that happens to point to a opened file. Functions like NativeFileUtil::CreateOrOpen() don't set the handle on error, so you really have to check anyways.
LGTM https://codereview.chromium.org/12703012/diff/67001/webkit/fileapi/media/nati... File webkit/fileapi/media/native_media_file_util.cc (right): https://codereview.chromium.org/12703012/diff/67001/webkit/fileapi/media/nati... webkit/fileapi/media/native_media_file_util.cc:28: base::ClosePlatformFile(file); On 2013/04/12 22:58:35, Kevin Bailey wrote: > I can of course make that tiny change, but realize it won't do anything about > the real danger: Creating a ScopedPlatformFile with an uninitialized handle, or > worse, one that happens to point to a opened file. Functions like > NativeFileUtil::CreateOrOpen() don't set the handle on error, so you really have > to check anyways. It looks like you're current use of it is ok. The code checks that the there was no error before assigning to the ScopedPlatformFile
I moved the MIME sniffer changes into this CL, and made them so that they don't affect the current MIME sniffer behavior. I err'd on the side of touching the existing code as little as possible. Please comment if you think things should be merged more.
Also, you'll want to rebase your change - the media directoy was moved to chrome/browser/media_galleries/fileapi/ https://codereview.chromium.org/12703012/diff/86001/net/base/mime_sniffer.cc File net/base/mime_sniffer.cc (right): https://codereview.chromium.org/12703012/diff/86001/net/base/mime_sniffer.cc#... net/base/mime_sniffer.cc:119: struct MagicMaskNumber { I think it's ok to modify MagicNumber. It's an improvement to the infrastructure with very little cost. https://codereview.chromium.org/12703012/diff/86001/net/base/mime_sniffer.cc#... net/base/mime_sniffer.cc:190: static const MagicMaskNumber kMagicMaskNumbers[] = { These aren't all mask'd magic numbers... maybe kExtraMagicNumbers[] ? https://codereview.chromium.org/12703012/diff/86001/net/base/mime_sniffer.h File net/base/mime_sniffer.h (right): https://codereview.chromium.org/12703012/diff/86001/net/base/mime_sniffer.h#n... net/base/mime_sniffer.h:22: NET_EXPORT bool IdentifyExtraMimeType(const char* content, nit: add a comment and move below SniffMimeType
Also handled changes to ...FileUtil::CreateSnapshotFile. https://codereview.chromium.org/12703012/diff/86001/net/base/mime_sniffer.cc File net/base/mime_sniffer.cc (right): https://codereview.chromium.org/12703012/diff/86001/net/base/mime_sniffer.cc#... net/base/mime_sniffer.cc:119: struct MagicMaskNumber { On 2013/04/29 19:43:50, vandebo wrote: > I think it's ok to modify MagicNumber. It's an improvement to the > infrastructure with very little cost. Done. https://codereview.chromium.org/12703012/diff/86001/net/base/mime_sniffer.cc#... net/base/mime_sniffer.cc:190: static const MagicMaskNumber kMagicMaskNumbers[] = { On 2013/04/29 19:43:50, vandebo wrote: > These aren't all mask'd magic numbers... maybe kExtraMagicNumbers[] ? Done. https://codereview.chromium.org/12703012/diff/86001/net/base/mime_sniffer.h File net/base/mime_sniffer.h (right): https://codereview.chromium.org/12703012/diff/86001/net/base/mime_sniffer.h#n... net/base/mime_sniffer.h:22: NET_EXPORT bool IdentifyExtraMimeType(const char* content, On 2013/04/29 19:43:50, vandebo wrote: > nit: add a comment and move below SniffMimeType Done.
+abarth Are these mime sniffer changes ok? +kinuko things have moved around a bit, does the the fileapi part still look ok? https://codereview.chromium.org/12703012/diff/96001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc (right): https://codereview.chromium.org/12703012/diff/96001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:76: if (*error == base::PLATFORM_FILE_ERROR_SECURITY) nit: {}'s because the body is multi line. https://codereview.chromium.org/12703012/diff/96001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/device_media_async_file_util.h (right): https://codereview.chromium.org/12703012/diff/96001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/device_media_async_file_util.h:152: void OnDidCheckMediaRunTask( nit: I think this should be just OnDidCheckMedia. https://codereview.chromium.org/12703012/diff/96001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/native_media_file_util.cc (right): https://codereview.chromium.org/12703012/diff/96001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/native_media_file_util.cc:268: else nit: remove else, just return. https://codereview.chromium.org/12703012/diff/96001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/native_media_file_util.cc:285: // Read as much as SniffMimeType() will bother looking at. nit: update comment. https://codereview.chromium.org/12703012/diff/96001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/native_media_file_util_unittest.cc (right): https://codereview.chromium.org/12703012/diff/96001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/native_media_file_util_unittest.cc:640: base::PlatformFileError expected_error, error; nit: initialize error to something other than what you expect.
I don't think this logic should be in mime_sniffer.cc. The job of mime_sniffer.cpp is to implement http://mimesniff.spec.whatwg.org/. The code you're adding is unrelated to the mime sniffing spec and therefore does not belong in that file.
On 2013/04/30 23:40:32, abarth wrote: > I don't think this logic should be in mime_sniffer.cc. The job of > mime_sniffer.cpp is to implement http://mimesniff.spec.whatwg.org/. The code > you're adding is unrelated to the mime sniffing spec and therefore does not > belong in that file. There is a bit of infrastructure in this file. Should we duplicate it, or move most of it out to base/ and consume the infrastructure from multiple places?
On 2013/04/30 23:43:55, vandebo wrote: > There is a bit of infrastructure in this file. Should we duplicate it, or move > most of it out to base/ and consume the infrastructure from multiple places? I'd probably just duplicate it in the chrome layer. It's only like five lines of code.
On 2013/04/30 23:50:53, abarth wrote: > On 2013/04/30 23:43:55, vandebo wrote: > > There is a bit of infrastructure in this file. Should we duplicate it, or > move > > most of it out to base/ and consume the infrastructure from multiple places? > > I'd probably just duplicate it in the chrome layer. It's only like five lines > of code. Deleting all the code from mime_sniffer.cc that we won't use, I'm left with roughly 100 lines. I find it distasteful to have a second copy of that much, but the risk of bugs because of the duplication is low so if that's the call then we'll make our own version.
On 2013/05/01 00:02:21, vandebo wrote: > Deleting all the code from mime_sniffer.cc that we won't use, I'm left with > roughly 100 lines. I find it distasteful to have a second copy of that much, > but the risk of bugs because of the duplication is low so if that's the call > then we'll make our own version. Maybe I don't understand what all you're re-using. Isn't it just a strcmp?
On 2013/05/01 00:11:04, abarth wrote: > On 2013/05/01 00:02:21, vandebo wrote: > > Deleting all the code from mime_sniffer.cc that we won't use, I'm left with > > roughly 100 lines. I find it distasteful to have a second copy of that much, > > but the risk of bugs because of the duplication is low so if that's the call > > then we'll make our own version. > > Maybe I don't understand what all you're re-using. Isn't it just a strcmp? All the audio/video/image types from kMagicNumbers, and all the infrastructure to do binary sniffing (CheckForMagicNumbers, MatchMagicNumber, MagicCmp, etc).
https://codereview.chromium.org/12703012/diff/96001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc (right): https://codereview.chromium.org/12703012/diff/96001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:337: callback, base::Unretained(media_task_runner), Please pass media_task_runner as a refptr, i.e. make_scoped_refptr(media_task_runner) so that the task can keep a ref to the runner https://codereview.chromium.org/12703012/diff/96001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:338: file_info, platform_path, base::Owned(error))); nit: You can create a ShareableFileReference earlier in this method and pass the ref (rather than a platform_path) to OnDidCheckMediaRunTask, and it'd be slightly safer, i.e. has less possibility to leak a temporary file. You can probably remove the line 78 (manual file deletion on failure) then. And if you delete the line 78 I guess you can also remove the indirect CheckMediaFile function? like: media_task_runner->PostTaskAndReplyWith( FROM_HERE, base::Bind(&NativeMediaFileUtil::IsMediaFile, file_path), base::Bind(&DeviceMediaAsyncFileUtil::...., file_ref));
Hi abarth, I suspect we haven't described the reason why we're using the MIME sniffer library very well. Let me do that and I'm sure you will see the value of re-using the code and data. We're modifying the media gallery code to be more strict about what type of file it will open. Previously, it only looked at a file's extension but, as your paper mentions, this isn't sufficient. Therefore we want to make it look into the file to verify that a file is a media type file before allowing it to be opened, viewed, uploaded, etc. We would have simply used SniffMimeType() untouched - it does exactly what we need - except that there are a few MIME types that we need which it doesn't identify. Ironically, many of these types are mentioned in your paper, such as mpeg, ogg and wav. I initially added these types, including partial byte matching, to the existing table, but we realized that the calling code might have assumptions about *which* MIME types would be identified. Therefore, we made a separate function which recognized the additional types, leaving the existing code unchanged functionally. It seems to me as though we're doing very nearly the same thing. Certainly, we're reading files off the filesystem while it appears you are sniffing network data, but your paper mentions both (section 6.3). For this reason, I don't understand why we would want to put this code any other place. My main concern is not the added size of duplicating the code and tables elsewhere. My concern is more about having one authoritative library for detecting MIME types. It can be tricky identifying these types - it's not an exact science, there is lots of overlap, it's easy to leave out legitimate patterns, etc. It would be too easy for two (or more) libraries to grow apart. Additionally, we would very much like your feedback if we are doing the detection incorrectly. We probably wouldn't get that if we put it elsewhere. Apologies if that was too long. I hope you can see the value in keeping this expertise in one place.
I'm out of the office until Monday. I'll reply when I get back. Sorry for delaying this CL. :( If you're willing to address feedback when I get back, you can go ahead and land it and get unblocked. On Wed, May 1, 2013 at 7:09 AM, <krb@chromium.org> wrote: > Hi abarth, > > I suspect we haven't described the reason why we're using the MIME sniffer > library very well. Let me do that and I'm sure you will see the value of > re-using the code and data. > > We're modifying the media gallery code to be more strict about what type > of file > it will open. Previously, it only looked at a file's extension but, as your > paper mentions, this isn't sufficient. Therefore we want to make it look > into > the file to verify that a file is a media type file before allowing it to > be > opened, viewed, uploaded, etc. > > We would have simply used SniffMimeType() untouched - it does exactly what > we > need - except that there are a few MIME types that we need which it doesn't > identify. Ironically, many of these types are mentioned in your paper, > such as > mpeg, ogg and wav. > > I initially added these types, including partial byte matching, to the > existing > table, but we realized that the calling code might have assumptions about > *which* MIME types would be identified. Therefore, we made a separate > function > which recognized the additional types, leaving the existing code unchanged > functionally. > > It seems to me as though we're doing very nearly the same thing. Certainly, > we're reading files off the filesystem while it appears you are sniffing > network > data, but your paper mentions both (section 6.3). For this reason, I don't > understand why we would want to put this code any other place. > > My main concern is not the added size of duplicating the code and tables > elsewhere. My concern is more about having one authoritative library for > detecting MIME types. It can be tricky identifying these types - it's not > an > exact science, there is lots of overlap, it's easy to leave out legitimate > patterns, etc. It would be too easy for two (or more) libraries to grow > apart. > Additionally, we would very much like your feedback if we are doing the > detection incorrectly. We probably wouldn't get that if we put it > elsewhere. > > Apologies if that was too long. I hope you can see the value in keeping > this > expertise in one place. > > https://codereview.chromium.**org/12703012/<https://codereview.chromium.org/1... >
https://codereview.chromium.org/12703012/diff/96001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc (right): https://codereview.chromium.org/12703012/diff/96001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:76: if (*error == base::PLATFORM_FILE_ERROR_SECURITY) On 2013/04/30 21:20:36, vandebo wrote: > nit: {}'s because the body is multi line. Removed function. https://codereview.chromium.org/12703012/diff/96001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:337: callback, base::Unretained(media_task_runner), On 2013/05/01 02:03:46, kinuko wrote: > Please pass media_task_runner as a refptr, i.e. > > make_scoped_refptr(media_task_runner) > > so that the task can keep a ref to the runner Done. It's unfortunate that it doesn't do that automatically since it seems to know that it's a reference counted object. https://codereview.chromium.org/12703012/diff/96001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:338: file_info, platform_path, base::Owned(error))); I moved the GetOrCreate upstream but it seems like overkill. All I really want is something that will remove the file if something fails. A ScopedFile would suffice, however, ScopedFile isn't very friendly (copyable) and I didn't want to go so far as to wrap it in a scoped_ptr. Also, the callback wants a ShareableFileReference. BTW, CreateSnapshotFileCallback can remove the FilePath parameter since it can get it from the ShareableFileReference. https://codereview.chromium.org/12703012/diff/96001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/device_media_async_file_util.h (right): https://codereview.chromium.org/12703012/diff/96001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/device_media_async_file_util.h:152: void OnDidCheckMediaRunTask( On 2013/04/30 21:20:36, vandebo wrote: > nit: I think this should be just OnDidCheckMedia. Done. https://codereview.chromium.org/12703012/diff/96001/chrome/browser/media_gall... File chrome/browser/media_galleries/fileapi/native_media_file_util.cc (right): https://codereview.chromium.org/12703012/diff/96001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/native_media_file_util.cc:268: else On 2013/04/30 21:20:36, vandebo wrote: > nit: remove else, just return. Done. https://codereview.chromium.org/12703012/diff/96001/chrome/browser/media_gall... chrome/browser/media_galleries/fileapi/native_media_file_util.cc:285: // Read as much as SniffMimeType() will bother looking at. On 2013/04/30 21:20:36, vandebo wrote: > nit: update comment. Done.
LGTM after the following changes. https://codereview.chromium.org/12703012/diff/121001/chrome/browser/media_gal... File chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc (right): https://codereview.chromium.org/12703012/diff/121001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:345: if (*error == base::PLATFORM_FILE_OK) { Instead of doing the if else with the callback.Run in both, just do (it's less duplicated code) if (*error != base::PLATFORM_FILE_OK) platform_file = NULL; callback.Run(...) https://codereview.chromium.org/12703012/diff/121001/net/base/mime_sniffer.cc File net/base/mime_sniffer.cc (right): https://codereview.chromium.org/12703012/diff/121001/net/base/mime_sniffer.cc... net/base/mime_sniffer.cc:117: const char* mask; // must have same length as |magic| nit: If not NULL, must have same length as |magic|. https://codereview.chromium.org/12703012/diff/121001/net/base/mime_sniffer.h File net/base/mime_sniffer.h (right): https://codereview.chromium.org/12703012/diff/121001/net/base/mime_sniffer.h#... net/base/mime_sniffer.h:52: // @return Returns true if we have enough content to guess the mime type. Returns true if a mime-type match was found.
https://codereview.chromium.org/12703012/diff/121001/chrome/browser/media_gal... File chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc (right): https://codereview.chromium.org/12703012/diff/121001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/device_media_async_file_util.cc:345: if (*error == base::PLATFORM_FILE_OK) { On 2013/05/02 21:58:24, vandebo wrote: > Instead of doing the if else with the callback.Run in both, just do (it's less > duplicated code) > > if (*error != base::PLATFORM_FILE_OK) > platform_file = NULL; > callback.Run(...) Hope this is what you meant. https://codereview.chromium.org/12703012/diff/121001/net/base/mime_sniffer.cc File net/base/mime_sniffer.cc (right): https://codereview.chromium.org/12703012/diff/121001/net/base/mime_sniffer.cc... net/base/mime_sniffer.cc:117: const char* mask; // must have same length as |magic| On 2013/05/02 21:58:24, vandebo wrote: > nit: If not NULL, must have same length as |magic|. Not nit, good point. https://codereview.chromium.org/12703012/diff/121001/net/base/mime_sniffer.h File net/base/mime_sniffer.h (right): https://codereview.chromium.org/12703012/diff/121001/net/base/mime_sniffer.h#... net/base/mime_sniffer.h:52: // @return Returns true if we have enough content to guess the mime type. On 2013/05/02 21:58:24, vandebo wrote: > Returns true if a mime-type match was found. Done.
+rsleevi for net OWNERS stamp - abarth is ok with this for now, unless you have any generic comments on the net changes. LGTM
LGTM with nits/reservations. https://codereview.chromium.org/12703012/diff/133001/net/base/mime_sniffer.cc File net/base/mime_sniffer.cc (right): https://codereview.chromium.org/12703012/diff/133001/net/base/mime_sniffer.cc... net/base/mime_sniffer.cc:124: { (mime_type), (magic), sizeof(magic)-1, false, (mask) }, Is there any sort of compiler trickery you can use here to ensure that sizeof(magic)-1 == sizeof(mask)--1. Seems a very subtle element that required careful counting of the bytes, and should be possible to do at the compiler level since these are fixed strings. https://codereview.chromium.org/12703012/diff/133001/net/base/mime_sniffer.cc... net/base/mime_sniffer.cc:258: size_t len, const char* mask) { style nit (oh Chromium-dev centi-thread...) static bool MagicMaskCmp(const char* magic_entry, const char* content, size_t len, const char* mask) { } Context: Line 270 is(was) relying on the exception to the "one arg per line, except when it's something like a buffer+size", but that was replaced with the generic "all or nothing" in http://dev.chromium.org/developers/coding-style https://codereview.chromium.org/12703012/diff/133001/net/base/mime_sniffer.cc... net/base/mime_sniffer.cc:298: magic_entry->mask); style nit: match = MagicMaskCmp(magic_entry->magic, content, len, magic_entry->mask); As per http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... , wrap arguments at the parens https://codereview.chromium.org/12703012/diff/133001/net/base/mime_sniffer.cc... net/base/mime_sniffer.cc:733: std::string* result) { style nit: one arg per line (and I'll throw in a cookie if you can fix 658-660 and 270 while you're here, since they're the only ones violating it in this file) https://codereview.chromium.org/12703012/diff/133001/net/base/mime_sniffer.cc... net/base/mime_sniffer.cc:741: sizeof(kMagicNumbers) / sizeof(MagicNumber), Both of these calls should be using arraysize() from base/basictypes.h https://codereview.chromium.org/12703012/diff/133001/net/base/mime_sniffer.h File net/base/mime_sniffer.h (right): https://codereview.chromium.org/12703012/diff/133001/net/base/mime_sniffer.h#... net/base/mime_sniffer.h:47: // otherwise returns false. So I realize that you plan to work with Adam to address (further) feedback on this Monday, but it would be good to add a comment indicating the appropriateness - or inappropriateness - of calling this function. eg: "Note: This should not be used when handling data that comes from untrusted sources - eg: a network or filesystem. For such cases, use SniffMimeType, which is meant to strictly adhere to http://mimesniff.spec.whatwg.org/ and be compatible/inter-operable with other implementations. IdentifyExtraMimeType may be used to [...]" Or just *SOMETHING* to indicate that this is not a blank cheque to go calling this function willy nilly. I'm sure Adam will have a much better comment suggestion on his return.
https://codereview.chromium.org/12703012/diff/133001/net/base/mime_sniffer.h File net/base/mime_sniffer.h (right): https://codereview.chromium.org/12703012/diff/133001/net/base/mime_sniffer.h#... net/base/mime_sniffer.h:47: // otherwise returns false. On 2013/05/03 19:03:24, Ryan Sleevi wrote: > Or just *SOMETHING* to indicate that this is not a blank cheque to go calling > this function willy nilly. Yeah, I think it's important to pick a name for this function and to add a comment explaining when it's appropriate to call each function. Generally speaking, SniffMimeType should be called for untrusted data and for network traffic. The new function should can be called only for trusted, local data.
https://codereview.chromium.org/12703012/diff/133001/net/base/mime_sniffer.h File net/base/mime_sniffer.h (right): https://codereview.chromium.org/12703012/diff/133001/net/base/mime_sniffer.h#... net/base/mime_sniffer.h:53: NET_EXPORT bool IdentifyExtraMimeType(const char* content, Maybe "SniffMimeTypeFromTrustedLocalData" would be a better name?
On 2013/05/06 17:54:11, abarth wrote: > https://codereview.chromium.org/12703012/diff/133001/net/base/mime_sniffer.h > File net/base/mime_sniffer.h (right): > > https://codereview.chromium.org/12703012/diff/133001/net/base/mime_sniffer.h#... > net/base/mime_sniffer.h:47: // otherwise returns false. > On 2013/05/03 19:03:24, Ryan Sleevi wrote: > > Or just *SOMETHING* to indicate that this is not a blank cheque to go calling > > this function willy nilly. > > Yeah, I think it's important to pick a name for this function and to add a > comment explaining when it's appropriate to call each function. > > Generally speaking, SniffMimeType should be called for untrusted data and for > network traffic. The new function should can be called only for trusted, local > data. Why do you say it should be used for trusted data only? If the data was fully trusted, we could just use the filename extension. I understand that it might be easy to fool mime sniffing because some of the patterns are pretty weak but there's nothing dangerous about attempting to sniff the data, right?
On 2013/05/06 18:02:46, vandebo wrote: > Why do you say it should be used for trusted data only? Because one of the reasons we use a carefully chosen set of sniffing rules is to prevent untrusted content from tricking us into interpreting it with a different MIME type than the provider of the data intended. As an example, the provider of the content might not expect us to interpret a piece of data as application/x-shockwave-flash just because it starts with the string "FWS". The point of writing a specification for how content sniffing works is so that the sender and receiver of data can agree on how the data ought to be interpreted. > If the data was fully trusted, we could just use the filename extension. I'm not sure I agree with that statement. For example, you might trust the content of the data without trusting the file name associated with the data. > I understand that it might > be easy to fool mime sniffing because some of the patterns are pretty weak but > there's nothing dangerous about attempting to sniff the data, right? Yes, there is indeed something dangerous about attempting to sniff the data. There's much more information in this paper if you're interested in some of the subtly issues here: http://www.adambarth.com/papers/2009/barth-caballero-song.pdf
I agree that the function we want to add should be clearly commented with respect to what are and aren't valid use cases for it. I'm trying to soften the warning you've suggested so far because technically, we want to use the function on untrusted data (the content is not controlled by Chrome). I think the threat model is different though. For this application, a Chrome App API give access to media files on the user's disk. Files are first filtered by filename extension and then mime-type sniffed to confirm that they are plausibly media files. The would be attacker (the App) doesn't control the file content, the attacker is the one receiving the file. On 2013/05/06 19:48:24, abarth wrote: > On 2013/05/06 18:02:46, vandebo wrote: > > Why do you say it should be used for trusted data only? > > Because one of the reasons we use a carefully chosen set of sniffing rules is to > prevent untrusted content from tricking us into interpreting it with a different > MIME type than the provider of the data intended. > > As an example, the provider of the content might not expect us to interpret a > piece of data as application/x-shockwave-flash just because it starts with the > string "FWS". The point of writing a specification for how content sniffing > works is so that the sender and receiver of data can agree on how the data ought > to be interpreted. > > > If the data was fully trusted, we could just use the filename extension. > > I'm not sure I agree with that statement. For example, you might trust the > content of the data without trusting the file name associated with the data. > > > I understand that it might > > be easy to fool mime sniffing because some of the patterns are pretty weak but > > there's nothing dangerous about attempting to sniff the data, right? > > Yes, there is indeed something dangerous about attempting to sniff the data. > There's much more information in this paper if you're interested in some of the > subtly issues here: > > http://www.adambarth.com/papers/2009/barth-caballero-song.pdf
I think the issue is that "trusted" is too coarse a word. Maybe we should just say "local" ?
On 2013/05/07 01:01:04, abarth wrote: > I think the issue is that "trusted" is too coarse a word. Maybe we should just > say "local" ? SGTM, thanks. Kevin will take care of that.
On 2013/05/07 01:02:37, vandebo wrote: > > SGTM, thanks. Kevin will take care of that. Done.
Belated, but here you go. https://codereview.chromium.org/12703012/diff/133001/net/base/mime_sniffer.cc File net/base/mime_sniffer.cc (right): https://codereview.chromium.org/12703012/diff/133001/net/base/mime_sniffer.cc... net/base/mime_sniffer.cc:124: { (mime_type), (magic), sizeof(magic)-1, false, (mask) }, On 2013/05/03 19:03:24, Ryan Sleevi wrote: > Is there any sort of compiler trickery you can use here to ensure that > sizeof(magic)-1 == sizeof(mask)--1. Seems a very subtle element that required > careful counting of the bytes, and should be possible to do at the compiler > level since these are fixed strings. I verified that it catches mismatches. Let me know what you think. https://codereview.chromium.org/12703012/diff/133001/net/base/mime_sniffer.cc... net/base/mime_sniffer.cc:258: size_t len, const char* mask) { On 2013/05/03 19:03:24, Ryan Sleevi wrote: > style nit (oh Chromium-dev centi-thread...) > > static bool MagicMaskCmp(const char* magic_entry, > const char* content, > size_t len, > const char* mask) { > > } > > Context: Line 270 is(was) relying on the exception to the "one arg per line, > except when it's something like a buffer+size", but that was replaced with the > generic "all or nothing" in http://dev.chromium.org/developers/coding-style Done. https://codereview.chromium.org/12703012/diff/133001/net/base/mime_sniffer.cc... net/base/mime_sniffer.cc:298: magic_entry->mask); On 2013/05/03 19:03:24, Ryan Sleevi wrote: > style nit: > match = MagicMaskCmp(magic_entry->magic, content, len, > magic_entry->mask); > > As per > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... > , wrap arguments at the parens Done. https://codereview.chromium.org/12703012/diff/133001/net/base/mime_sniffer.cc... net/base/mime_sniffer.cc:733: std::string* result) { On 2013/05/03 19:03:24, Ryan Sleevi wrote: > style nit: one arg per line (and I'll throw in a cookie if you can fix 658-660 > and 270 while you're here, since they're the only ones violating it in this > file) Done, assuming that's what you meant. https://codereview.chromium.org/12703012/diff/133001/net/base/mime_sniffer.cc... net/base/mime_sniffer.cc:741: sizeof(kMagicNumbers) / sizeof(MagicNumber), On 2013/05/03 19:03:24, Ryan Sleevi wrote: > Both of these calls should be using arraysize() from base/basictypes.h Done.
LGTM https://codereview.chromium.org/12703012/diff/157001/net/base/mime_sniffer.cc File net/base/mime_sniffer.cc (right): https://codereview.chromium.org/12703012/diff/157001/net/base/mime_sniffer.cc... net/base/mime_sniffer.cc:753: NULL, result)) nit: line wrap. https://codereview.chromium.org/12703012/diff/157001/net/base/mime_sniffer.cc... net/base/mime_sniffer.cc:757: arraysize(kMagicNumbers), nit: line wrap.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/krb@chromium.org/12703012/168001
Message was sent while issue was closed.
Change committed as 199587 |
