Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2717123004:
Avoid overflow in WebRtcSpl_DotProductWithScale (Closed)
|
Created:
3 years, 9 months ago by hlundin-webrtc Modified:
3 years, 9 months ago Reviewers:
kwiberg-webrtc CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, peah-webrtc, minyue-webrtc, aluebs-webrtc, bjornv1 Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAvoid overflow in WebRtcSpl_DotProductWithScale
BUG=chromium:676935
Review-Url: https://codereview.webrtc.org/2717123004
Cr-Commit-Position: refs/heads/master@{#17091}
Committed: https://chromium.googlesource.com/external/webrtc/+/d461ffce2ac65ceaa6e6e4bb8d1abe2b0be87bd7
Patch Set 1 #
Total comments: 2
Patch Set 2 : Convert to C++ and use saturated_cast #
Total comments: 2
Patch Set 3 : Move to separate target #
Total comments: 6
Patch Set 4 : Remove old comment #Patch Set 5 : Rebase #Patch Set 6 : Fixing deps problems #
Total comments: 3
Patch Set 7 : Remove wd4334 #
Messages
Total messages: 34 (14 generated)
henrik.lundin@webrtc.org changed reviewers: + kwiberg@webrtc.org
kwiberg@, ptal
https://codereview.webrtc.org/2717123004/diff/1/webrtc/common_audio/signal_pr... File webrtc/common_audio/signal_processing/dot_product_with_scale.c (right): https://codereview.webrtc.org/2717123004/diff/1/webrtc/common_audio/signal_pr... webrtc/common_audio/signal_processing/dot_product_with_scale.c:31: return (int32_t) sum; You'll get garbage if this cast truncates. And wasn't the reason for this change that the int32_t sum was found to be overflowing? Suggestion: This is a tiny file. Convert it to C++ so that you can use saturated_cast.
PTAL again. https://codereview.webrtc.org/2717123004/diff/1/webrtc/common_audio/signal_pr... File webrtc/common_audio/signal_processing/dot_product_with_scale.c (right): https://codereview.webrtc.org/2717123004/diff/1/webrtc/common_audio/signal_pr... webrtc/common_audio/signal_processing/dot_product_with_scale.c:31: return (int32_t) sum; On 2017/02/28 10:07:47, kwiberg-webrtc wrote: > You'll get garbage if this cast truncates. And wasn't the reason for this change > that the int32_t sum was found to be overflowing? > > Suggestion: This is a tiny file. Convert it to C++ so that you can use > saturated_cast. Done.
https://codereview.webrtc.org/2717123004/diff/20001/webrtc/common_audio/BUILD.gn File webrtc/common_audio/BUILD.gn (right): https://codereview.webrtc.org/2717123004/diff/20001/webrtc/common_audio/BUILD... webrtc/common_audio/BUILD.gn:50: "signal_processing/dot_product_with_scale.cc", The header file is still in common_audio_c, and that target doesn't depend on this one. I'd suggest moving dot_product_with_scale.cc to its own target, and give it its own header file there. Then #include that header from signal_processing_library.h, and add the required dep from common_audio_c on your new target---or just add the #include to all callers directly, since signal_processing_library.h isn't a public header?
PTAL again. https://codereview.webrtc.org/2717123004/diff/20001/webrtc/common_audio/BUILD.gn File webrtc/common_audio/BUILD.gn (right): https://codereview.webrtc.org/2717123004/diff/20001/webrtc/common_audio/BUILD... webrtc/common_audio/BUILD.gn:50: "signal_processing/dot_product_with_scale.cc", On 2017/02/28 12:11:21, kwiberg-webrtc wrote: > The header file is still in common_audio_c, and that target doesn't depend on > this one. > > I'd suggest moving dot_product_with_scale.cc to its own target, and give it its > own header file there. Then #include that header from > signal_processing_library.h, and add the required dep from common_audio_c on > your new target---or just add the #include to all callers directly, since > signal_processing_library.h isn't a public header? Done. https://codereview.webrtc.org/2717123004/diff/40001/webrtc/common_audio/signa... File webrtc/common_audio/signal_processing/include/signal_processing_library.h (right): https://codereview.webrtc.org/2717123004/diff/40001/webrtc/common_audio/signa... webrtc/common_audio/signal_processing/include/signal_processing_library.h:612: // int32_t WebRtcSpl_DotProductWithScale(const int16_t* vector1, The declaration is intentionally left in this file. I think this is the pattern used here, so that this file serves as the master catalog for SPL functions.
https://codereview.webrtc.org/2717123004/diff/40001/webrtc/common_audio/BUILD.gn File webrtc/common_audio/BUILD.gn (right): https://codereview.webrtc.org/2717123004/diff/40001/webrtc/common_audio/BUILD... webrtc/common_audio/BUILD.gn:215: cflags = [ "/wd4334" ] # Ignore warning on shift operator promotion. This looks like an easy warning to just fix in this one tiny file: https://msdn.microsoft.com/en-us/library/ke55d167.aspx https://codereview.webrtc.org/2717123004/diff/40001/webrtc/common_audio/signa... File webrtc/common_audio/signal_processing/include/signal_processing_library.h (right): https://codereview.webrtc.org/2717123004/diff/40001/webrtc/common_audio/signa... webrtc/common_audio/signal_processing/include/signal_processing_library.h:612: // int32_t WebRtcSpl_DotProductWithScale(const int16_t* vector1, On 2017/02/28 12:35:17, hlundin-webrtc wrote: > The declaration is intentionally left in this file. I think this is the pattern > used here, so that this file serves as the master catalog for SPL functions. I think this is a bad idea. It's just a question of time before this comment and the real declaration diverge. This is a job for doxygen or something similar.
https://codereview.webrtc.org/2717123004/diff/40001/webrtc/common_audio/signa... File webrtc/common_audio/signal_processing/include/signal_processing_library.h (right): https://codereview.webrtc.org/2717123004/diff/40001/webrtc/common_audio/signa... webrtc/common_audio/signal_processing/include/signal_processing_library.h:612: // int32_t WebRtcSpl_DotProductWithScale(const int16_t* vector1, On 2017/02/28 12:51:01, kwiberg-webrtc wrote: > On 2017/02/28 12:35:17, hlundin-webrtc wrote: > > The declaration is intentionally left in this file. I think this is the > pattern > > used here, so that this file serves as the master catalog for SPL functions. > > I think this is a bad idea. It's just a question of time before this comment and > the real declaration diverge. > > This is a job for doxygen or something similar. Would you like me to keep lines 601--611 in this file?
https://codereview.webrtc.org/2717123004/diff/40001/webrtc/common_audio/signa... File webrtc/common_audio/signal_processing/include/signal_processing_library.h (right): https://codereview.webrtc.org/2717123004/diff/40001/webrtc/common_audio/signa... webrtc/common_audio/signal_processing/include/signal_processing_library.h:612: // int32_t WebRtcSpl_DotProductWithScale(const int16_t* vector1, On 2017/03/06 12:43:26, hlundin-webrtc wrote: > On 2017/02/28 12:51:01, kwiberg-webrtc wrote: > > On 2017/02/28 12:35:17, hlundin-webrtc wrote: > > > The declaration is intentionally left in this file. I think this is the > > pattern > > > used here, so that this file serves as the master catalog for SPL functions. > > > > I think this is a bad idea. It's just a question of time before this comment > and > > the real declaration diverge. > > > > This is a job for doxygen or something similar. > > Would you like me to keep lines 601--611 in this file? I would prefer that you delete WebRtcSpl_DotProductWithScale entirely from this file and have it just in the new header. Having it just in this file is still better than duplicating the information, though. Duplication is a public health hazard... Oh, but wait. If you keep it here, you'll need to make this header part of both build targets. (You can't put it in just one and have a dep for the other, because you'll end up with circular deps.) If that's OK, you can keep the declaration here. Otherwise, my recommendation is the usual 1:1 relationship between .h and .cc files...
https://codereview.webrtc.org/2717123004/diff/40001/webrtc/common_audio/signa... File webrtc/common_audio/signal_processing/include/signal_processing_library.h (right): https://codereview.webrtc.org/2717123004/diff/40001/webrtc/common_audio/signa... webrtc/common_audio/signal_processing/include/signal_processing_library.h:612: // int32_t WebRtcSpl_DotProductWithScale(const int16_t* vector1, On 2017/03/06 14:35:50, kwiberg-webrtc wrote: > On 2017/03/06 12:43:26, hlundin-webrtc wrote: > > On 2017/02/28 12:51:01, kwiberg-webrtc wrote: > > > On 2017/02/28 12:35:17, hlundin-webrtc wrote: > > > > The declaration is intentionally left in this file. I think this is the > > > pattern > > > > used here, so that this file serves as the master catalog for SPL > functions. > > > > > > I think this is a bad idea. It's just a question of time before this comment > > and > > > the real declaration diverge. > > > > > > This is a job for doxygen or something similar. > > > > Would you like me to keep lines 601--611 in this file? > > I would prefer that you delete WebRtcSpl_DotProductWithScale entirely from this > file and have it just in the new header. Having it just in this file is still > better than duplicating the information, though. Duplication is a public health > hazard... > > Oh, but wait. If you keep it here, you'll need to make this header part of both > build targets. (You can't put it in just one and have a dep for the other, > because you'll end up with circular deps.) If that's OK, you can keep the > declaration here. Otherwise, my recommendation is the usual 1:1 relationship > between .h and .cc files... Done.
lgtm Thanks for indulging me...
The CQ bit was checked by henrik.lundin@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/23592) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/22364) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
Rebase
The CQ bit was checked by henrik.lundin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2717123004/#ps80001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_arm on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_arm/builds/6504)
The CQ bit was checked by henrik.lundin@webrtc.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.webrtc.org/...
lgtm https://codereview.webrtc.org/2717123004/diff/100001/webrtc/common_audio/BUIL... File webrtc/common_audio/BUILD.gn (right): https://codereview.webrtc.org/2717123004/diff/100001/webrtc/common_audio/BUIL... webrtc/common_audio/BUILD.gn:220: cflags = [ "/wd4334" ] # Ignore warning on shift operator promotion. Ideally, you'd fix this warning instead since it's such a small amount of code, but I guess it's OK to let it be for now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2717123004/diff/100001/webrtc/common_audio/BUIL... File webrtc/common_audio/BUILD.gn (right): https://codereview.webrtc.org/2717123004/diff/100001/webrtc/common_audio/BUIL... webrtc/common_audio/BUILD.gn:220: cflags = [ "/wd4334" ] # Ignore warning on shift operator promotion. On 2017/03/07 10:27:21, kwiberg-webrtc wrote: > Ideally, you'd fix this warning instead since it's such a small amount of code, > but I guess it's OK to let it be for now. Done.
The CQ bit was checked by henrik.lundin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2717123004/#ps120001 (title: "Remove wd4334")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2717123004/diff/100001/webrtc/common_audio/BUIL... File webrtc/common_audio/BUILD.gn (right): https://codereview.webrtc.org/2717123004/diff/100001/webrtc/common_audio/BUIL... webrtc/common_audio/BUILD.gn:220: cflags = [ "/wd4334" ] # Ignore warning on shift operator promotion. On 2017/03/07 11:40:05, hlundin-webrtc wrote: > On 2017/03/07 10:27:21, kwiberg-webrtc wrote: > > Ideally, you'd fix this warning instead since it's such a small amount of > code, > > but I guess it's OK to let it be for now. > > Done. Aha, it was that easy!
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488886810426830,
"parent_rev": "57f19cc0cdd295ec409b0a05b67c0e993419f181", "commit_rev":
"d461ffce2ac65ceaa6e6e4bb8d1abe2b0be87bd7"}
Message was sent while issue was closed.
Description was changed from ========== Avoid overflow in WebRtcSpl_DotProductWithScale BUG=chromium:676935 ========== to ========== Avoid overflow in WebRtcSpl_DotProductWithScale BUG=chromium:676935 Review-Url: https://codereview.webrtc.org/2717123004 Cr-Commit-Position: refs/heads/master@{#17091} Committed: https://chromium.googlesource.com/external/webrtc/+/d461ffce2ac65ceaa6e6e4bb8... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/d461ffce2ac65ceaa6e6e4bb8... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
