746920 - Uninitialised value use in nsDiskCacheMap::FlushRecords

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Julian Seward [:jseward]]

I've never seen this one before; perhaps it's new?


Unfortunately I don't have good STR.  I had to run the entire first
1/10th of mochitests on Valgrind in order to repro it, using the
command line:

for ((qq = 1; qq <= 1; qq++)); do (DISPLAY=:3.0 make -C ff-opt mochitest-plain EXTRA_TEST_ARGS="--this-chunk=$qq --total-chunks=10 --close-when-done --debugger=/home/sewardj/VgTRUNK/tchain/Inst/bin/valgrind --debugger-args='--smc-check=all-non-file --suppressions=/home/sewardj/MOZ/SUPPS/mochitest-mc.supp --error-limit=no --stats=no --trace-children=yes --child-silent-after-fork=yes "--trace-children-skip=/usr/bin/hg,/bin/rm,*/bin/certutil,*/bin/pk12util,*/bin/ssltunnel,*/bin/uname,*/bin/which,*/bin/ps,*/bin/grep" --tool=memcheck --track-origins=no'") 2>&1 | tee spew-mc-1--chunk-$qq-of-10-XXX-2 ; done

Note the use of --this-chunk and --total-chunks


The resulting complaint happens only at the end of the run (which is about 30
CPU minutes running Memcheck on a fast machine):


Conditional jump or move depends on uninitialised value(s)
   at 0x62FB818: nsDiskCacheMap::FlushRecords(bool) (nsDiskCacheMap.cpp:268)
   by 0x62FC297: nsDiskCacheMap::Close(bool) (nsDiskCacheMap.cpp:197)
   by 0x62F3F05: nsDiskCacheDevice::Shutdown_Private(bool) (nsDiskCacheDevice.cpp:485)
   by 0x62F3F3D: nsDiskCacheDevice::Shutdown() (nsDiskCacheDevice.cpp:462)
   by 0x62EFD89: nsCacheService::OnProfileShutdown(bool) (nsCacheService.cpp:2193)
   by 0x62F039D: nsCacheProfilePrefObserver::Observe(nsISupports*, char const*, unsigned short const*) (nsCacheService.cpp:420)
   by 0x6FCB639: nsObserverList::NotifyObservers(nsISupports*, char const*, unsigned short const*) (nsObserverList.cpp:130)
   by 0x6FCB921: nsObserverService::NotifyObservers(nsISupports*, char const*, unsigned short const*) (nsObserverService.cpp:182)
   by 0x6282756: nsXREDirProvider::DoShutdown() (nsXREDirProvider.cpp:852)
   by 0x627B2D3: ScopedXPCOMStartup::~ScopedXPCOMStartup() (nsAppRunner.cpp:1125)
   by 0x6280AC7: XREMain::XRE_main(int, char**, nsXREAppData const*) (nsAppRunner.cpp:3867)
   by 0x6280DC9: XRE_main (nsAppRunner.cpp:3921)

 Uninitialised value was created by a heap allocation
   at 0x4029C92: realloc (vg_replace_malloc.c:632)
   by 0x62FBB6F: nsDiskCacheMap::ShrinkRecords() (nsDiskCacheMap.cpp:383)
   by 0x62FBBF5: nsDiskCacheMap::Trim() (nsDiskCacheMap.cpp:225)
   by 0x62F414C: nsDiskCacheDevice::EvictEntries(char const*) (nsDiskCacheDevice.cpp:1003)
   by 0x62EEC3A: nsCacheService::EvictEntriesForClient(char const*, int) (nsCacheService.cpp:1394)
   by 0x7002A07: NS_InvokeByIndex_P (xptcinvoke_x86_64_unix.cpp:195)
   by 0x6B5166A: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:3114)
   by 0x6B579CF: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1549)
   by 0x73226D6: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:314)
   by 0x731BA9B: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2757)
   by 0x73220FE: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:475)
   by 0x732279C: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jsinterp.cpp:535)

Comment 1

[Michal Novotny [:michal]]

Attachment: fix

>    if (newRecordsPerBucket < kMinRecordCount)
>        newRecordsPerBucket = kMinRecordCount;

newRecordsPerBucket should be compared against minimum per bucket count instead of minimum total count. The wrong minimum count causes growing the records in same cases and the newly allocated memory is not initialized.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 618831 [details] [diff] [review]
fix

Review of attachment 618831 [details] [diff] [review]:
-----------------------------------------------------------------

Nick, I am not familiar with this code. Could you review the patch? If not, maybe Jason would be a better reviewer.

Comment 3

[u408661]

Comment on attachment 618831 [details] [diff] [review]
fix

Review of attachment 618831 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 4

[Michal Novotny [:michal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fe07a3a8458

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5fe07a3a8458