Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2534873005:
Sandbox the component updater's patcher utility process. (Closed)
DescriptionSandbox the component updater's patcher utility process.
The code will now open files in the browser process before passing the
handles across IPC to the utility process. The utility process in turn
invokes courgette/bsdiff, which memory maps the files and operates on
them as before.
There is a behavioral difference when using the courgette or
courgette_mini tools: the output file will now be created/overwritten
at the start of the operation, and in the case of a failure, will be
deleted. Previously, the output file was created late in the operation
operation and several failure modes would leave it unmodified.
BUG=660325
Committed: https://crrev.com/53796c72e2ec50eec77f0bd79ec0c8426cee4d58
Cr-Commit-Position: refs/heads/master@{#437669}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Through #14 #
Total comments: 2
Patch Set 3 : Through #18 #
Total comments: 2
Patch Set 4 : Through #29 #
Total comments: 2
Messages
Total messages: 49 (25 generated)
The CQ bit was checked by waffles@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
waffles@chromium.org changed reviewers: + dcheng@chromium.org, huangs@chromium.org, jam@chromium.org, kerrnel@chromium.org, sorin@chromium.org
Hi all, please take a look at this change to sandbox the component updater's patcher utility process.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Thank you! component_updater lgtm
ipc lgtm
I know a few of you were OOO last week; pinging this in case you missed it. (Sorry for the noise.)
Acknowledge. Sorry for not seeing this earlier!
https://codereview.chromium.org/2534873005/diff/1/courgette/third_party/bsdif... File courgette/third_party/bsdiff/bsdiff.h (right): https://codereview.chromium.org/2534873005/diff/1/courgette/third_party/bsdif... courgette/third_party/bsdiff/bsdiff.h:88: // As above, but simply takes base::Files. Please move this between the Stream version and FilePath version. Same in the .cc file.
On 2016/12/06 18:10:40, huangs wrote: > https://codereview.chromium.org/2534873005/diff/1/courgette/third_party/bsdif... > File courgette/third_party/bsdiff/bsdiff.h (right): > > https://codereview.chromium.org/2534873005/diff/1/courgette/third_party/bsdif... > courgette/third_party/bsdiff/bsdiff.h:88: // As above, but simply takes > base::Files. > Please move this between the Stream version and FilePath version. Same in the > .cc file. Enabling the sandbox and sending files over IPC instead? LGTM. Thank you.
More comments. https://codereview.chromium.org/2534873005/diff/1/courgette/courgette.h File courgette/courgette.h (right): https://codereview.chromium.org/2534873005/diff/1/courgette/courgette.h#newco... courgette/courgette.h:13: class File; Same comment as in bsdiff.h: Please - Remove the forward declaration. - #include "base/files/file.h" https://codereview.chromium.org/2534873005/diff/1/courgette/third_party/bsdif... File courgette/third_party/bsdiff/bsdiff.h (right): https://codereview.chromium.org/2534873005/diff/1/courgette/third_party/bsdif... courgette/third_party/bsdiff/bsdiff.h:49: namespace base { You're not using File pointers, and looks like this works because file_util.h includes file.h? Please: - Remove the forward declaration here. - #include "base/files/file.h" - Remove the #include from bsdiff_apply.cc. https://codereview.chromium.org/2534873005/diff/1/courgette/third_party/bsdif... File courgette/third_party/bsdiff/bsdiff_apply.cc (right): https://codereview.chromium.org/2534873005/diff/1/courgette/third_party/bsdif... courgette/third_party/bsdiff/bsdiff_apply.cc:237: // Write the stream to disk. There seems to be a change in behavior: Previously if failure occurs before this point then |new_file_path| does not get written. But with the change, |new_file_path| always gets written, if failure happens then we'll have a size-0 file lying around.
https://codereview.chromium.org/2534873005/diff/1/courgette/courgette.h File courgette/courgette.h (right): https://codereview.chromium.org/2534873005/diff/1/courgette/courgette.h#newco... courgette/courgette.h:90: // Applies the patch in |patch_file| to the bytes in |old_file| and writes the Please move this between the SourceStream* version and the FielPath::CharType* version, same in .cc file.
The CQ bit was checked by waffles@chromium.org to run a CQ dry run
https://codereview.chromium.org/2534873005/diff/1/courgette/courgette.h File courgette/courgette.h (right): https://codereview.chromium.org/2534873005/diff/1/courgette/courgette.h#newco... courgette/courgette.h:13: class File; On 2016/12/06 18:23:22, huangs wrote: > Same comment as in bsdiff.h: Please > - Remove the forward declaration. > - #include "base/files/file.h" Done, thanks; this was leftover from an intermediate form of the code. https://codereview.chromium.org/2534873005/diff/1/courgette/courgette.h#newco... courgette/courgette.h:90: // Applies the patch in |patch_file| to the bytes in |old_file| and writes the On 2016/12/06 18:30:32, huangs wrote: > Please move this between the SourceStream* version and the FielPath::CharType* > version, same in .cc file. Done. Curious: What's the rationale here? https://codereview.chromium.org/2534873005/diff/1/courgette/third_party/bsdif... File courgette/third_party/bsdiff/bsdiff.h (right): https://codereview.chromium.org/2534873005/diff/1/courgette/third_party/bsdif... courgette/third_party/bsdiff/bsdiff.h:49: namespace base { On 2016/12/06 18:23:22, huangs wrote: > You're not using File pointers, and looks like this works because file_util.h > includes file.h? Please: > - Remove the forward declaration here. > - #include "base/files/file.h" > - Remove the #include from bsdiff_apply.cc. Done. https://codereview.chromium.org/2534873005/diff/1/courgette/third_party/bsdif... courgette/third_party/bsdiff/bsdiff.h:88: // As above, but simply takes base::Files. On 2016/12/06 18:10:40, huangs wrote: > Please move this between the Stream version and FilePath version. Same in the > .cc file. Done. https://codereview.chromium.org/2534873005/diff/1/courgette/third_party/bsdif... File courgette/third_party/bsdiff/bsdiff_apply.cc (right): https://codereview.chromium.org/2534873005/diff/1/courgette/third_party/bsdif... courgette/third_party/bsdiff/bsdiff_apply.cc:237: // Write the stream to disk. On 2016/12/06 18:23:22, huangs wrote: > There seems to be a change in behavior: Previously if failure occurs before this > point then |new_file_path| does not get written. But with the change, > |new_file_path| always gets written, if failure happens then we'll have a size-0 > file lying around. Yes, that is correct. This can't be avoided in the case where we create the file in the browser process and pass the handle to the utility process. It could be avoided for the in-process patching case (i.e. on ios or from the courgette executable), at the cost of some code duplication. Alternatively, the path-based ApplyBinaryPatch could be removed and its only call site updated. There are a few more call sites for ApplyEnsemblePatch. Let me know if this is a concern, and if so how you want to proceed.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2534873005/diff/1/courgette/courgette.h File courgette/courgette.h (right): https://codereview.chromium.org/2534873005/diff/1/courgette/courgette.h#newco... courgette/courgette.h:90: // Applies the patch in |patch_file| to the bytes in |old_file| and writes the Ah, symmetry: We have A() calling B() alling C(), I'd prefer call direction to be upwards. https://codereview.chromium.org/2534873005/diff/1/courgette/third_party/bsdif... File courgette/third_party/bsdiff/bsdiff_apply.cc (right): https://codereview.chromium.org/2534873005/diff/1/courgette/third_party/bsdif... courgette/third_party/bsdiff/bsdiff_apply.cc:237: // Write the stream to disk. This is not an issue for Component Updator, but it's a change for stand-alone Courgette and usage by Installer. - For stand-alone Courgette: This departs from what I'm used to for Courgette-apply -- but I can live with that. There's a slight risk of having validation script that checks "existence of output file after running Courgette". I don't believe anyone's doing that. - For installer: On patch failure there will be a size-0 file lying around. I believe entire directories get wiped out on failure anyway, so we probably won't accumulate permanent artifacts on user's in this case. The "beliefs" above are not verified by fact. To not waste time, I propose: - Mention the behavioral change in CL comment, commit as usual. - Ping Chrome build folks to see if, after they generate patch, whether do they verify by applying the patch. If things fail, there will be size-0 file lying around and hopefully that's not an issue. - Ping grt@ re. installer, and note that now size-0 file will be created if patching fails, and ask whether things will be cleaned up. https://codereview.chromium.org/2534873005/diff/20001/courgette/third_party/b... File courgette/third_party/bsdiff/bsdiff.h (right): https://codereview.chromium.org/2534873005/diff/20001/courgette/third_party/b... courgette/third_party/bsdiff/bsdiff.h:86: BSDiffStatus ApplyBinaryPatch(base::File old_stream, Order not changed?
Description was changed from ========== Sandbox the component updater's patcher utility process. The code will now open files in the browser process before passing the handles across IPC to the utility process. The utility process in turn invokes courgette/bsdiff, which memory maps the files and operates on them as before. BUG=660325 ========== to ========== Sandbox the component updater's patcher utility process. The code will now open files in the browser process before passing the handles across IPC to the utility process. The utility process in turn invokes courgette/bsdiff, which memory maps the files and operates on them as before. There is a behavioral difference when using the courgette or courgette_mini tools: the patch file will now be created at the start of the operation, and in the case of a failure, will have no contents written to it. Previously, the patch file was created late in the operation and several failure modes would skip the creation of such a file. BUG=660325 ==========
https://codereview.chromium.org/2534873005/diff/1/courgette/third_party/bsdif... File courgette/third_party/bsdiff/bsdiff_apply.cc (right): https://codereview.chromium.org/2534873005/diff/1/courgette/third_party/bsdif... courgette/third_party/bsdiff/bsdiff_apply.cc:237: // Write the stream to disk. On 2016/12/06 19:40:07, huangs wrote: > This is not an issue for Component Updator, but it's a change for stand-alone > Courgette and usage by Installer. > > - For stand-alone Courgette: This departs from what I'm used to for > Courgette-apply -- but I can live with that. There's a slight risk of having > validation script that checks "existence of output file after running > Courgette". I don't believe anyone's doing that. > > - For installer: On patch failure there will be a size-0 file lying around. I > believe entire directories get wiped out on failure anyway, so we probably won't > accumulate permanent artifacts on user's in this case. > > The "beliefs" above are not verified by fact. To not waste time, I propose: > - Mention the behavioral change in CL comment, commit as usual. > - Ping Chrome build folks to see if, after they generate patch, whether do they > verify by applying the patch. If things fail, there will be size-0 file lying > around and hopefully that's not an issue. > - Ping grt@ re. installer, and note that now size-0 file will be created if > patching fails, and ask whether things will be cleaned up. Sounds good, will do. https://codereview.chromium.org/2534873005/diff/20001/courgette/third_party/b... File courgette/third_party/bsdiff/bsdiff.h (right): https://codereview.chromium.org/2534873005/diff/20001/courgette/third_party/b... courgette/third_party/bsdiff/bsdiff.h:86: BSDiffStatus ApplyBinaryPatch(base::File old_stream, On 2016/12/06 19:40:07, huangs wrote: > Order not changed? Sorry, I really thought I did that... Done now.
The CQ bit was checked by waffles@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Sandbox the component updater's patcher utility process. The code will now open files in the browser process before passing the handles across IPC to the utility process. The utility process in turn invokes courgette/bsdiff, which memory maps the files and operates on them as before. There is a behavioral difference when using the courgette or courgette_mini tools: the patch file will now be created at the start of the operation, and in the case of a failure, will have no contents written to it. Previously, the patch file was created late in the operation and several failure modes would skip the creation of such a file. BUG=660325 ========== to ========== Sandbox the component updater's patcher utility process. The code will now open files in the browser process before passing the handles across IPC to the utility process. The utility process in turn invokes courgette/bsdiff, which memory maps the files and operates on them as before. There is a behavioral difference when using the courgette or courgette_mini tools: the patch file will now be created at the start of the operation, and in the case of a failure, will have no contents written to it. Previously, the patch file was created late in the operation and several failure modes would skip the creation of such a file. BUG=660325 ==========
Thanks! LGTM re. Courgette code.
Description was changed from ========== Sandbox the component updater's patcher utility process. The code will now open files in the browser process before passing the handles across IPC to the utility process. The utility process in turn invokes courgette/bsdiff, which memory maps the files and operates on them as before. There is a behavioral difference when using the courgette or courgette_mini tools: the patch file will now be created at the start of the operation, and in the case of a failure, will have no contents written to it. Previously, the patch file was created late in the operation and several failure modes would skip the creation of such a file. BUG=660325 ========== to ========== Sandbox the component updater's patcher utility process. The code will now open files in the browser process before passing the handles across IPC to the utility process. The utility process in turn invokes courgette/bsdiff, which memory maps the files and operates on them as before. There is a behavioral difference when using the courgette or courgette_mini tools: the output file will now be created at the start of the operation, and in the case of a failure, will have no contents written to it. Previously, the patched file was created late in the operation and several failure modes would skip the creation of such a file. BUG=660325 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
which files do you want me to review? if chrome/utility, lgtm
https://codereview.chromium.org/2534873005/diff/40001/courgette/ensemble_appl... File courgette/ensemble_apply.cc (right): https://codereview.chromium.org/2534873005/diff/40001/courgette/ensemble_appl... courgette/ensemble_apply.cc:433: } is there a reason not to delete the new file in case of error?
jam: Yes, just utility, thank you very much! https://codereview.chromium.org/2534873005/diff/40001/courgette/ensemble_appl... File courgette/ensemble_apply.cc (right): https://codereview.chromium.org/2534873005/diff/40001/courgette/ensemble_appl... courgette/ensemble_apply.cc:433: } On 2016/12/07 10:14:42, grt (UTC plus 1) wrote: > is there a reason not to delete the new file in case of error? You're so smart, Greg. (Done.)
The CQ bit was checked by waffles@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please also update CL description to reflect this. Now Courgette-apply would always overwrite file, and delete on failure.
Description was changed from ========== Sandbox the component updater's patcher utility process. The code will now open files in the browser process before passing the handles across IPC to the utility process. The utility process in turn invokes courgette/bsdiff, which memory maps the files and operates on them as before. There is a behavioral difference when using the courgette or courgette_mini tools: the output file will now be created at the start of the operation, and in the case of a failure, will have no contents written to it. Previously, the patched file was created late in the operation and several failure modes would skip the creation of such a file. BUG=660325 ========== to ========== Sandbox the component updater's patcher utility process. The code will now open files in the browser process before passing the handles across IPC to the utility process. The utility process in turn invokes courgette/bsdiff, which memory maps the files and operates on them as before. There is a behavioral difference when using the courgette or courgette_mini tools: the output file will now be created/overwritten at the start of the operation, and in the case of a failure, will be deleted. Previously, the output file was created late in the operation operation and several failure modes would leave it unmodified. BUG=660325 ==========
On 2016/12/09 19:12:57, huangs wrote: > Please also update CL description to reflect this. Now Courgette-apply would > always overwrite file, and delete on failure. Thanks for the reminder, done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by waffles@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sorin@chromium.org, kerrnel@chromium.org, dcheng@chromium.org, jam@chromium.org, huangs@chromium.org Link to the patchset: https://codereview.chromium.org/2534873005/#ps60001 (title: "Through #29")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481319486334010,
"parent_rev": "191c4b6cab0a1016f3a7a3ee55e8c94e8b550834", "commit_rev":
"3a2258bd2eadd49804576be9f937bb01cd8cb663"}
Message was sent while issue was closed.
Description was changed from ========== Sandbox the component updater's patcher utility process. The code will now open files in the browser process before passing the handles across IPC to the utility process. The utility process in turn invokes courgette/bsdiff, which memory maps the files and operates on them as before. There is a behavioral difference when using the courgette or courgette_mini tools: the output file will now be created/overwritten at the start of the operation, and in the case of a failure, will be deleted. Previously, the output file was created late in the operation operation and several failure modes would leave it unmodified. BUG=660325 ========== to ========== Sandbox the component updater's patcher utility process. The code will now open files in the browser process before passing the handles across IPC to the utility process. The utility process in turn invokes courgette/bsdiff, which memory maps the files and operates on them as before. There is a behavioral difference when using the courgette or courgette_mini tools: the output file will now be created/overwritten at the start of the operation, and in the case of a failure, will be deleted. Previously, the output file was created late in the operation operation and several failure modes would leave it unmodified. BUG=660325 Review-Url: https://codereview.chromium.org/2534873005 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Sandbox the component updater's patcher utility process. The code will now open files in the browser process before passing the handles across IPC to the utility process. The utility process in turn invokes courgette/bsdiff, which memory maps the files and operates on them as before. There is a behavioral difference when using the courgette or courgette_mini tools: the output file will now be created/overwritten at the start of the operation, and in the case of a failure, will be deleted. Previously, the output file was created late in the operation operation and several failure modes would leave it unmodified. BUG=660325 Review-Url: https://codereview.chromium.org/2534873005 ========== to ========== Sandbox the component updater's patcher utility process. The code will now open files in the browser process before passing the handles across IPC to the utility process. The utility process in turn invokes courgette/bsdiff, which memory maps the files and operates on them as before. There is a behavioral difference when using the courgette or courgette_mini tools: the output file will now be created/overwritten at the start of the operation, and in the case of a failure, will be deleted. Previously, the output file was created late in the operation operation and several failure modes would leave it unmodified. BUG=660325 Committed: https://crrev.com/53796c72e2ec50eec77f0bd79ec0c8426cee4d58 Cr-Commit-Position: refs/heads/master@{#437669} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/53796c72e2ec50eec77f0bd79ec0c8426cee4d58 Cr-Commit-Position: refs/heads/master@{#437669}
Message was sent while issue was closed.
noel@chromium.org changed reviewers: + noel@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2534873005/diff/60001/chrome/utility/chrome_c... File chrome/utility/chrome_content_utility_client.cc (left): https://codereview.chromium.org/2534873005/diff/60001/chrome/utility/chrome_c... chrome/utility/chrome_content_utility_client.cc:274: if (input_file.empty() || patch_file.empty() || output_file.empty()) { The empty file path detection code was removed here, and I do not see it in component_patcher_operation_out_of_process.cc. So it seems we completely removed the empty path check. How come? (The change description for this patch doesn't say).
Message was sent while issue was closed.
https://codereview.chromium.org/2534873005/diff/60001/chrome/utility/chrome_c... File chrome/utility/chrome_content_utility_client.cc (left): https://codereview.chromium.org/2534873005/diff/60001/chrome/utility/chrome_c... chrome/utility/chrome_content_utility_client.cc:274: if (input_file.empty() || patch_file.empty() || output_file.empty()) { On 2016/12/13 05:42:08, noel gordon wrote: > The empty file path detection code was removed here, and I do not see it in > component_patcher_operation_out_of_process.cc. > > So it seems we completely removed the empty path check. How come? (The change > description for this patch doesn't say). > > It doesn't make sense to check it here, because we're receiving a file handle, not a path. We could do some defensive code in component_patcher_operation_out_of_process to verify it isn't called with empty paths. Is this causing a problem?
Message was sent while issue was closed.
On 2016/12/13 17:38:28, waffles (ooo Jan10 use sorin) wrote: > https://codereview.chromium.org/2534873005/diff/60001/chrome/utility/chrome_c... > File chrome/utility/chrome_content_utility_client.cc (left): > > https://codereview.chromium.org/2534873005/diff/60001/chrome/utility/chrome_c... > chrome/utility/chrome_content_utility_client.cc:274: if (input_file.empty() || > patch_file.empty() || output_file.empty()) { > On 2016/12/13 05:42:08, noel gordon wrote: > > The empty file path detection code was removed here, and I do not see it in > > component_patcher_operation_out_of_process.cc. > > > > So it seems we completely removed the empty path check. How come? (The change > > description for this patch doesn't say). > > It doesn't make sense to check it here, because we're receiving a file handle, > not a path. We could do some defensive code in > component_patcher_operation_out_of_process to verify it isn't called with empty > paths. Right, or DCHECK they are not empty or something. > Is this causing a problem? Maybe. No tests failed, but that's because there are no tests [1], which seems potentially problematic to me. [1] https://codereview.chromium.org/2597513003 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
