Summary: | UBSan: JSC::Parser<LexerType>::parseProperty(): runtime error: load of value nnn, which is not a valid value for type 'bool' | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
Component: | JavaScriptCore | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | darin, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=176131 | ||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2021-03-29 13:18:09 PDT
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.) 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. 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. Created attachment 424816 [details]
Patch v1
(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. Committed r275343: <https://commits.webkit.org/r275343> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424816 [details]. |