From d4dfba8ea1efacb88de636b466140b0176a32097 Mon Sep 17 00:00:00 2001 From: "kwiberg@webrtc.org" Date: Wed, 25 Feb 2015 08:08:59 +0000 Subject: [PATCH] iSAC Decode: Prevent Memcheck from complaining about uninitialized value Without this patch, Valgrind's Memcheck was complaining that the test for whether we should return -1 following the call to WebRtcIsac_DecodeLb made a conditional branch or move based on the value of numSamplesLB, which was uninitialized if WebRtcIsac_DecodeLb failed. However, as can be seen in the source, the control flow only depends on the value of numSamplesLB if numDecodedBytesLB >= 0; i.e., if WebRtcIsac_DecodeLb returned successfully, in which case numSamplesLB is always initialized. The discrepancy is due to the fact that Valgrind works on the generated machine code, which contains spurious such dependencies. The generated code for this test: if ((numDecodedBytesLB < 0) || (numDecodedBytesLB > lenEncodedLBBytes) || (numSamplesLB > MAX_FRAMESAMPLES)) { looks like this: 95: 0f bf 45 d6 movswl -0x2a(%rbp),%eax 99: 3d c0 03 00 00 cmp $0x3c0,%eax 9e: 0f 8f 45 01 00 00 jg 1e9 a4: 44 89 f0 mov %r14d,%eax a7: c1 e0 10 shl $0x10,%eax aa: 0f 88 39 01 00 00 js 1e9 b0: 41 0f bf ce movswl %r14w,%ecx b4: 89 8d 98 e1 ff ff mov %ecx,-0x1e68(%rbp) ba: 41 0f bf c7 movswl %r15w,%eax be: 39 c1 cmp %eax,%ecx c0: 0f 8f 23 01 00 00 jg 1e9 Note how the compiler has seemingly ignored the C language's guarantee that the arguments to || must be evaluated in left-to-right order, and compares numSamplesLB (%eax) with MAX_FRAMESAMPLES (0x3c0, a.k.a. 960) before the other two conditions! If the uninitialized value in numSamplesLB happens to be greater than 960, we'll jump to Decode+0x1e9 (where we'll return -1) without even looking at the other two conditions. Has the compiler generated broken code? Well, no. If numDecodedBytesLB is < 0 so that numSamplesLB is uninitialized, we'll end up jumping to 1e9 whether that value is greater than 960 or not; we'll just do it with different jump instructions. This is entirely invisible as far as the C language is concerned, but the dependency on the uninitialized value is visible at the machine code level, which is why Memcheck complains. This patch solves the problem by pragmatically initializing numSamplesLB before the call even though it isn't necessary other than for placating Memcheck. BUG=4143 R=henrik.lundin@webrtc.org Review URL: https://webrtc-codereview.appspot.com/36309004 Cr-Commit-Position: refs/heads/master@{#8492} git-svn-id: http://webrtc.googlecode.com/svn/trunk@8492 4adac7df-926f-26a2-2b94-8c16560cd09d --- tools/valgrind-webrtc/memcheck/suppressions.txt | 14 -------------- .../audio_coding/codecs/isac/main/source/isac.c | 7 +++++++ 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/tools/valgrind-webrtc/memcheck/suppressions.txt b/tools/valgrind-webrtc/memcheck/suppressions.txt index c9495cbc40..1c02366631 100644 --- a/tools/valgrind-webrtc/memcheck/suppressions.txt +++ b/tools/valgrind-webrtc/memcheck/suppressions.txt @@ -612,20 +612,6 @@ fun:_ZN12_GLOBAL__N_118StatsCollectorTest22TestCertificateReportsERKN3rtc18FakeSSLCertificateERKSt6vectorISsSaISsEES4_S9_ fun:_ZN12_GLOBAL__N_156StatsCollectorTest_ChainedCertificateReportsCreated_Test8TestBodyEv } -{ - bug_4143 - Memcheck:Uninitialized - fun:Decode - fun:WebRtcIsac_Decode - fun:_ZN6webrtc9IsacFloat6DecodeEP16WebRtcISACStructPKhsPsS5_ - fun:_ZN6webrtc24AudioEncoderDecoderIsacTINS_9IsacFloatEE6DecodeEPKhmPsPNS_12AudioDecoder10SpeechTypeE - fun:_ZThn8_N6webrtc24AudioEncoderDecoderIsacTINS_9IsacFloatEE6DecodeEPKhmPsPNS_12AudioDecoder10SpeechTypeE - fun:_ZN6webrtc9NetEqImpl10DecodeLoopEPSt4listIPNS_6PacketESaIS3_EEPNS_10OperationsEPNS_12AudioDecoderEPiPNS9_10SpeechTypeE - fun:_ZN6webrtc9NetEqImpl6DecodeEPSt4listIPNS_6PacketESaIS3_EEPNS_10OperationsEPiPNS_12AudioDecoder10SpeechTypeE - fun:_ZN6webrtc9NetEqImpl16GetAudioInternalEmPsPiS2_ - fun:_ZN6webrtc9NetEqImpl8GetAudioEmPsPiS2_PNS_15NetEqOutputTypeE - fun:_ZN6webrtc35NetEqDecodingTest_DecoderError_Test8TestBodyEv -} { bug_4147_1 Memcheck:Unaddressable diff --git a/webrtc/modules/audio_coding/codecs/isac/main/source/isac.c b/webrtc/modules/audio_coding/codecs/isac/main/source/isac.c index d2260e26bb..db78e6de2e 100644 --- a/webrtc/modules/audio_coding/codecs/isac/main/source/isac.c +++ b/webrtc/modules/audio_coding/codecs/isac/main/source/isac.c @@ -1096,6 +1096,13 @@ static int16_t Decode(ISACStruct* ISAC_main_inst, memcpy(instISAC->instLB.ISACdecLB_obj.bitstr_obj.stream, encoded, lenEncodedLBBytes); + /* We need to initialize numSamplesLB to something; otherwise, in the test + for whether we should return -1 below, the compiler might generate code + that fools Memcheck (Valgrind) into thinking that the control flow depends + on the uninitialized value in numSamplesLB (since WebRtcIsac_DecodeLb will + not fill it in if it fails and returns -1). */ + numSamplesLB = 0; + /* Regardless of that the current codec is setup to work in * wideband or super-wideband, the decoding of the lower-band * has to be performed. */