Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1261333004:
Add support for Flash Player Component updates on Linux (Closed)
|
Created:
5 years, 4 months ago by Greg K Modified:
5 years, 4 months ago Reviewers:
Bernhard Bauer, jochen (gone - plz use gerrit), rickyz (no longer on Chrome), jln (very slow on Chromium), Lei Zhang CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for Flash Player Component updates on Linux
This change adds support for Flash Player Component updates on Linux.
It enables the component updater on Linux, and creates a hints file that
the zygote will use to locate and preload the latest component updated flash.
This should not affect component update OS X and Windows, however, because
it forks the update path on Linux from those platforms, there is a risk of a bug
that could affect component update on OS X and Windows.
BUG=460595
Committed: https://crrev.com/2bb08b2a5f6bcc382ebaccf1b50bd655592dfe51
Cr-Commit-Position: refs/heads/master@{#343435}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Rebase to fix merge conflicts #
Total comments: 14
Patch Set 3 : Use SecureHash, fix up unit tests, general cleanup #
Total comments: 54
Patch Set 4 : Updates per jln's code review. #
Total comments: 10
Patch Set 5 : Further refinements. #
Total comments: 44
Patch Set 6 : Updates per bauerb's code review. #
Total comments: 2
Patch Set 7 : Fix up bauerb's final few comments. #
Total comments: 20
Messages
Total messages: 37 (16 generated)
rickyz@chromium.org changed reviewers: + rickyz@chromium.org
https://codereview.chromium.org/1261333004/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/1261333004/diff/1/chrome/browser/component_up... chrome/browser/component_updater/pepper_flash_component_installer.cc:16: #include "base/files/important_file_writer.h" Are these still used in this file? https://codereview.chromium.org/1261333004/diff/1/chrome/browser/component_up... chrome/browser/component_updater/pepper_flash_component_installer.cc:245: if (!hint_file.RecordFlashUpdate(unpacked_plugin, flash_path, I'd probably prefer to write the new flash library first, then record the update, the rationale being that the way it currently is, if the process dies between the move and the revert, the next chrome launch will fall back to the bundled version (as opposed to the previously component update version). If the write/record were switched, then the only negative consequence would be an unused copy of flash sitting in the directory, which seems slightly less bad to me (though you'd probably also have to handle the target existing in the move). https://codereview.chromium.org/1261333004/diff/1/chrome/common/chrome_conten... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1261333004/diff/1/chrome/common/chrome_conten... chrome/common/chrome_content_client.cc:315: return true; Is there any handling for when DIR_USER_DATA is on a filesystem mounted noexec? Luckily, we're unsandboxed here, so we can test for this by attempting to mmap a page PROT_READ | PROT_EXEC. https://codereview.chromium.org/1261333004/diff/1/chrome/common/component_fla... File chrome/common/component_flash_hint_file.cc (right): https://codereview.chromium.org/1261333004/diff/1/chrome/common/component_fla... chrome/common/component_flash_hint_file.cc:74: int version, Don't think this param is used - kCurrentHintFileVersion is used below instead, which makes sense. https://codereview.chromium.org/1261333004/diff/1/chrome/common/component_fla... chrome/common/component_flash_hint_file.cc:87: if (!ReadFileToString(hint_file_path, &previous_hint_file_)) Might it be easier to back it up to a file instead? https://codereview.chromium.org/1261333004/diff/1/chrome/common/component_fla... chrome/common/component_flash_hint_file.cc:178: DCHECK(hashes_same); I'd probably replace this with a LOG(WARNING) instead.
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
kerrnel@chromium.org changed reviewers: + jln@chromium.org
https://codereview.chromium.org/1261333004/diff/1/chrome/browser/component_up... File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/1261333004/diff/1/chrome/browser/component_up... chrome/browser/component_updater/pepper_flash_component_installer.cc:16: #include "base/files/important_file_writer.h" On 2015/07/31 00:13:56, rickyz wrote: > Are these still used in this file? Done. https://codereview.chromium.org/1261333004/diff/1/chrome/browser/component_up... chrome/browser/component_updater/pepper_flash_component_installer.cc:245: if (!hint_file.RecordFlashUpdate(unpacked_plugin, flash_path, On 2015/07/31 00:13:56, rickyz wrote: > I'd probably prefer to write the new flash library first, then record the > update, the rationale being that the way it currently is, if the process dies > between the move and the revert, the next chrome launch will fall back to the > bundled version (as opposed to the previously component update version). > > If the write/record were switched, then the only negative consequence would be > an unused copy of flash sitting in the directory, which seems slightly less bad > to me (though you'd probably also have to handle the target existing in the > move). It's a valid point. As you say, I will need to make sure the installer can deal with the pre-existing directory the next time around if it attempts to retry the installation. Otherwise, if the hint file cannot be written out, the install will permanently fail. https://codereview.chromium.org/1261333004/diff/1/chrome/common/chrome_conten... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1261333004/diff/1/chrome/common/chrome_conten... chrome/common/chrome_content_client.cc:315: return true; On 2015/07/31 00:13:56, rickyz wrote: > Is there any handling for when DIR_USER_DATA is on a filesystem mounted noexec? > Luckily, we're unsandboxed here, so we can test for this by attempting to mmap a > page PROT_READ | PROT_EXEC. Done. https://codereview.chromium.org/1261333004/diff/1/chrome/common/component_fla... File chrome/common/component_flash_hint_file.cc (right): https://codereview.chromium.org/1261333004/diff/1/chrome/common/component_fla... chrome/common/component_flash_hint_file.cc:74: int version, On 2015/07/31 00:13:56, rickyz wrote: > Don't think this param is used - kCurrentHintFileVersion is used below instead, > which makes sense. That is correct, although I corrected it to use the version passed in. This is so that WriteToDisk can handle multiple versions of the file in the future. https://codereview.chromium.org/1261333004/diff/1/chrome/common/component_fla... chrome/common/component_flash_hint_file.cc:87: if (!ReadFileToString(hint_file_path, &previous_hint_file_)) On 2015/07/31 00:13:56, rickyz wrote: > Might it be easier to back it up to a file instead? We don't know to revert at all anymore since the file is always written atomically, so the whole back up is gone. https://codereview.chromium.org/1261333004/diff/1/chrome/common/component_fla... chrome/common/component_flash_hint_file.cc:178: DCHECK(hashes_same); On 2015/07/31 00:13:56, rickyz wrote: > I'd probably replace this with a LOG(WARNING) instead. I went with LOG(ERROR) on the grounds that this condition is an error, but indeed the DCHECK() seemed unnecessary, especially because as you pointed out, an external file is not an invariant that Chrome controls.
I took a 10s look and I'll take a better look tomorrow. https://codereview.chromium.org/1261333004/diff/60001/chrome/common/chrome_co... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1261333004/diff/60001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:323: #endif // defined(FLAPPER_AVAILABLE) You should return something even if !FLAPPER_AVAILABLE. Dis you try to build without FLAPPER_AVAILABLE? This should have generated an error. https://codereview.chromium.org/1261333004/diff/60001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:518: #endif // defined(OS_LINUX) https://codereview.chromium.org/1261333004/diff/60001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:526: #endif // defined(OS_LINUX) https://codereview.chromium.org/1261333004/diff/60001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:546: #endif // defined(ENABLE_PLUGINS) https://codereview.chromium.org/1261333004/diff/60001/chrome/common/component... File chrome/common/component_flash_hint_file.h (right): https://codereview.chromium.org/1261333004/diff/60001/chrome/common/component... chrome/common/component_flash_hint_file.h:21: ComponentFlashHintFile() {} Nit: add destructor explicitly. https://codereview.chromium.org/1261333004/diff/60001/chrome/common/component... chrome/common/component_flash_hint_file.h:54: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1261333004/diff/60001/chrome/common/component... File chrome/common/component_flash_hint_file_linux.cc (right): https://codereview.chromium.org/1261333004/diff/60001/chrome/common/component... chrome/common/component_flash_hint_file_linux.cc:188: if (memcmp(&file_hash[0], string_as_array(&decoded_hash), Ugh. There isn't a better API in crypto for this?
https://codereview.chromium.org/1261333004/diff/60001/chrome/common/chrome_co... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1261333004/diff/60001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:323: #endif // defined(FLAPPER_AVAILABLE) On 2015/08/04 01:08:33, jln wrote: > You should return something even if !FLAPPER_AVAILABLE. > > Dis you try to build without FLAPPER_AVAILABLE? This should have generated an > error. Yes, if I #undef FLAPPER_AVAILABLE it still builds. Unless I'm parsing the #ifdef's wrong, it shouid say #else return false; #endif when !FLAPPER_AVAILABLE, right? https://codereview.chromium.org/1261333004/diff/60001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:518: #endif On 2015/08/04 01:08:33, jln wrote: > // defined(OS_LINUX) Done. https://codereview.chromium.org/1261333004/diff/60001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:526: #endif On 2015/08/04 01:08:33, jln wrote: > // defined(OS_LINUX) Done. https://codereview.chromium.org/1261333004/diff/60001/chrome/common/chrome_co... chrome/common/chrome_content_client.cc:546: #endif On 2015/08/04 01:08:33, jln wrote: > // defined(ENABLE_PLUGINS) Done. https://codereview.chromium.org/1261333004/diff/60001/chrome/common/component... File chrome/common/component_flash_hint_file.h (right): https://codereview.chromium.org/1261333004/diff/60001/chrome/common/component... chrome/common/component_flash_hint_file.h:21: ComponentFlashHintFile() {} On 2015/08/04 01:08:33, jln wrote: > Nit: add destructor explicitly. Done. https://codereview.chromium.org/1261333004/diff/60001/chrome/common/component... chrome/common/component_flash_hint_file.h:54: }; On 2015/08/04 01:08:33, jln wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1261333004/diff/60001/chrome/common/component... File chrome/common/component_flash_hint_file_linux.cc (right): https://codereview.chromium.org/1261333004/diff/60001/chrome/common/component... chrome/common/component_flash_hint_file_linux.cc:188: if (memcmp(&file_hash[0], string_as_array(&decoded_hash), On 2015/08/04 01:08:33, jln wrote: > Ugh. There isn't a better API in crypto for this? I will talk to Sleevi.
Patchset #5 (id:120001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #4 (id:160001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:180001) has been deleted
Looks good in general. A few remarks. I wonder if we could get rid of explicitly checking if the file can be mapped executable. https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/browser/... File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/browser/... chrome/browser/component_updater/pepper_flash_component_installer.cc:106: #if !defined(OS_LINUX) || defined(GOOGLE_CHROME_BUILD) Why is this needed? Wasn't this building unconditionally before? https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/browser/... chrome/browser/component_updater/pepper_flash_component_installer.cc:247: LOG(WARNING) << "Hint file creation failed, but unable to delete " I would upgrade to ERROR, but your choice. https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/chrome_c... File chrome/chrome_common.gypi (right): https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/chrome_c... chrome/chrome_common.gypi:46: 'common/component_flash_hint_file.h', This is interesting. You forgot to update "gn" files, but the gn build is still working. I'm not sure why that is, my understanding was that pepper_flash_component_installer.cc would use the hint class regardless of the configuration. Could you check what's happening? (And add the proper file to the .gn) https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/chrome_t... File chrome/chrome_tests_unit.gypi (right): https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/chrome_t... chrome/chrome_tests_unit.gypi:531: 'common/component_flash_hint_file_unittests_linux.cc', Same remark: don't forget .gn https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... File chrome/common/chrome_content_client.cc (right): https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/chrome_content_client.cc:310: // In case the user's home directory is mounted NOEXEC, or the plugin could s/NOEXEC/noexec/ https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/chrome_content_client.cc:311: // not be verified, do not use the plugin. Log an error if !verified? https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/chrome_content_client.cc:313: chrome::ComponentFlashHintFile::TestExecutableMapping(flash_path)) { Explain why it's useful to bail early in case we can't map it as executable. https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/chrome_content_client.cc:320: return false; Nit: return false outside of the #if #else seems a bit more readable. https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/chrome_content_client.cc:512: // this case. What happens in this case? Is it falling back to the system Flash plugin or not? https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/chrome_content_client.cc:518: std::vector<content::PepperPluginInfo*> flash_versions; This is a little strange because this vector will outlive the objects it is pointing to at the function's epilogue. How about a scoped_vector and allocating these objects on the heap instead? https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/chrome_content_client.cc:526: content::PepperPluginInfo bundled_flash; How is this logic affecting other platforms than Linux? The commit message doesn't say anything, so please make that clear. https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/chrome_content_client.cc:535: std::sort(flash_versions.begin(), flash_versions.end(), I wonder if version comparison shouldn't be it's own separate function that is unit tested. https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/chrome_content_client.cc:542: if (flash_versions.size() > 0) It looks like you only need max_element, not a full sort, no? https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... File chrome/common/component_flash_hint_file.h (right): https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/component_flash_hint_file.h:19: class ComponentFlashHintFile { This class has only static members. You may want to consider using a namespace instead: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Nonmember,_St... But grouping in classes is common. https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/component_flash_hint_file.h:22: // unpacked_plugin is the current location of the plugin. Use |argument| to document argument "argument". https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/component_flash_hint_file.h:30: // the correct hash sum. Mention and document both arguments. https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/component_flash_hint_file.h:36: // NOEXEC. nit: "noexec". Explain why this matters. See my other remark in the caller: why is it useful to treat this failure in a special way? We don't seem to be providing any helpful error message. https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... File chrome/common/component_flash_hint_file_linux.cc (right): https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/component_flash_hint_file_linux.cc:55: CHECK(len == crypto::kSHA256Length); CHECK_EQ() https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/component_flash_hint_file_linux.cc:58: size_t bytes_remaining = mapped_file.length(), bytes_read = 0; Style: one line per variable (can't find where that is in the style guide but definitely true. https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/component_flash_hint_file_linux.cc:64: CHECK(bytes_remaining >= len); CHECK_GE() https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/component_flash_hint_file_linux.cc:65: secure_hash->Update(mapped_file.data() + bytes_read, len); Why force this page by page? Why not ask update to do the full thing? This seems overly complicated. https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/component_flash_hint_file_linux.cc:76: const base::ScopedFD fd(HANDLE_EINTR(open(path.value().c_str(), O_RDONLY))); Do CLOEXEC while you're at it. https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/component_flash_hint_file_linux.cc:79: const MmapDeleter deleter = {.map_size = sizeof(uint8_t)}; Style: I don't think we allow designed initializers like this. https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/component_flash_hint_file_linux.cc:81: reinterpret_cast<uint8_t*>(mmap(NULL, deleter.map_size, Given the simplicity here, I would not use the MmapDeleter and just munmap as needed. https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/component_flash_hint_file_linux.cc:208: if (memcmp(&file_hash[0], string_as_array(&decoded_hash), So there is nothing better than memcmp in crypto/ ? https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... File chrome/common/component_flash_hint_file_unittests_linux.cc (right): https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/component_flash_hint_file_unittests_linux.cc:69: srand48(time(NULL) ^ getpid()); Unit tests should reproduce 100% of the time. If you need randomness yous hould still always use the same seed. I don't think you need randomness here anyways. https://chromiumcodereview.appspot.com/1261333004/diff/200001/chrome/common/c... chrome/common/component_flash_hint_file_unittests_linux.cc:206: CHECK(base::WriteFile(file_path, reinterpret_cast<const char*>(file), If this fails we're not unmounting the path.
Patchset #4 (id:220001) has been deleted
https://codereview.chromium.org/1261333004/diff/200001/chrome/browser/compone... File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/1261333004/diff/200001/chrome/browser/compone... chrome/browser/component_updater/pepper_flash_component_installer.cc:106: #if !defined(OS_LINUX) || defined(GOOGLE_CHROME_BUILD) On 2015/08/06 18:48:14, jln wrote: > Why is this needed? Wasn't this building unconditionally before? On Linux, because PepperFlashComponentInstaller::Install does not call RegisterPepperFlashWithChrome, these functions are all unused, except when GOOGLE_CHROME_BUILD is defined. https://codereview.chromium.org/1261333004/diff/200001/chrome/browser/compone... chrome/browser/component_updater/pepper_flash_component_installer.cc:247: LOG(WARNING) << "Hint file creation failed, but unable to delete " On 2015/08/06 18:48:14, jln wrote: > I would upgrade to ERROR, but your choice. Done. https://codereview.chromium.org/1261333004/diff/200001/chrome/chrome_common.gypi File chrome/chrome_common.gypi (right): https://codereview.chromium.org/1261333004/diff/200001/chrome/chrome_common.g... chrome/chrome_common.gypi:46: 'common/component_flash_hint_file.h', On 2015/08/06 18:48:14, jln wrote: > This is interesting. You forgot to update "gn" files, but the gn build is still > working. > > I'm not sure why that is, my understanding was that > pepper_flash_component_installer.cc would use the hint class regardless of the > configuration. > > Could you check what's happening? (And add the proper file to the .gn) Turns out that part of the codebase isn't using GN. https://codereview.chromium.org/1261333004/diff/200001/chrome/chrome_tests_un... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/1261333004/diff/200001/chrome/chrome_tests_un... chrome/chrome_tests_unit.gypi:531: 'common/component_flash_hint_file_unittests_linux.cc', On 2015/08/06 18:48:14, jln wrote: > Same remark: don't forget .gn Acknowledged. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/chrome_c... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1261333004/diff/200001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:310: // In case the user's home directory is mounted NOEXEC, or the plugin could On 2015/08/06 18:48:14, jln wrote: > s/NOEXEC/noexec/ Done. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:311: // not be verified, do not use the plugin. On 2015/08/06 18:48:14, jln wrote: > Log an error if !verified? Yes, although it's misleading because that function can return false for a number of reasons, and the function already logs if the SHAs don't match up. I will drop the verified bool and just add a log explaining that it failed to find the flash plugin. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:313: chrome::ComponentFlashHintFile::TestExecutableMapping(flash_path)) { On 2015/08/06 18:48:14, jln wrote: > Explain why it's useful to bail early in case we can't map it as executable. > Done. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:320: return false; On 2015/08/06 18:48:14, jln wrote: > Nit: return false outside of the #if #else seems a bit more readable. Done. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:512: // this case. On 2015/08/06 18:48:14, jln wrote: > What happens in this case? Is it falling back to the system Flash plugin or not? I clarified that it will continue to use whatever flash plugin was loaded before sandbox initialization. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:518: std::vector<content::PepperPluginInfo*> flash_versions; On 2015/08/06 18:48:14, jln wrote: > This is a little strange because this vector will outlive the objects it is > pointing to at the function's epilogue. > > How about a scoped_vector and allocating these objects on the heap instead? Done. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:526: content::PepperPluginInfo bundled_flash; On 2015/08/06 18:48:14, jln wrote: > How is this logic affecting other platforms than Linux? > > The commit message doesn't say anything, so please make that clear. Will do, answer: it shouldn't affect other platforms at all, unless there's a bug of course. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:535: std::sort(flash_versions.begin(), flash_versions.end(), On 2015/08/06 18:48:14, jln wrote: > I wonder if version comparison shouldn't be it's own separate function that is > unit tested. Done. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:542: if (flash_versions.size() > 0) On 2015/08/06 18:48:14, jln wrote: > It looks like you only need max_element, not a full sort, no? Done. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/componen... File chrome/common/component_flash_hint_file.h (right): https://codereview.chromium.org/1261333004/diff/200001/chrome/common/componen... chrome/common/component_flash_hint_file.h:19: class ComponentFlashHintFile { On 2015/08/06 18:48:14, jln wrote: > This class has only static members. You may want to consider using a namespace > instead: > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Nonmember,_St... > > But grouping in classes is common. Done. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/componen... chrome/common/component_flash_hint_file.h:22: // unpacked_plugin is the current location of the plugin. On 2015/08/06 18:48:14, jln wrote: > Use |argument| to document argument "argument". Done. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/componen... chrome/common/component_flash_hint_file.h:30: // the correct hash sum. On 2015/08/06 18:48:14, jln wrote: > Mention and document both arguments. Done. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/componen... chrome/common/component_flash_hint_file.h:36: // NOEXEC. On 2015/08/06 18:48:14, jln wrote: > nit: "noexec". Explain why this matters. > > See my other remark in the caller: why is it useful to treat this failure in a > special way? We don't seem to be providing any helpful error message. It's useful so that Chrome can fall back to the bundled flash plugin. Indeed, I added an error message where this is called. And I have explained the function in the comment. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/componen... File chrome/common/component_flash_hint_file_linux.cc (right): https://codereview.chromium.org/1261333004/diff/200001/chrome/common/componen... chrome/common/component_flash_hint_file_linux.cc:55: CHECK(len == crypto::kSHA256Length); On 2015/08/06 18:48:15, jln wrote: > CHECK_EQ() Done. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/componen... chrome/common/component_flash_hint_file_linux.cc:58: size_t bytes_remaining = mapped_file.length(), bytes_read = 0; On 2015/08/06 18:48:15, jln wrote: > Style: one line per variable (can't find where that is in the style guide but > definitely true. Done. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/componen... chrome/common/component_flash_hint_file_linux.cc:64: CHECK(bytes_remaining >= len); On 2015/08/06 18:48:14, jln wrote: > CHECK_GE() Acknowledged. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/componen... chrome/common/component_flash_hint_file_linux.cc:65: secure_hash->Update(mapped_file.data() + bytes_read, len); On 2015/08/06 18:48:14, jln wrote: > Why force this page by page? Why not ask update to do the full thing? This seems > overly complicated. Done. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/componen... chrome/common/component_flash_hint_file_linux.cc:76: const base::ScopedFD fd(HANDLE_EINTR(open(path.value().c_str(), O_RDONLY))); On 2015/08/06 18:48:15, jln wrote: > Do CLOEXEC while you're at it. Done. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/componen... chrome/common/component_flash_hint_file_linux.cc:79: const MmapDeleter deleter = {.map_size = sizeof(uint8_t)}; On 2015/08/06 18:48:14, jln wrote: > Style: I don't think we allow designed initializers like this. Done. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/componen... chrome/common/component_flash_hint_file_linux.cc:81: reinterpret_cast<uint8_t*>(mmap(NULL, deleter.map_size, On 2015/08/06 18:48:14, jln wrote: > Given the simplicity here, I would not use the MmapDeleter and just munmap as > needed. Makes sense, but I will leave it there in case someone adds to the function in the future. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/componen... chrome/common/component_flash_hint_file_linux.cc:208: if (memcmp(&file_hash[0], string_as_array(&decoded_hash), On 2015/08/06 18:48:15, jln wrote: > So there is nothing better than memcmp in crypto/ ? Turns out there is SecureEqualMem. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/componen... File chrome/common/component_flash_hint_file_unittests_linux.cc (right): https://codereview.chromium.org/1261333004/diff/200001/chrome/common/componen... chrome/common/component_flash_hint_file_unittests_linux.cc:69: srand48(time(NULL) ^ getpid()); On 2015/08/06 18:48:15, jln wrote: > Unit tests should reproduce 100% of the time. If you need randomness yous hould > still always use the same seed. > > I don't think you need randomness here anyways. Done. https://codereview.chromium.org/1261333004/diff/200001/chrome/common/componen... chrome/common/component_flash_hint_file_unittests_linux.cc:206: CHECK(base::WriteFile(file_path, reinterpret_cast<const char*>(file), On 2015/08/06 18:48:15, jln wrote: > If this fails we're not unmounting the path. Changed all those to EXPECT_TRUE.
https://codereview.chromium.org/1261333004/diff/240001/chrome/common/chrome_c... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1261333004/diff/240001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:7: #include <fcntl.h> might want to #ifdef this to Linux https://codereview.chromium.org/1261333004/diff/240001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:312: // By testing for this, Chrome can fallback to the bundled flash pluign. nit: plugin https://codereview.chromium.org/1261333004/diff/240001/chrome/common/chrome_c... File chrome/common/chrome_content_client.h (right): https://codereview.chromium.org/1261333004/diff/240001/chrome/common/chrome_c... chrome/common/chrome_content_client.h:56: std::vector<content::PepperPluginInfo*>& plugins); non const references are forbidden. https://codereview.chromium.org/1261333004/diff/240001/chrome/common/componen... File chrome/common/component_flash_hint_file_linux.cc (right): https://codereview.chromium.org/1261333004/diff/240001/chrome/common/componen... chrome/common/component_flash_hint_file_linux.cc:62: CHECK_EQ(len, crypto::kSHA256Length); FYI, CHECK_EQ(value that we know is correct, thing to test) is a bit more idiomatic in Chromium and in Google C++. https://codereview.chromium.org/1261333004/diff/240001/chrome/common/componen... File chrome/common/component_flash_hint_file_unittests_linux.cc (right): https://codereview.chromium.org/1261333004/diff/240001/chrome/common/componen... chrome/common/component_flash_hint_file_unittests_linux.cc:201: EXPECT_EQ(base::WriteFile(file_path, reinterpret_cast<const char*>(file), Are you sure EXPECT_* macros work in MULTIPROCESS_TEST_MAIN? You might have to crash. In which case you shoudl set bools depending on what happened, umount and then CHECK the bools. Please verify how MULTIPROCES_TEST_MAIN works, as it is very undocumented.
https://codereview.chromium.org/1261333004/diff/240001/chrome/common/chrome_c... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1261333004/diff/240001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:7: #include <fcntl.h> On 2015/08/07 22:27:19, jln wrote: > might want to #ifdef this to Linux Done. https://codereview.chromium.org/1261333004/diff/240001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:312: // By testing for this, Chrome can fallback to the bundled flash pluign. On 2015/08/07 22:27:19, jln wrote: > nit: plugin Done. https://codereview.chromium.org/1261333004/diff/240001/chrome/common/chrome_c... File chrome/common/chrome_content_client.h (right): https://codereview.chromium.org/1261333004/diff/240001/chrome/common/chrome_c... chrome/common/chrome_content_client.h:56: std::vector<content::PepperPluginInfo*>& plugins); On 2015/08/07 22:27:19, jln wrote: > non const references are forbidden. Done. https://codereview.chromium.org/1261333004/diff/240001/chrome/common/componen... File chrome/common/component_flash_hint_file_linux.cc (right): https://codereview.chromium.org/1261333004/diff/240001/chrome/common/componen... chrome/common/component_flash_hint_file_linux.cc:62: CHECK_EQ(len, crypto::kSHA256Length); On 2015/08/07 22:27:19, jln wrote: > FYI, CHECK_EQ(value that we know is correct, thing to test) is a bit more > idiomatic in Chromium and in Google C++. Done. https://codereview.chromium.org/1261333004/diff/240001/chrome/common/componen... File chrome/common/component_flash_hint_file_unittests_linux.cc (right): https://codereview.chromium.org/1261333004/diff/240001/chrome/common/componen... chrome/common/component_flash_hint_file_unittests_linux.cc:201: EXPECT_EQ(base::WriteFile(file_path, reinterpret_cast<const char*>(file), On 2015/08/07 22:27:20, jln wrote: > Are you sure EXPECT_* macros work in MULTIPROCESS_TEST_MAIN? > > You might have to crash. > > In which case you shoudl set bools depending on what happened, umount and then > CHECK the bools. > > Please verify how MULTIPROCES_TEST_MAIN works, as it is very undocumented. Indeed, it actually needs to crash, so I have restructured this.
lgtm, on to owner reviews! Nit: it's better if your commit message respects 80 columns.
kerrnel@chromium.org changed reviewers: + jochen@chromium.org
jochen@chromium.org: Please review changes.
jochen@chromium.org changed reviewers: + bauerb@chromium.org
deferring to bauerb@ rubberstamp lgtm once he's happy
https://codereview.chromium.org/1261333004/diff/260001/chrome/common/chrome_c... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1261333004/diff/260001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:510: // static Move the comment inside of the #ifdef? https://codereview.chromium.org/1261333004/diff/260001/chrome/common/chrome_c... File chrome/common/chrome_content_client.h (right): https://codereview.chromium.org/1261333004/diff/260001/chrome/common/chrome_c... chrome/common/chrome_content_client.h:54: // plugin type. This function may return a nullptr. Under which circumstances? https://codereview.chromium.org/1261333004/diff/260001/chrome/common/chrome_c... chrome/common/chrome_content_client.h:55: static content::PepperPluginInfo* FindMostRecentPlugin( Is this method called outside of ChromeContentClient and tests? If not, can you add a comment that this is only visible for testing? https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... File chrome/common/component_flash_hint_file.h (right): https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... chrome/common/component_flash_hint_file.h:9: #include <vector> Unnecessary? https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... chrome/common/component_flash_hint_file.h:11: #include "base/files/file_util.h" Unnecessary? (You probably have to forward-declare base::FilePath when you remove these includes.) https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... chrome/common/component_flash_hint_file.h:12: #include "crypto/secure_hash.h" Unnecessary? https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... chrome/common/component_flash_hint_file.h:19: namespace ComponentFlashHintFile { Namespaces are separated_by_underscores. (Or make it a static class again) https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... File chrome/common/component_flash_hint_file_linux.cc (right): https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... chrome/common/component_flash_hint_file_linux.cc:5: #include "chrome/common/component_flash_hint_file.h" So, if someone on non-Linux calls a method from this header file, they will only get an error at link time, right? I think it would be a bit more helpful to make sure that these methods are only declared if there actually is an implementation available -- either compile this file on all platforms (and possibly #ifdef out the parts that are actually Linux-specific, but both in the header and the implementation file), or make the header Linux-only as well. https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... chrome/common/component_flash_hint_file_linux.cc:113: const MmapDeleter deleter = {sizeof(uint8_t)}; Is this ({sizeof(uint8_t)}) the |map_size|? Any reason you're not using a regular constructor? https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... chrome/common/component_flash_hint_file_linux.cc:115: reinterpret_cast<uint8_t*>(mmap(NULL, deleter.map_size, Use nullptr rather than NULL. https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... chrome/common/component_flash_hint_file_linux.cc:160: if (value == nullptr) { I would just do !value. https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... chrome/common/component_flash_hint_file_linux.cc:206: if (!crypto::SecureMemEqual(&file_hash[0], string_as_array(&decoded_hash), Is it actually important not to leak side-channel information here? We're not really concerned about an attacker that already can modify files on disk, right? https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... File chrome/common/component_flash_hint_file_unittests_linux.cc (right): https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... chrome/common/component_flash_hint_file_unittests_linux.cc:47: ASSERT_TRUE(base::WriteFile(flash_dir, reinterpret_cast<const char*>(file), ASSERT_EQ https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... chrome/common/component_flash_hint_file_unittests_linux.cc:58: ASSERT_TRUE(returned_flash_path == flash_dir); ASSERT_EQ https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... chrome/common/component_flash_hint_file_unittests_linux.cc:70: ASSERT_FALSE(empty_path == flash_dir); ASSERT_NE https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... chrome/common/component_flash_hint_file_unittests_linux.cc:107: // flash_dir_update needs to be moved to flash_dir, but if that fails (for the Nit: Put pipe symbols around variables if you use them in prose text (|flash_dir_update|). https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... chrome/common/component_flash_hint_file_unittests_linux.cc:129: LOG(ERROR) << "This kernel does not support unpriveleged namespaces. " Nit: "unprivileged" 😃 https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... chrome/common/component_flash_hint_file_unittests_linux.cc:137: CHECK(mount("tmpfs", temp_dir.path().value().c_str(), "tmpfs", tmpfs_flags, CHECK_EQ(0, ...) https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... chrome/common/component_flash_hint_file_unittests_linux.cc:145: sizeof(file)) == (int)sizeof(file); Use static_cast<int>(). https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... chrome/common/component_flash_hint_file_unittests_linux.cc:161: EXPECT_TRUE(process.WaitForExitWithTimeout(TestTimeouts::action_max_timeout(), I would probably make this an ASSERT -- if this times out, the exit code is not going to tell you anything. https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... chrome/common/component_flash_hint_file_unittests_linux.cc:163: EXPECT_EQ(exit_code, 0); Put the expected value first.
https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... File chrome/common/chrome_content_client.cc (right): https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... chrome/common/chrome_content_client.cc:510: // static On 2015/08/13 12:40:26, Bernhard Bauer wrote: > Move the comment inside of the #ifdef? Done. https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... File chrome/common/chrome_content_client.h (right): https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... chrome/common/chrome_content_client.h:54: // plugin type. This function may return a nullptr. On 2015/08/13 12:40:26, Bernhard Bauer wrote: > Under which circumstances? Done. https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... chrome/common/chrome_content_client.h:55: static content::PepperPluginInfo* FindMostRecentPlugin( On 2015/08/13 12:40:26, Bernhard Bauer wrote: > Is this method called outside of ChromeContentClient and tests? If not, can you > add a comment that this is only visible for testing? Done. https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... File chrome/common/component_flash_hint_file.h (right): https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... chrome/common/component_flash_hint_file.h:9: #include <vector> On 2015/08/13 12:40:26, Bernhard Bauer wrote: > Unnecessary? Done. https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... chrome/common/component_flash_hint_file.h:11: #include "base/files/file_util.h" On 2015/08/13 12:40:26, Bernhard Bauer wrote: > Unnecessary? > > (You probably have to forward-declare base::FilePath when you remove these > includes.) Done. https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... chrome/common/component_flash_hint_file.h:12: #include "crypto/secure_hash.h" On 2015/08/13 12:40:26, Bernhard Bauer wrote: > Unnecessary? Done. https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... chrome/common/component_flash_hint_file.h:19: namespace ComponentFlashHintFile { On 2015/08/13 12:40:26, Bernhard Bauer wrote: > Namespaces are separated_by_underscores. (Or make it a static class again) Done. https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... File chrome/common/component_flash_hint_file_linux.cc (right): https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... chrome/common/component_flash_hint_file_linux.cc:5: #include "chrome/common/component_flash_hint_file.h" On 2015/08/13 12:40:26, Bernhard Bauer wrote: > So, if someone on non-Linux calls a method from this header file, they will only > get an error at link time, right? I think it would be a bit more helpful to make > sure that these methods are only declared if there actually is an implementation > available -- either compile this file on all platforms (and possibly #ifdef out > the parts that are actually Linux-specific, but both in the header and the > implementation file), or make the header Linux-only as well. I'm going to make the header Linux-only, because I want it to be clear the class isn't meant to be called on anything but Linux. https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... chrome/common/component_flash_hint_file_linux.cc:113: const MmapDeleter deleter = {sizeof(uint8_t)}; On 2015/08/13 12:40:26, Bernhard Bauer wrote: > Is this ({sizeof(uint8_t)}) the |map_size|? Any reason you're not using a > regular constructor? Done. https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... chrome/common/component_flash_hint_file_linux.cc:115: reinterpret_cast<uint8_t*>(mmap(NULL, deleter.map_size, On 2015/08/13 12:40:26, Bernhard Bauer wrote: > Use nullptr rather than NULL. Done. https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... chrome/common/component_flash_hint_file_linux.cc:160: if (value == nullptr) { On 2015/08/13 12:40:26, Bernhard Bauer wrote: > I would just do !value. Done. https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... chrome/common/component_flash_hint_file_linux.cc:206: if (!crypto::SecureMemEqual(&file_hash[0], string_as_array(&decoded_hash), On 2015/08/13 12:40:26, Bernhard Bauer wrote: > Is it actually important not to leak side-channel information here? We're not > really concerned about an attacker that already can modify files on disk, right? It's not a huge concern, but to be thorough and cautious, I wanted to use the secure mem equal function. Who knows how these classes can get extended down the road. https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... File chrome/common/component_flash_hint_file_unittests_linux.cc (right): https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... chrome/common/component_flash_hint_file_unittests_linux.cc:47: ASSERT_TRUE(base::WriteFile(flash_dir, reinterpret_cast<const char*>(file), On 2015/08/13 12:40:26, Bernhard Bauer wrote: > ASSERT_EQ Done. https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... chrome/common/component_flash_hint_file_unittests_linux.cc:58: ASSERT_TRUE(returned_flash_path == flash_dir); On 2015/08/13 12:40:27, Bernhard Bauer wrote: > ASSERT_EQ Done. https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... chrome/common/component_flash_hint_file_unittests_linux.cc:70: ASSERT_FALSE(empty_path == flash_dir); On 2015/08/13 12:40:26, Bernhard Bauer wrote: > ASSERT_NE Done. https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... chrome/common/component_flash_hint_file_unittests_linux.cc:107: // flash_dir_update needs to be moved to flash_dir, but if that fails (for the On 2015/08/13 12:40:26, Bernhard Bauer wrote: > Nit: Put pipe symbols around variables if you use them in prose text > (|flash_dir_update|). This comment actually turns out to be irrelevant now, so I updated it. https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... chrome/common/component_flash_hint_file_unittests_linux.cc:129: LOG(ERROR) << "This kernel does not support unpriveleged namespaces. " On 2015/08/13 12:40:26, Bernhard Bauer wrote: > Nit: "unprivileged" 😃 Done. https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... chrome/common/component_flash_hint_file_unittests_linux.cc:137: CHECK(mount("tmpfs", temp_dir.path().value().c_str(), "tmpfs", tmpfs_flags, On 2015/08/13 12:40:26, Bernhard Bauer wrote: > CHECK_EQ(0, ...) Done. https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... chrome/common/component_flash_hint_file_unittests_linux.cc:145: sizeof(file)) == (int)sizeof(file); On 2015/08/13 12:40:26, Bernhard Bauer wrote: > Use static_cast<int>(). Done. https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... chrome/common/component_flash_hint_file_unittests_linux.cc:161: EXPECT_TRUE(process.WaitForExitWithTimeout(TestTimeouts::action_max_timeout(), On 2015/08/13 12:40:26, Bernhard Bauer wrote: > I would probably make this an ASSERT -- if this times out, the exit code is not > going to tell you anything. Done. https://chromiumcodereview.appspot.com/1261333004/diff/260001/chrome/common/c... chrome/common/component_flash_hint_file_unittests_linux.cc:163: EXPECT_EQ(exit_code, 0); On 2015/08/13 12:40:26, Bernhard Bauer wrote: > Put the expected value first. Done.
LGTM with an optional comment and a nit: https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... File chrome/common/component_flash_hint_file_linux.cc (right): https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... chrome/common/component_flash_hint_file_linux.cc:5: #include "chrome/common/component_flash_hint_file.h" On 2015/08/13 22:00:45, Greg Kerr wrote: > On 2015/08/13 12:40:26, Bernhard Bauer wrote: > > So, if someone on non-Linux calls a method from this header file, they will > only > > get an error at link time, right? I think it would be a bit more helpful to > make > > sure that these methods are only declared if there actually is an > implementation > > available -- either compile this file on all platforms (and possibly #ifdef > out > > the parts that are actually Linux-specific, but both in the header and the > > implementation file), or make the header Linux-only as well. > > I'm going to make the header Linux-only, because I want it to be clear the class > isn't meant to be called on anything but Linux. In that case, consider renaming the header to ..._linux.h as well. https://codereview.chromium.org/1261333004/diff/260001/chrome/common/componen... chrome/common/component_flash_hint_file_linux.cc:206: if (!crypto::SecureMemEqual(&file_hash[0], string_as_array(&decoded_hash), On 2015/08/13 22:00:44, Greg Kerr wrote: > On 2015/08/13 12:40:26, Bernhard Bauer wrote: > > Is it actually important not to leak side-channel information here? We're not > > really concerned about an attacker that already can modify files on disk, > right? > > It's not a huge concern, but to be thorough and cautious, I wanted to use the > secure mem equal function. Who knows how these classes can get extended down the > road. Ok... although I would hope that if the use case of this changes to something security-relevant, it would get a security review which would flag unsafe hash comparisons. https://codereview.chromium.org/1261333004/diff/280001/chrome/common/componen... File chrome/common/component_flash_hint_file.h (right): https://codereview.chromium.org/1261333004/diff/280001/chrome/common/componen... chrome/common/component_flash_hint_file.h:4: // Remove this?
The CQ bit was checked by kerrnel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, jln@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/1261333004/#ps300001 (title: "Fix up bauerb's final few comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1261333004/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1261333004/300001
Message was sent while issue was closed.
Committed patchset #7 (id:300001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/2bb08b2a5f6bcc382ebaccf1b50bd655592dfe51 Cr-Commit-Position: refs/heads/master@{#343435}
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
Just some drive by nits to fix later. https://codereview.chromium.org/1261333004/diff/300001/chrome/browser/compone... File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/1261333004/diff/300001/chrome/browser/compone... chrome/browser/component_updater/pepper_flash_component_installer.cc:246: if (!base::DeleteFile(path, true)) If you are just trying to delete a file, why pass recursive=true? https://codereview.chromium.org/1261333004/diff/300001/chrome/common/chrome_c... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1261333004/diff/300001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:28: #include "chrome/common/component_flash_hint_file_linux.h" Rather than including this header here and putting #if defined(OS_LINUX) in the header, it's probably better to: - Put the #include in a #if defined(OS_LINUX) section here - Add #if !defined(OS_LINUX) #error "No penguins here" #endif in component_flash_hint_file_linux.h https://codereview.chromium.org/1261333004/diff/300001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:295: return base::PathExists(user_data_dir); This isn't necessary. See the DIR_USER_DATA handling code in chrome/common/chrome_paths.cc. https://codereview.chromium.org/1261333004/diff/300001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:324: } else { no need for else if the ablove block returns. https://codereview.chromium.org/1261333004/diff/300001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:525: return result; You can use a ternary operator and not even bother with |result|. https://codereview.chromium.org/1261333004/diff/300001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:567: if (max_flash != nullptr) just if (max_flash) https://codereview.chromium.org/1261333004/diff/300001/chrome/common/chrome_p... File chrome/common/chrome_paths.cc (right): https://codereview.chromium.org/1261333004/diff/300001/chrome/common/chrome_p... chrome/common/chrome_paths.cc:577: &cur)) add braces https://codereview.chromium.org/1261333004/diff/300001/chrome/common/componen... File chrome/common/component_flash_hint_file_linux.cc (right): https://codereview.chromium.org/1261333004/diff/300001/chrome/common/componen... chrome/common/component_flash_hint_file_linux.cc:30: namespace { empty line after https://codereview.chromium.org/1261333004/diff/300001/chrome/common/componen... File chrome/common/component_flash_hint_file_linux.h (right): https://codereview.chromium.org/1261333004/diff/300001/chrome/common/componen... chrome/common/component_flash_hint_file_linux.h:18: namespace chrome { We actually decided to stop using the chrome namespace a few years ago. Please do not add more.
Message was sent while issue was closed.
https://codereview.chromium.org/1261333004/diff/300001/chrome/browser/compone... File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/1261333004/diff/300001/chrome/browser/compone... chrome/browser/component_updater/pepper_flash_component_installer.cc:232: GetPepperFlashBaseDirectory().AppendASCII(version.GetString()); Since |path| is being deleted recursively, we should check the PathService::Get() result in GetPepperFlashBaseDirectory(). Maybe just call PathService::Get() right here drop the help function? https://codereview.chromium.org/1261333004/diff/300001/chrome/browser/compone... chrome/browser/component_updater/pepper_flash_component_installer.cc:246: if (!base::DeleteFile(path, true)) On 2015/08/14 22:37:07, Lei Zhang wrote: > If you are just trying to delete a file, why pass recursive=true? Oh, this is deleting |path| but I read it as |flash_path| in my mind. So indeed we need to do a recursive delete, but we should be careful. See other new comment above.
Message was sent while issue was closed.
https://codereview.chromium.org/1261333004/diff/280001/chrome/common/componen... File chrome/common/component_flash_hint_file.h (right): https://codereview.chromium.org/1261333004/diff/280001/chrome/common/componen... chrome/common/component_flash_hint_file.h:4: // On 2015/08/14 07:55:12, Bernhard Bauer wrote: > Remove this? Done. https://codereview.chromium.org/1261333004/diff/300001/chrome/browser/compone... File chrome/browser/component_updater/pepper_flash_component_installer.cc (right): https://codereview.chromium.org/1261333004/diff/300001/chrome/browser/compone... chrome/browser/component_updater/pepper_flash_component_installer.cc:246: if (!base::DeleteFile(path, true)) On 2015/08/14 22:37:07, Lei Zhang wrote: > If you are just trying to delete a file, why pass recursive=true? That's actually a directory and not just a file. The layout PepperFlash/1.0.0.0/libpepflashplayer.so. In this case, we want to delete the 1.0.0.0 folder. https://codereview.chromium.org/1261333004/diff/300001/chrome/common/chrome_c... File chrome/common/chrome_content_client.cc (right): https://codereview.chromium.org/1261333004/diff/300001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:28: #include "chrome/common/component_flash_hint_file_linux.h" On 2015/08/14 22:37:07, Lei Zhang wrote: > Rather than including this header here and putting #if defined(OS_LINUX) in the > header, it's probably better to: > > - Put the #include in a #if defined(OS_LINUX) section here > - Add > > #if !defined(OS_LINUX) > #error "No penguins here" > #endif > > in component_flash_hint_file_linux.h Done. https://codereview.chromium.org/1261333004/diff/300001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:295: return base::PathExists(user_data_dir); On 2015/08/14 22:37:07, Lei Zhang wrote: > This isn't necessary. See the DIR_USER_DATA handling code in > chrome/common/chrome_paths.cc. Done. https://codereview.chromium.org/1261333004/diff/300001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:324: } else { On 2015/08/14 22:37:07, Lei Zhang wrote: > no need for else if the ablove block returns. Done. https://codereview.chromium.org/1261333004/diff/300001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:525: return result; On 2015/08/14 22:37:07, Lei Zhang wrote: > You can use a ternary operator and not even bother with |result|. Done. https://codereview.chromium.org/1261333004/diff/300001/chrome/common/chrome_c... chrome/common/chrome_content_client.cc:567: if (max_flash != nullptr) On 2015/08/14 22:37:07, Lei Zhang wrote: > just if (max_flash) Done. https://codereview.chromium.org/1261333004/diff/300001/chrome/common/chrome_p... File chrome/common/chrome_paths.cc (right): https://codereview.chromium.org/1261333004/diff/300001/chrome/common/chrome_p... chrome/common/chrome_paths.cc:577: &cur)) On 2015/08/14 22:37:07, Lei Zhang wrote: > add braces Done. https://codereview.chromium.org/1261333004/diff/300001/chrome/common/componen... File chrome/common/component_flash_hint_file_linux.cc (right): https://codereview.chromium.org/1261333004/diff/300001/chrome/common/componen... chrome/common/component_flash_hint_file_linux.cc:30: namespace { On 2015/08/14 22:37:07, Lei Zhang wrote: > empty line after Done. https://codereview.chromium.org/1261333004/diff/300001/chrome/common/componen... File chrome/common/component_flash_hint_file_linux.h (right): https://codereview.chromium.org/1261333004/diff/300001/chrome/common/componen... chrome/common/component_flash_hint_file_linux.h:18: namespace chrome { On 2015/08/14 22:37:07, Lei Zhang wrote: > We actually decided to stop using the chrome namespace a few years ago. Please > do not add more. Done.
Message was sent while issue was closed.
For the record, the follow-up CL is https://codereview.chromium.org/1298013002/ |
