223896 2021-03-29 13:18:09 -0700 UBSan: JSC::Parser<LexerType>::parseProperty(): runtime error: load of value nnn, which is not a valid value for type 'bool' 2021-03-31 19:55:14 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=176131 InRadar P2 Normal --- 1 ddkilzer ddkilzer darin ews-watchlist keith_miller mark.lam msaboff saam tzagallo webkit-bug-importer oldest_to_newest 1744855 0 ddkilzer 2021-03-29 13:18:09 -0700 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; }; }; 1744873 1 ddkilzer 2021-03-29 13:26:39 -0700 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.) 1744875 2 webkit-bug-importer 2021-03-29 13:28:30 -0700 <rdar://problem/75970132> 1745004 3 darin 2021-03-29 18:21:35 -0700 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. 1745005 4 darin 2021-03-29 18:23:38 -0700 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. 1745763 5 424816 ddkilzer 2021-03-31 13:51:38 -0700 Created attachment 424816 Patch v1 1745767 6 ddkilzer 2021-03-31 14:00:16 -0700 (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. 1745932 7 ews-feeder 2021-03-31 19:55:12 -0700 Committed r275343: <https://commits.webkit.org/r275343> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424816. 424816 2021-03-31 13:51:38 -0700 2021-03-31 19:55:13 -0700 Patch v1 bug-223896-20210331135137.patch text/plain 2639 ddkilzer U3VidmVyc2lvbiBSZXZpc2lvbjogMjc1MTM3CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBl MGU2Njc0MDA3ZTAyOWIxNzU2MTU5Y2VlMjk1NDZjYWYzYzIwNDJmLi44NmE3MjMyOThjZTE1NDNi MTU0OGYwOTc5NGUyNzZmODU5OWQyYmZlIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwyMSBAQAorMjAyMS0wMy0zMSAgRGF2aWQgS2lsemVyICA8ZGRraWx6ZXJAYXBwbGUuY29t PgorCisgICAgICAgIFVCU2FuOiBKU0M6OlBhcnNlcjxMZXhlclR5cGU+OjpwYXJzZVByb3BlcnR5 KCk6IHJ1bnRpbWUgZXJyb3I6IGxvYWQgb2YgdmFsdWUgbm5uLCB3aGljaCBpcyBub3QgYSB2YWxp ZCB2YWx1ZSBmb3IgdHlwZSAnYm9vbCcKKyAgICAgICAgPGh0dHBzOi8vd2Via2l0Lm9yZy9iLzIy Mzg5Nj4KKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzc1OTcwMTMyPgorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEJhc2VkIG9uIGEgc3VnZ2VzdGlvbiBi eSBEYXJpbiBBZGxlci4KKworICAgICAgICAqIHBhcnNlci9QYXJzZXIuY3BwOgorICAgICAgICAo SlNDOjpQYXJzZXI8TGV4ZXJUeXBlPjo6cGFyc2VQcm9wZXJ0eSk6CisgICAgICAgIC0gQ2hhbmdl ICdlc2NhcGVkJyB0byAnd2FzVW5lc2NhcGVkSWRlbnQnIHRvIGF2b2lkIHRoZSB1bmRlZmluZWQK KyAgICAgICAgICBiZWhhdmlvciBzaW5jZSBtX3Rva2VuLm1fZGF0YS5lc2NhcGVkIGlzIG9ubHkg c2V0IGluIHRoZSBjYXNlCisgICAgICAgICAgd2hlbiBhbiBpZGVudGlmZXIgaXMgcGFyc2VkIChp biBMZXhlcjw+OjpwYXJzZUlkZW50aWZlcigpKSwKKyAgICAgICAgICBub3QgYSBzdHJpbmcgKGlu IExleGVyPD46OnBhcnNlU3RyaW5nKCkpLiBUaGlzIHNpbXBsaWZpZXMgdGhlCisgICAgICAgICAg bG9naWMgbGF0ZXIgaW4gdGhlIG1ldGhvZC4KKwogMjAyMS0wMy0yNiAgWXVzdWtlIFN1enVraSAg PHlzdXp1a2lAYXBwbGUuY29tPgogCiAgICAgICAgIFtKU0NdIFVzZSBBcHBsZUlDVSBTUEkgZm9y IGNhbm9uaWNhbGl6YXRpb24KZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9wYXJz ZXIvUGFyc2VyLmNwcCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9wYXJzZXIvUGFyc2VyLmNwcApp bmRleCBlNzYxYmZhYTE4ZTRhZTdiZmI1ZDE3ZDMyZDVhNzQyMGI3ODI5MGU0Li4xYTk4NzgyOTM5 NzJhYzAyODJhZTQ0NWU4YmI5MTU4MjY1ZjRhNjY2IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNj cmlwdENvcmUvcGFyc2VyL1BhcnNlci5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3Bh cnNlci9QYXJzZXIuY3BwCkBAIC00MzM2LDExICs0MzM2LDExIEBAIHBhcnNlUHJvcGVydHk6CiAg ICAgY2FzZSBTVFJJTkc6IHsKIG5hbWVkUHJvcGVydHk6CiAgICAgICAgIGNvbnN0IElkZW50aWZp ZXIqIGlkZW50ID0gbV90b2tlbi5tX2RhdGEuaWRlbnQ7Ci0gICAgICAgIGJvb2wgZXNjYXBlZCA9 IG1fdG9rZW4ubV9kYXRhLmVzY2FwZWQ7CisgICAgICAgIGJvb2wgd2FzVW5lc2NhcGVkSWRlbnQg PSB3YXNJZGVudCAmJiAhbV90b2tlbi5tX2RhdGEuZXNjYXBlZDsKICAgICAgICAgdW5zaWduZWQg Z2V0dGVyT3JTZXR0ZXJTdGFydE9mZnNldCA9IHRva2VuU3RhcnQoKTsKICAgICAgICAgSlNUb2tl biBpZGVudFRva2VuID0gbV90b2tlbjsKIAotICAgICAgICBpZiAod2FzSWRlbnQgJiYgIWlzR2Vu ZXJhdG9yTWV0aG9kUGFyc2VNb2RlKHBhcnNlTW9kZSkgJiYgKCFlc2NhcGVkICYmICgqaWRlbnQg PT0gbV92bS5wcm9wZXJ0eU5hbWVzLT5nZXQgfHwgKmlkZW50ID09IG1fdm0ucHJvcGVydHlOYW1l cy0+c2V0KSkpCisgICAgICAgIGlmICh3YXNVbmVzY2FwZWRJZGVudCAmJiAhaXNHZW5lcmF0b3JN ZXRob2RQYXJzZU1vZGUocGFyc2VNb2RlKSAmJiAoKmlkZW50ID09IG1fdm0ucHJvcGVydHlOYW1l cy0+Z2V0IHx8ICppZGVudCA9PSBtX3ZtLnByb3BlcnR5TmFtZXMtPnNldCkpCiAgICAgICAgICAg ICBuZXh0RXhwZWN0SWRlbnRpZmllcihMZXhlckZsYWdzOjpJZ25vcmVSZXNlcnZlZFdvcmRzKTsK ICAgICAgICAgZWxzZQogICAgICAgICAgICAgbmV4dEV4cGVjdElkZW50aWZpZXIoVHJlZUJ1aWxk ZXI6OkRvbnRCdWlsZEtleXdvcmRzIHwgTGV4ZXJGbGFnczo6SWdub3JlUmVzZXJ2ZWRXb3Jkcyk7 CkBAIC00Mzc5LDcgKzQzNzksNyBAQCBuYW1lZFByb3BlcnR5OgogICAgICAgICAgICAgY2xhc3Np ZnlFeHByZXNzaW9uRXJyb3IoRXJyb3JJbmRpY2F0ZXNQYXR0ZXJuKTsKIAogICAgICAgICBPcHRp b25hbDxQcm9wZXJ0eU5vZGU6OlR5cGU+IHR5cGU7Ci0gICAgICAgIGlmICghZXNjYXBlZCkgewor ICAgICAgICBpZiAod2FzVW5lc2NhcGVkSWRlbnQpIHsKICAgICAgICAgICAgIGlmICgqaWRlbnQg PT0gbV92bS5wcm9wZXJ0eU5hbWVzLT5nZXQpCiAgICAgICAgICAgICAgICAgdHlwZSA9IFByb3Bl cnR5Tm9kZTo6R2V0dGVyOwogICAgICAgICAgICAgZWxzZSBpZiAoKmlkZW50ID09IG1fdm0ucHJv cGVydHlOYW1lcy0+c2V0KQo=