Last Comment Bug 746920 - Uninitialised value use in nsDiskCacheMap::FlushRecords
: Uninitialised value use in nsDiskCacheMap::FlushRecords
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: Trunk
: x86_64 Linux
-- normal (vote)
: mozilla17
Assigned To: Michal Novotny (:michal)
: Patrick McManus [:mcmanus]
Depends on:
  Show dependency treegraph
Reported: 2012-04-19 02:39 PDT by Julian Seward [:jseward]
Modified: 2012-08-08 09:27 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

fix (1.91 KB, patch)
2012-04-26 15:45 PDT, Michal Novotny (:michal)
hurley: review+
Details | Diff | Splinter Review

Description User image Julian Seward [:jseward] 2012-04-19 02:39:16 PDT
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 User image Michal Novotny (:michal) 2012-04-26 15:45:22 PDT
Created attachment 618831 [details] [diff] [review]

>    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 User image Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-06-12 14:39:23 PDT
Comment on attachment 618831 [details] [diff] [review]

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 User image OFFLINE UNTIL 27 FEB 2017 Nicholas Hurley [:nwgh][:hurley] (also 2012-06-12 16:55:50 PDT
Comment on attachment 618831 [details] [diff] [review]

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

Looks good to me.
Comment 5 User image Ed Morley [:emorley] 2012-08-08 09:27:10 PDT

Note You need to log in before you can comment on or make changes to this bug.