Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1979973003:
Fix UBSan errors (left shift of negative value, left shift overflows int) (Closed)
|
Created:
4 years, 7 months ago by kwiberg-webrtc Modified:
4 years, 7 months ago Reviewers:
tlegrand-webrtc CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, minyue-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix UBSan errors (left shift of negative value, left shift overflows int)
BUG=chromium:603498
Committed: https://crrev.com/89f237cedcb8204f2c5553673669f55a41a9e2d9
Cr-Commit-Position: refs/heads/master@{#12787}
Patch Set 1 #
Total comments: 6
Patch Set 2 : variable name #
Messages
Total messages: 12 (4 generated)
kwiberg@webrtc.org changed reviewers: + tina.legrand@webrtc.org
https://codereview.webrtc.org/1979973003/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/isac/main/source/arith_routines_hist.c (right): https://codereview.webrtc.org/1979973003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/isac/main/source/arith_routines_hist.c:220: streamval |= (uint32_t)(*++stream_ptr); In the first line, *stream_ptr was promoted from uint8_t to int, then left-shifted 24 steps. If the value is >= 128, this leads to signed overflow. Solve by manually casting to uint32_t instead. Also do the same on the other three lines for symmetry. https://codereview.webrtc.org/1979973003/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/isac/main/source/entropy_coding.c (right): https://codereview.webrtc.org/1979973003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/isac/main/source/entropy_coding.c:167: CurveQ16[k] += d; diffQ16[k] was sometimes negative, and left shift of a negative value leads to undefined behavior. Solve by casting to unsigned, and then back again (both casts are compile-time-only affairs that are no-ops at runtime). Also extract the common subexpression, so we don't have to have two copies of that mouthful.
One nit, and then LGTM https://codereview.webrtc.org/1979973003/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/isac/main/source/entropy_coding.c (right): https://codereview.webrtc.org/1979973003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/isac/main/source/entropy_coding.c:165: int32_t d = (int32_t)((uint32_t)(diffQ16[k]) << shftVal); "d" is not a very nice variable name. Can you rename to something more descriptive, maybe "diffQ16" will do?
https://codereview.webrtc.org/1979973003/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/isac/main/source/entropy_coding.c (right): https://codereview.webrtc.org/1979973003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/isac/main/source/entropy_coding.c:165: int32_t d = (int32_t)((uint32_t)(diffQ16[k]) << shftVal); On 2016/05/17 12:43:37, tlegrand-webrtc wrote: > "d" is not a very nice variable name. Can you rename to something more > descriptive, maybe "diffQ16" will do? Yes, "d" is short and therefore not very descriptive, but its scope is just the next two lines. It doesn't have to be descriptive, because the definition is *right there*. I'll change it if you still don't agree (to "diff_q16_shifted"? "diffQ16" is already taken), but IMO this will decrease rather than increase legibility.
Yupp, still want you to change the variable name. :) https://codereview.webrtc.org/1979973003/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/isac/main/source/entropy_coding.c (right): https://codereview.webrtc.org/1979973003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/isac/main/source/entropy_coding.c:165: int32_t d = (int32_t)((uint32_t)(diffQ16[k]) << shftVal); On 2016/05/17 13:10:39, kwiberg-webrtc wrote: > On 2016/05/17 12:43:37, tlegrand-webrtc wrote: > > "d" is not a very nice variable name. Can you rename to something more > > descriptive, maybe "diffQ16" will do? > > Yes, "d" is short and therefore not very descriptive, but its scope is just the > next two lines. It doesn't have to be descriptive, because the definition is > *right there*. > > I'll change it if you still don't agree (to "diff_q16_shifted"? "diffQ16" is > already taken), but IMO this will decrease rather than increase legibility. Yes, sorry, it is of course taken. I've had trouble in the past with short names, because it is impossible to search for other places where the variable might be used. And I don't think the style guide has exceptions for variables only being used on adjacent lines. You normally don't know what will happen with the code in the future, so that could potentially change. Maybe not in this old code, but I like consistency.
https://codereview.webrtc.org/1979973003/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/isac/main/source/entropy_coding.c (right): https://codereview.webrtc.org/1979973003/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/isac/main/source/entropy_coding.c:165: int32_t d = (int32_t)((uint32_t)(diffQ16[k]) << shftVal); On 2016/05/17 14:01:06, tlegrand-webrtc wrote: > On 2016/05/17 13:10:39, kwiberg-webrtc wrote: > > On 2016/05/17 12:43:37, tlegrand-webrtc wrote: > > > "d" is not a very nice variable name. Can you rename to something more > > > descriptive, maybe "diffQ16" will do? > > > > Yes, "d" is short and therefore not very descriptive, but its scope is just > the > > next two lines. It doesn't have to be descriptive, because the definition is > > *right there*. > > > > I'll change it if you still don't agree (to "diff_q16_shifted"? "diffQ16" is > > already taken), but IMO this will decrease rather than increase legibility. > > Yes, sorry, it is of course taken. I've had trouble in the past with short > names, because it is impossible to search for other places where the variable > might be used. Yes, but that's not all that relevant for variables with very limited scope (which is the only case where a short name should be used---otherwise, some uses of the name will inevitably be far from the definition). > And I don't think the style guide has exceptions for variables > only being used on adjacent lines. You're right---it doesn't. I'll ask if that's on purpose. > You normally don't know what will happen with > the code in the future, so that could potentially change. Maybe not in this old > code, but I like consistency. I just feel like this is the same sort of thinking that often gets me in trouble, writing code that's more general than what's currently needed. If the additional work (including testing) isn't negligible, and it won't cost extra to change it if and when the extra generality is needed, it's usually a good idea to keep your fingers in check and write just what you need at the moment. Variable names are of course much less work than whole chunks of code, but the cost of inventing informative variable names is not zero, and they take more space on the screen and in the mind when you read the code. Well, enough arguing. I'm landing this CL with a longer variable name now.
The CQ bit was checked by kwiberg@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tina.legrand@webrtc.org Link to the patchset: https://codereview.webrtc.org/1979973003/#ps20001 (title: "variable name")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1979973003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1979973003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix UBSan errors (left shift of negative value, left shift overflows int) BUG=chromium:603498 ========== to ========== Fix UBSan errors (left shift of negative value, left shift overflows int) BUG=chromium:603498 Committed: https://crrev.com/89f237cedcb8204f2c5553673669f55a41a9e2d9 Cr-Commit-Position: refs/heads/master@{#12787} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/89f237cedcb8204f2c5553673669f55a41a9e2d9 Cr-Commit-Position: refs/heads/master@{#12787} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
