Bug 223896 – UBSan: JSC::Parser<LexerType>::parseProperty(): runtime error: load of value nnn, which is not a valid value for type 'bool'

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[David Kilzer (:ddkilzer)]

Running all layout tests with a Release+UBSan build of WebKit (see Bug 176131) results in ~951 tests hitting this UBSan warning at least once with different values of "nnn":

    parser/Parser.cpp:4339:39: runtime error: load of value nnn, which is not a valid value for type 'bool'

Note that "struct JSToken" attempts to initialize "union JSTokenData" in its definition in PaserTokens.h:

    struct JSToken {
        JSTokenType m_type { ERRORTOK };
        JSTokenData m_data { { nullptr, nullptr, false } };
        JSTokenLocation m_location;
        JSTextPosition m_startPosition;
        JSTextPosition m_endPosition;
    
        void dump(WTF::PrintStream&) const;
    };

However, it seems the `escaped` field may not be initialized somewhere else the JSTokenData's m_type is set to JSTokenType::STRING.

    union JSTokenData {
        struct {
            const Identifier* cooked;
            const Identifier* raw;
            bool isTail;
        };
        struct {
            uint32_t line;
            uint32_t offset;
            uint32_t lineStartOffset;
        };
        double doubleValue;
        struct {
            const Identifier* ident;
            bool escaped;
        };
        struct {
            const Identifier* bigIntString;
            uint8_t radix;
        };
        struct {
            const Identifier* pattern;
            const Identifier* flags;
        };
    };

[David Kilzer (:ddkilzer)]

The line of code with the undefined behavior is:

    [...]
    case STRING: {
namedProperty:
        const Identifier* ident = m_token.m_data.ident;
        bool escaped = m_token.m_data.escaped;  // UBSan runtime error.
    [...]

I have been unable to find the place where `ident` is set but not `escaped` using some basic text searches in the JavaScriptCore project.  (I'm not really familiar with the parser code.)

[Radar WebKit Bug Importer]

<rdar://problem/75970132>

[Darin Adler]

The issue is that parseString does not set the escaped flag, but parseIdentifier does. When the token is an actual STRING, the escaped flag is uninitialized. Only when wasIdent is set to true and we either fall through to this case or use "goto namedProperty" to join the code in this case is the escaped flag properly initialized.

The code doesn’t actually look at the escaped flag unless wasIdent is true, so the issue is really how the code is structured. The smallest possible fix is probably this:

    bool escaped = wasIdent && m_token.m_data.escaped;

But there are probably many other ways to rearrange the code to avoid the reliance on undefined behavior.

[Darin Adler]

Another possibility is to write:

    bool wasUnescapedIdent = wasIdent && !m_token.m_data.escaped;

This would make the logic in the two places we check the "escaped" flag slightly easier to understand. In both cases we are just checking for "unescaped get" and "unescaped set", so wasUnescapedIdent is a better building block than the escaped flag.

[David Kilzer (:ddkilzer)]

Created attachment 424816 [details]
Patch v1

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #5)
> Created attachment 424816 [details]
> Patch v1

Should also note that this is covered by 951 layout tests which warn about this specific UBSan issue.

[EWS]

Committed r275343: <https://commits.webkit.org/r275343>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424816 [details].