Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2182873003:
Add bounds check for negative ctrlsrc->z. (Closed)
|
Created:
4 years, 4 months ago by rickyz (no longer on Chrome) Modified:
4 years, 4 months ago CC:
chromium-reviews, Will Harris Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd bounds check for negative ctrlsrc->z.
Also adds some basic integer overflow checking and fixes undefined
pointer arithmetic in preexisting bounds checks.
BUG=631375
Committed: https://crrev.com/df3791021e31e0b99c5a64fea1161d0cf8aad5df
Cr-Commit-Position: refs/heads/master@{#414156}
Patch Set 1 #Patch Set 2 : Add check on cblen. #Patch Set 3 : Address signed comparison warnings. #
Total comments: 8
Patch Set 4 : Oops. #Patch Set 5 : More build fixes. #Patch Set 6 : Add regression test with rolled back mbspatch.cc. #Patch Set 7 : Re-add mbspatch.cc with more MSVC fixes. Sorry for the noise, don't have a windows build env. #Patch Set 8 : Try again with more negative offset. #Patch Set 9 : Last attempt before bed #Patch Set 10 : I give up. #Patch Set 11 : Add fix again. #
Total comments: 4
Patch Set 12 : Respond to comnets. #Patch Set 13 : Rebase. #
Messages
Total messages: 58 (39 generated)
The CQ bit was checked by rickyz@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 ========== Add bounds check for negative ctrlsrc->z. Also adds some basic integer overflow checking and fixes undefined pointer arithmetic in preexisting bounds checks. BUG=631375 ========== to ========== Add bounds check for negative ctrlsrc->z. Also adds some basic integer overflow checking and fixes undefined pointer arithmetic in preexisting bounds checks. BUG=631375 ==========
rickyz@chromium.org changed reviewers: + kcwu@chromium.org, wfh@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
rickyz@chromium.org changed reviewers: + huangs@chromium.org
The CQ bit was checked by rickyz@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...
https://codereview.chromium.org/2182873003/diff/40001/third_party/bspatch/mbs... File third_party/bspatch/mbspatch.cc (right): https://codereview.chromium.org/2182873003/diff/40001/third_party/bspatch/mbs... third_party/bspatch/mbspatch.cc:85: size_t size = static_cast<size_t>(hs.st_size); size -= sizeof(MBSPatchHeader); https://codereview.chromium.org/2182873003/diff/40001/third_party/bspatch/mbs... third_party/bspatch/mbspatch.cc:86: if (slen < header->cblen || header->cblen % sizeof(MBSPatchTriple) != 0) s/slen/size/ https://codereview.chromium.org/2182873003/diff/40001/third_party/bspatch/mbs... third_party/bspatch/mbspatch.cc:129: wb += c good catch. but missed semicolon; https://codereview.chromium.org/2182873003/diff/40001/third_party/bspatch/mbs... third_party/bspatch/mbspatch.cc:139: if (header->cblen % sizeof(MBSPatchTriple) != 0) { This is redundant? You have checked it in MBS_ReadHeader.
The CQ bit was checked by rickyz@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...
https://codereview.chromium.org/2182873003/diff/40001/third_party/bspatch/mbs... File third_party/bspatch/mbspatch.cc (right): https://codereview.chromium.org/2182873003/diff/40001/third_party/bspatch/mbs... third_party/bspatch/mbspatch.cc:85: size_t size = static_cast<size_t>(hs.st_size); On 2016/07/27 04:14:36, kcwu wrote: > size -= sizeof(MBSPatchHeader); Done. https://codereview.chromium.org/2182873003/diff/40001/third_party/bspatch/mbs... third_party/bspatch/mbspatch.cc:86: if (slen < header->cblen || header->cblen % sizeof(MBSPatchTriple) != 0) On 2016/07/27 04:14:36, kcwu wrote: > s/slen/size/ Done. https://codereview.chromium.org/2182873003/diff/40001/third_party/bspatch/mbs... third_party/bspatch/mbspatch.cc:129: wb += c On 2016/07/27 04:14:36, kcwu wrote: > good catch. but missed semicolon; Oops, apologies for all of the build errors, been a while since I've made a chromium change, so don't have a build setup fully ready to go yet. https://codereview.chromium.org/2182873003/diff/40001/third_party/bspatch/mbs... third_party/bspatch/mbspatch.cc:139: if (header->cblen % sizeof(MBSPatchTriple) != 0) { On 2016/07/27 04:14:36, kcwu wrote: > This is redundant? You have checked it in MBS_ReadHeader. I was debating between doing validation earlier vs. having the check next to the code that depends on it. I ended up going with the latter.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by rickyz@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...
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 rickyz@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
where is the actual fix?
The CQ bit was checked by rickyz@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...
On 2016/07/27 16:40:20, Will Harris wrote: > where is the actual fix? Sorry for the noise, I have reuploaded the fix. I have not demonstrated that the regression tests fail before the fix on windows, but leaving them in anyway - I did verify that they cause a crash when I hand-compile the kcwu's repro with CRC verification reenabled on Linux.
On 2016/07/27 16:40:20, Will Harris wrote: > where is the actual fix? Sorry for the noise, I have reuploaded the fix. I have not demonstrated that the regression tests fail before the fix on windows, but leaving them in anyway - I did verify that they cause a crash when I hand-compile the kcwu's repro with CRC verification reenabled on Linux.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Just a couple of comments. https://codereview.chromium.org/2182873003/diff/200001/third_party/bspatch/mb... File third_party/bspatch/mbspatch.cc (right): https://codereview.chromium.org/2182873003/diff/200001/third_party/bspatch/mb... third_party/bspatch/mbspatch.cc:32: */ Update ChangeLog? https://codereview.chromium.org/2182873003/diff/200001/third_party/bspatch/mb... third_party/bspatch/mbspatch.cc:125: while (r) { For robustness, maybe while (r > 0) ? Otherwise "r -= c" might transition from positive to negative of data are corrupt?
wfh@chromium.org changed reviewers: - wfh@chromium.org
https://codereview.chromium.org/2182873003/diff/200001/third_party/bspatch/mb... File third_party/bspatch/mbspatch.cc (right): https://codereview.chromium.org/2182873003/diff/200001/third_party/bspatch/mb... third_party/bspatch/mbspatch.cc:32: */ On 2016/07/27 22:39:16, huangs wrote: > Update ChangeLog? Done. https://codereview.chromium.org/2182873003/diff/200001/third_party/bspatch/mb... third_party/bspatch/mbspatch.cc:125: while (r) { On 2016/07/27 22:39:16, huangs wrote: > For robustness, maybe > while (r > 0) > ? > Otherwise "r -= c" might transition from positive to negative of data are > corrupt? This should be impossible because c is at most r (due to the read argument), right?
The CQ bit was checked by rickyz@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Friendly ping, does this look good to land?
On 2016/08/01 at 23:51:21, rickyz wrote: > Friendly ping, does this look good to land? Another friendly ping :-)
On 2016/08/12 17:36:59, rickyz wrote: > On 2016/08/01 at 23:51:21, rickyz wrote: > > Friendly ping, does this look good to land? > > Another friendly ping :-) Not sure you are pinging huangs or me. Anyway, the code lgtm. But I have difficulty to see the test data. What can I do? The web interface don't show binary data in the diff. And git cl patch also failed. $ git cl patch 2182873003 fatal: git diff header lacks filename information when removing 0 leading pathname components (line 48) Failed to apply the patch
The CQ bit was checked by rickyz@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...
On 2016/08/17 at 11:31:32, kcwu wrote: > On 2016/08/12 17:36:59, rickyz wrote: > > On 2016/08/01 at 23:51:21, rickyz wrote: > > > Friendly ping, does this look good to land? > > > > Another friendly ping :-) > > Not sure you are pinging huangs or me. Anyway, the code lgtm. > > But I have difficulty to see the test data. What can I do? > The web interface don't show binary data in the diff. > And git cl patch also failed. > > $ git cl patch 2182873003 > fatal: git diff header lacks filename information when removing 0 leading pathname components (line 48) > Failed to apply the patch Thanks for taking a look - I just wanted another set of eyes on this before I add folks for OWNERs. Not sure why git cl patch fails, but here are the binary files from the change: $ xxd bin.old 00000000: f0 . $ xxd misaligned_cblen.diff 00000000: 4d42 4449 4646 3130 0000 0001 9040 e26e MBDIFF10.....@.n 00000010: 0000 0000 0000 0003 0000 0001 0000 0001 ................ 00000020: 0000 0001 0a ..... $ xxd negative_seek.diff 00000000: 4d42 4449 4646 3130 0000 0001 9040 e26e MBDIFF10.....@.n 00000010: 0000 0001 0000 0018 0000 0001 0000 0000 ................ 00000020: 0000 0000 0000 0000 ff00 0000 0000 0001 ................ 00000030: 0000 0000 0000 0000 00 .........
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rickyz@chromium.org changed reviewers: + jochen@chromium.org
Adding jochen@ for third_party/ and chrome/ OWNERS, thanks!
lgtm
The CQ bit was checked by rickyz@chromium.org
Thanks!
The patchset sent to the CQ was uploaded after l-g-t-m from kcwu@chromium.org Link to the patchset: https://codereview.chromium.org/2182873003/#ps240001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add bounds check for negative ctrlsrc->z. Also adds some basic integer overflow checking and fixes undefined pointer arithmetic in preexisting bounds checks. BUG=631375 ========== to ========== Add bounds check for negative ctrlsrc->z. Also adds some basic integer overflow checking and fixes undefined pointer arithmetic in preexisting bounds checks. BUG=631375 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Add bounds check for negative ctrlsrc->z. Also adds some basic integer overflow checking and fixes undefined pointer arithmetic in preexisting bounds checks. BUG=631375 ========== to ========== Add bounds check for negative ctrlsrc->z. Also adds some basic integer overflow checking and fixes undefined pointer arithmetic in preexisting bounds checks. BUG=631375 Committed: https://crrev.com/df3791021e31e0b99c5a64fea1161d0cf8aad5df Cr-Commit-Position: refs/heads/master@{#414156} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/df3791021e31e0b99c5a64fea1161d0cf8aad5df Cr-Commit-Position: refs/heads/master@{#414156} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
