235348 2022-01-18 22:51:23 -0800 [JSC] Fix YarrJIT backtrackCharacterClassNonGreedy breakpoint 2022-01-19 11:53:54 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 ysuzuki ysuzuki darin ews-watchlist keith_miller mark.lam msaboff saam tzagallo webkit-bug-importer oldest_to_newest 1831946 0 ysuzuki 2022-01-18 22:51:23 -0800 [JSC] Fix YarrJIT backtrackCharacterClassNonGreedy breakpoint 1831947 1 449465 ysuzuki 2022-01-18 22:57:59 -0800 Created attachment 449465 Patch 1831948 2 ysuzuki 2022-01-18 22:58:18 -0800 rdar://84108897 1832045 3 449465 darin 2022-01-19 08:53:13 -0800 Comment on attachment 449465 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449465&action=review > Source/JavaScriptCore/yarr/YarrJIT.cpp:2062 > + m_jit.sub32(MacroAssembler::TrustedImm32(1), m_regs.index); Do we need anything different in the m_decodeSurrogatePairs case? In that case it might be incremented my more than one? 1832056 4 449465 ysuzuki 2022-01-19 09:37:09 -0800 Comment on attachment 449465 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449465&action=review >> Source/JavaScriptCore/yarr/YarrJIT.cpp:2062 >> + m_jit.sub32(MacroAssembler::TrustedImm32(1), m_regs.index); > > Do we need anything different in the m_decodeSurrogatePairs case? In that case it might be incremented my more than one? nonGreedyFailuresDecrementIndex is added only when m_decodeSurrogatePairs is true. So, this path will be taken only when m_decodeSurrogatePairs is true case. (nonGreedyFailuresDecrementIndex is added in L2052, which is executed when m_decodeSurrogatePairs is true). 1832058 5 449465 darin 2022-01-19 09:46:00 -0800 Comment on attachment 449465 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449465&action=review >>> Source/JavaScriptCore/yarr/YarrJIT.cpp:2062 >>> + m_jit.sub32(MacroAssembler::TrustedImm32(1), m_regs.index); >> >> Do we need anything different in the m_decodeSurrogatePairs case? In that case it might be incremented my more than one? > > nonGreedyFailuresDecrementIndex is added only when m_decodeSurrogatePairs is true. So, this path will be taken only when m_decodeSurrogatePairs is true case. (nonGreedyFailuresDecrementIndex is added in L2052, which is executed when m_decodeSurrogatePairs is true). OK, maybe I can’t review then, because I can’t spot the add32 that this subtract is reversing, so I suppose you need a review from a subject matter expert. Sorry, I was hoping I could save a little time by being your reviewer! 1832081 6 449465 ysuzuki 2022-01-19 10:39:20 -0800 Comment on attachment 449465 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449465&action=review >>>> Source/JavaScriptCore/yarr/YarrJIT.cpp:2062 >>>> + m_jit.sub32(MacroAssembler::TrustedImm32(1), m_regs.index); >>> >>> Do we need anything different in the m_decodeSurrogatePairs case? In that case it might be incremented my more than one? >> >> nonGreedyFailuresDecrementIndex is added only when m_decodeSurrogatePairs is true. So, this path will be taken only when m_decodeSurrogatePairs is true case. (nonGreedyFailuresDecrementIndex is added in L2052, which is executed when m_decodeSurrogatePairs is true). > > OK, maybe I can’t review then, because I can’t spot the add32 that this subtract is reversing, so I suppose you need a review from a subject matter expert. Sorry, I was hoping I could save a little time by being your reviewer! np!!!!! Thank you for your review :D 1832102 7 449465 msaboff 2022-01-19 11:22:46 -0800 Comment on attachment 449465 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449465&action=review r=me >>>>> Source/JavaScriptCore/yarr/YarrJIT.cpp:2062 >>>>> + m_jit.sub32(MacroAssembler::TrustedImm32(1), m_regs.index); >>>> >>>> Do we need anything different in the m_decodeSurrogatePairs case? In that case it might be incremented my more than one? >>> >>> nonGreedyFailuresDecrementIndex is added only when m_decodeSurrogatePairs is true. So, this path will be taken only when m_decodeSurrogatePairs is true case. (nonGreedyFailuresDecrementIndex is added in L2052, which is executed when m_decodeSurrogatePairs is true). >> >> OK, maybe I can’t review then, because I can’t spot the add32 that this subtract is reversing, so I suppose you need a review from a subject matter expert. Sorry, I was hoping I could save a little time by being your reviewer! > > np!!!!! Thank you for your review :D This is safe and appropriate because we only get in this case when the following is true (from reading L2050-2063 and L516-520): 1) We are in the m_decodeSurrogatePairs case. 2) We are trying to match a non-BMP character. 3) We are at the end of the string input with the first of the two characters, i.e. we have incremented 1. 4) There we can't advance the index for the second character we can't read. Therefore we need to decrement index by 1 and fall through to the failed non-greedy match. 1832106 8 449465 ysuzuki 2022-01-19 11:27:12 -0800 Comment on attachment 449465 Patch Thanks! 1832125 9 ews-feeder 2022-01-19 11:53:50 -0800 Committed r288224 (246184@main): <https://commits.webkit.org/246184@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449465. 449465 2022-01-18 22:57:59 -0800 2022-01-19 11:53:53 -0800 Patch bug-235348-20220118225758.patch text/plain 2877 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMjg4MTY0CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBm NmEzMDhkN2MzZDEzMzg5YjgyNDA1YzJhNjE5YzBhZDc2ZmM5MjU4Li5iODA1NDJiYWQwZGM2MWQ2 MTgyNDAyOWM4MGI5OTRmNTMwZWUzNzE5IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNSBAQAorMjAyMi0wMS0xOCAgWXVzdWtlIFN1enVraSAgPHlzdXp1a2lAYXBwbGUuY29t PgorCisgICAgICAgIFtKU0NdIEZpeCBZYXJySklUIGJhY2t0cmFja0NoYXJhY3RlckNsYXNzTm9u R3JlZWR5IGJyZWFrcG9pbnQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTIzNTM0OAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgor CisgICAgICAgIFlhcnJKSVQncyBiYWNrdHJhY2tDaGFyYWN0ZXJDbGFzc05vbkdyZWVkeSBicmVh a3BvaW50IGlzIGFjdHVhbGx5IHJlYWNoYWJsZS4gV2Ugc2hvdWxkIHN1YnRyYWN0IGluZGV4IChz aW5jZSBpdCBpcyBhbHJlYWR5IGluY3JlbWVudGVkKSwKKyAgICAgICAgYW5kIGdvIHRvIHRoZSBu b3JtYWwgbm9uR3JlZWR5RmFpbHVyZXMgcGF0aC4KKworICAgICAgICAqIHlhcnIvWWFyckpJVC5j cHA6CisKIDIwMjItMDEtMTggIFl1c3VrZSBTdXp1a2kgIDx5c3V6dWtpQGFwcGxlLmNvbT4KIAog ICAgICAgICBbSlNDXSBSZW1vdmUgV2FzbTo6VGFibGUncyBtX21hc2sKZGlmZiAtLWdpdCBhL1Nv dXJjZS9KYXZhU2NyaXB0Q29yZS95YXJyL1lhcnJKSVQuY3BwIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL3lhcnIvWWFyckpJVC5jcHAKaW5kZXggYzhlYjcyYzc0ODdkNmVhNGJlZDM5ZjM0YjE0NWYz OGQwMTQ3ZDMwOC4uNTFmZjRhNzFkOTY5NjViMzgxYzhkNmFiNmVhN2JkZjQ4NDQ5MjFkYSAxMDA2 NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3JlL3lhcnIvWWFyckpJVC5jcHAKKysrIGIvU291 cmNlL0phdmFTY3JpcHRDb3JlL3lhcnIvWWFyckpJVC5jcHAKQEAgLTIwNTksNyArMjA1OSw3IEBA IGNsYXNzIFlhcnJHZW5lcmF0b3IgZmluYWwgOiBwdWJsaWMgWWFyckpJVEluZm8gewogCiAgICAg ICAgIGlmICghbm9uR3JlZWR5RmFpbHVyZXNEZWNyZW1lbnRJbmRleC5lbXB0eSgpKSB7CiAgICAg ICAgICAgICBub25HcmVlZHlGYWlsdXJlc0RlY3JlbWVudEluZGV4LmxpbmsoJm1faml0KTsKLSAg ICAgICAgICAgIG1faml0LmJyZWFrcG9pbnQoKTsKKyAgICAgICAgICAgIG1faml0LnN1YjMyKE1h Y3JvQXNzZW1ibGVyOjpUcnVzdGVkSW1tMzIoMSksIG1fcmVncy5pbmRleCk7CiAgICAgICAgIH0K ICAgICAgICAgbm9uR3JlZWR5RmFpbHVyZXMubGluaygmbV9qaXQpOwogICAgICAgICBtX2ppdC5z dWIzMihjb3VudFJlZ2lzdGVyLCBtX3JlZ3MuaW5kZXgpOwpkaWZmIC0tZ2l0IGEvSlNUZXN0cy9D aGFuZ2VMb2cgYi9KU1Rlc3RzL0NoYW5nZUxvZwppbmRleCAzYzc3MmM5NjY0ZDY5ZGJhMDI2MDUw OWIyNWEwY2NkYzgxMjQzNGJmLi43NjdkMGExMDY3M2FkNmRmMjQyMTQyMjBmZmFiOTQ4NDgyN2Y0 YzhiIDEwMDY0NAotLS0gYS9KU1Rlc3RzL0NoYW5nZUxvZworKysgYi9KU1Rlc3RzL0NoYW5nZUxv ZwpAQCAtMSwzICsxLDEyIEBACisyMDIyLTAxLTE4ICBZdXN1a2UgU3V6dWtpICA8eXN1enVraUBh cHBsZS5jb20+CisKKyAgICAgICAgW0pTQ10gRml4IFlhcnJKSVQgYmFja3RyYWNrQ2hhcmFjdGVy Q2xhc3NOb25HcmVlZHkgYnJlYWtwb2ludAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9MjM1MzQ4CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgKiBzdHJlc3MvcmVnZXhwLXVuaWNvZGUtc3Vycm9nYXRlLXBhaXIt aW5jcmVtZW50LXNob3VsZC1pbnZvbHZlLWxlbmd0aC1jaGVjay5qczoKKwogMjAyMi0wMS0xOCAg TWFyayBMYW0gIDxtYXJrLmxhbUBhcHBsZS5jb20+CiAKICAgICAgICAgR2FyZGVuaW5nOiByZXNv bHZlIHRlc3QgZmFpbHVyZSBvbiBDTG9vcC4KZGlmZiAtLWdpdCBhL0pTVGVzdHMvc3RyZXNzL3Jl Z2V4cC11bmljb2RlLXN1cnJvZ2F0ZS1wYWlyLWluY3JlbWVudC1zaG91bGQtaW52b2x2ZS1sZW5n dGgtY2hlY2suanMgYi9KU1Rlc3RzL3N0cmVzcy9yZWdleHAtdW5pY29kZS1zdXJyb2dhdGUtcGFp ci1pbmNyZW1lbnQtc2hvdWxkLWludm9sdmUtbGVuZ3RoLWNoZWNrLmpzCmluZGV4IDBmOTgzYjEw OWFjNDUzN2QwMGY4MmQ4YTFmNTkwYmM3ZDM1MzE4NGQuLjQ5Mzg4YmY5MWJlNGRiOTYzNjBiNGQx YjIyZDVlZmRkYTRiNmRjYzIgMTAwNjQ0Ci0tLSBhL0pTVGVzdHMvc3RyZXNzL3JlZ2V4cC11bmlj b2RlLXN1cnJvZ2F0ZS1wYWlyLWluY3JlbWVudC1zaG91bGQtaW52b2x2ZS1sZW5ndGgtY2hlY2su anMKKysrIGIvSlNUZXN0cy9zdHJlc3MvcmVnZXhwLXVuaWNvZGUtc3Vycm9nYXRlLXBhaXItaW5j cmVtZW50LXNob3VsZC1pbnZvbHZlLWxlbmd0aC1jaGVjay5qcwpAQCAtMzYsMyArMzYsNyBAQCBm dW5jdGlvbiB0ZXN0UmVnRXhwTm90TWF0Y2gocmUsIHN0cikKIGxldCBiYWNrdHJhY2tHcmVlZHlS ZWdFeHAgPSAvLipbXHgyMC1cdWRmZmZmXS5cdyouXGR7M30vdTsKIAogdGVzdFJlZ0V4cE1hdGNo KGJhY2t0cmFja0dyZWVkeVJlZ0V4cCwgdGVzdFN0cmluZzIpOworCitsZXQgbm9uR3JlZWR5UmVn RXhwMiA9IC8oLipbXnhdKz8pW15dKihbMV0pL3U7CisKK3Rlc3RSZWdFeHBOb3RNYXRjaChub25H cmVlZHlSZWdFeHAyLCB0ZXN0U3RyaW5nKTsK