Bug 50277 - key member variable in NumericStrings CacheData is never initialized
Summary: key member variable in NumericStrings CacheData is never initialized
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-30 15:06 PST by Xan Lopez
Modified: 2010-12-01 04:25 PST (History)
4 users (show)

See Also:


Attachments
cacheentryinit.diff (2.06 KB, patch)
2010-11-30 15:10 PST, Xan Lopez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xan Lopez 2010-11-30 15:06:52 PST
Gives warnings like the following, where the key will be garbage before an element is introduced.

==13821== 4 errors in context 6 of 917:
==13821== Conditional jump or move depends on uninitialised value(s)
==13821==    at 0x43B57C9: JSC::NumericStrings::add(int) (NumericStrings.h:52)
==13821==    by 0x43B67BE: JSC::JSValue::toString(JSC::ExecState*) const (JSString.h:595)
==13821==    by 0x51578C8: JSC::jsAddSlowCase(JSC::ExecState*, JSC::JSValue, JSC::JSValue) (Operations.cpp:56)
==13821==    by 0x50B1482: cti_op_add (JITStubs.cpp:1342)
==13821==    by 0x50B0B4F: JSC::JITThunks::tryCacheGetByID(JSC::ExecState*, JSC::CodeBlock*, JSC::ReturnAddressPtr, JSC::JSValue, JSC::Identifier const&, JS$
==13821==    by 0x5080B09: JSC::JITCode::execute(JSC::RegisterFile*, JSC::ExecState*, JSC::JSGlobalData*) (JITCode.h:77)
==13821==    by 0x507D8DE: JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::ScopeChainNode*, JSC::JSObject*) (Interpreter.cpp:778)
==13821==    by 0x5113D96: JSC::evaluate(JSC::ExecState*, JSC::ScopeChain&, JSC::SourceCode const&, JSC::JSValue) (Completion.cpp:62)
==13821==    by 0x43FDE84: WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::ScopeChain&, JSC::SourceCode const&, JSC::JSValue) (JSMainThreadEx$
==13821==    by 0x441C67C: WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld*, WebCore::ShouldAllowXSS) $
==13821==    by 0x441C816: WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&, WebCore::ShouldAllowXSS) (ScriptController.cpp:171)
==13821==    by 0x44459EB: WebCore::ScriptController::executeScript(WebCore::ScriptSourceCode const&, WebCore::ShouldAllowXSS) (ScriptControllerBase.cpp:60)
==13821==  Uninitialised value was created by a heap allocation
==13821==    at 0x4025BDC: malloc (vg_replace_malloc.c:195)
==13821==    by 0x517F063: WTF::fastMalloc(unsigned int) (FastMalloc.cpp:250)
==13821==    by 0x4378915: WTF::FastAllocBase::operator new(unsigned int) (FastAllocBase.h:121)
==13821==    by 0x5135C1F: JSC::JSGlobalData::create(JSC::ThreadStackType) (JSGlobalData.cpp:237)
==13821==    by 0x5135C8C: JSC::JSGlobalData::createLeaked(JSC::ThreadStackType) (JSGlobalData.cpp:243)
==13821==    by 0x43DFA15: WebCore::JSDOMWindowBase::commonJSGlobalData() (JSDOMWindowBase.cpp:185)
==13821==    by 0x441C888: WebCore::ScriptController::getAllWorlds(WTF::Vector<WebCore::DOMWrapperWorld*, 0u>&) (ScriptController.cpp:181)
==13821==    by 0x4824691: WebCore::FrameLoader::dispatchDidClearWindowObjectsInAllWorlds() (FrameLoader.cpp:3350)
==13821==    by 0x481933E: WebCore::FrameLoader::receivedFirstData() (FrameLoader.cpp:618)
==13821==    by 0x481B21F: WebCore::FrameLoader::willSetEncoding() (FrameLoader.cpp:1090)
==13821==    by 0x4814BC4: WebCore::DocumentWriter::setEncoding(WTF::String const&, bool) (DocumentWriter.cpp:236)
==13821==    by 0x480B35A: WebCore::DocumentLoader::commitData(char const*, int) (DocumentLoader.cpp:306)
Comment 1 Xan Lopez 2010-11-30 15:10:28 PST
Created attachment 75213 [details]
cacheentryinit.diff

Not sure if this is the proper way to fix it, but couldn't really think of anything else.
Comment 2 Darin Adler 2010-11-30 16:15:17 PST
Comment on attachment 75213 [details]
cacheentryinit.diff

View in context: https://bugs.webkit.org/attachment.cgi?id=75213&action=review

This patch doesn't fix a bug. There is no symptom. While the key has a random value in it, the value is guaranteed to have a null string, and the code always checks that.

I suspect this patch just makes a memory debugging tool stop complaining. Is there some other way to accomplish that without generating a lot of code that will run whenever we construct a JSGlobalData object?

> JavaScriptCore/runtime/NumericStrings.h:81
> +            CacheEntry()
> +            : key(0)
> +            {
> +            }

Initializing to zero is not all that helpful; zero itself has a cached string.
Comment 3 Xan Lopez 2010-11-30 16:27:02 PST
(In reply to comment #2)
> (From update of attachment 75213 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75213&action=review
> 
> This patch doesn't fix a bug. There is no symptom. While the key has a random value in it, the value is guaranteed to have a null string, and the code always checks that.
> 
> I suspect this patch just makes a memory debugging tool stop complaining. Is there some other way to accomplish that without generating a lot of code that will run whenever we construct a JSGlobalData object?

It's true that basically this can never result in any problem as the code is now, since the UString will have a null value at first and the code checks for that. It's just probably a good idea to shut up valgrind and protect ourselves against future refactorings where this could become an issue.

> 
> > JavaScriptCore/runtime/NumericStrings.h:81
> > +            CacheEntry()
> > +            : key(0)
> > +            {
> > +            }
> 
> Initializing to zero is not all that helpful; zero itself has a cached string.

Right, but because of the UString check in the if things will still work properly (the first time it will introduced, after that it will be returned from the cache). In any case as I said I realize this is not ideal.
Comment 4 Darin Adler 2010-11-30 16:29:11 PST
(In reply to comment #3)
> It's just probably a good idea to shut up valgrind

I guess so.

> protect ourselves against future refactorings where this could become an issue.

I don’t think it really does protect us, because the initial value of 0 is not necessarily good after future refactoring either.

I’d like to hear what the original author thinks.
Comment 5 Darin Adler 2010-11-30 16:30:10 PST
If the goal was just to quiet valgrind, we could just reverse the if statements and check the string for null before looking at the hash table key. That would make cache misses slower, though, so not sure it’s worth it.
Comment 6 Geoffrey Garen 2010-11-30 17:15:55 PST
I think initializing key to 0 is misleading because it implies that 0 is a special sentry/invalid value, when it isn't. More work when initializing JSGlobalData is also not great, although it's not a huge concern, since creating JSGlobalData is relatively rare.

I'm not too excited about making cache misses slower just to make a tool happy. That said, if SunSpider and v8 don't complain about the change, it's probably OK.

Is there any way to fix this problem with valgrind-only annotations or code?
Comment 7 Xan Lopez 2010-11-30 17:26:53 PST
(In reply to comment #6)
> Is there any way to fix this problem with valgrind-only annotations or code?

Tomorrow I'll see if swapping the order of checks in the if is a noticeable perf hit, since as Daring suggested that should make valgrind happy too.
Comment 8 Xan Lopez 2010-12-01 04:24:54 PST
The results of reversing the if check order:

TEST                   COMPARISON            FROM                 TO             DETAILS

=============================================================================

** TOTAL **:           ??                259.1ms +/- 0.5%   259.5ms +/- 0.5%     not conclusive: might be *1.002x as slow*

=============================================================================

  3d:                  ??                 35.3ms +/- 2.1%    35.4ms +/- 2.7%     not conclusive: might be *1.003x as slow*
    cube:              -                  12.6ms +/- 5.6%    12.2ms +/- 7.1% 
    morph:             ??                 10.2ms +/- 1.2%    10.3ms +/- 1.9%     not conclusive: might be *1.016x as slow*
    raytrace:          *1.022x as slow*   12.5ms +/- 1.4%    12.8ms +/- 1.6%     significant

  access:              ??                 36.6ms +/- 0.9%    36.8ms +/- 1.0%     not conclusive: might be *1.006x as slow*
    binary-trees:      ??                  3.3ms +/- 3.9%     3.3ms +/- 4.0%     not conclusive: might be *1.012x as slow*
    fannkuch:          ??                 18.1ms +/- 0.8%    18.3ms +/- 0.7%     not conclusive: might be *1.008x as slow*
    nbody:             ??                 10.2ms +/- 1.1%    10.2ms +/- 1.2%     not conclusive: might be *1.004x as slow*
    nsieve:            -                   5.0ms +/- 0.0%     5.0ms +/- 0.0% 

  bitops:              -                  23.1ms +/- 0.4%    23.2ms +/- 0.5% 
    3bit-bits-in-byte: -                   3.0ms +/- 0.0%     3.0ms +/- 0.0% 
    bits-in-byte:      ??                  6.0ms +/- 0.0%     6.0ms +/- 0.9%     not conclusive: might be *1.007x as slow*
    bitwise-and:       -                   5.1ms +/- 1.9%     5.1ms +/- 1.8% 
    nsieve-bits:       -                   9.0ms +/- 0.0%     9.0ms +/- 0.0% 

  controlflow:         -                   2.0ms +/- 0.0%     2.0ms +/- 0.0% 
    recursive:         -                   2.0ms +/- 0.0%     2.0ms +/- 0.0% 

  crypto:              -                  15.0ms +/- 0.4%    15.0ms +/- 0.3% 
    aes:               -                   9.0ms +/- 0.6%     9.0ms +/- 0.4% 
    md5:               -                   3.0ms +/- 0.0%     3.0ms +/- 0.0% 
    sha1:              -                   3.0ms +/- 0.0%     3.0ms +/- 0.0% 

  date:                ??                 29.1ms +/- 0.5%    29.4ms +/- 1.2%     not conclusive: might be *1.010x as slow*
    format-tofte:      ??                 15.0ms +/- 0.4%    15.2ms +/- 1.9%     not conclusive: might be *1.009x as slow*
    format-xparb:      ??                 14.1ms +/- 1.0%    14.2ms +/- 1.1%     not conclusive: might be *1.010x as slow*

  math:                ??                 26.1ms +/- 0.5%    26.2ms +/- 0.8%     not conclusive: might be *1.004x as slow*
    cordic:            ??                  8.0ms +/- 0.5%     8.0ms +/- 0.7%     not conclusive: might be *1.002x as slow*
    partial-sums:      ??                 14.0ms +/- 0.3%    14.1ms +/- 0.6%     not conclusive: might be *1.003x as slow*
    spectral-norm:     ??                  4.0ms +/- 1.0%     4.1ms +/- 1.7%     not conclusive: might be *1.010x as slow*

  regexp:              ??                 11.3ms +/- 1.1%    11.4ms +/- 2.3%     not conclusive: might be *1.009x as slow*
    dna:               ??                 11.3ms +/- 1.1%    11.4ms +/- 2.3%     not conclusive: might be *1.009x as slow*

  string:              -                  80.6ms +/- 0.6%    80.3ms +/- 0.4% 
    base64:            ??                  9.0ms +/- 0.0%     9.0ms +/- 0.4%     not conclusive: might be *1.002x as slow*
    fasta:             -                  11.0ms +/- 0.4%    11.0ms +/- 0.4% 
    tagcloud:          -                  20.3ms +/- 1.1%    20.2ms +/- 0.7% 
    unpack-code:       1.010x as fast     29.9ms +/- 0.7%    29.6ms +/- 0.6%     significant
    validate-input:    -                  10.4ms +/- 1.5%    10.4ms +/- 1.5%
Comment 9 Xan Lopez 2010-12-01 04:25:47 PST
Comment on attachment 75213 [details]
cacheentryinit.diff

Removing from the review queue.