41862 2010-07-08 08:43:01 -0700 [Arm] Missing NaN check in XPath substring function 2010-07-09 07:54:40 -0700 1 1 1 Unclassified WebKit XML 528+ (Nightly build) Android Android RESOLVED FIXED P2 Normal --- 1 benm benm android-webkit-unforking oldest_to_newest 248227 0 benm 2010-07-08 08:43:01 -0700 On ARM, conversion from NaN to an integer type yields 0 rather than MIN_INT (as it seems to do on other platforms). This exposes a bug in XPathFunctions.cpp, specifically the FunSubstring::evaluate function. Here we cast a potentially NaN value to a long but do not check for NaN first. The bug is not exposed currently on non-ARM platforms as the negative result of the cast means we early out further on in the function. We have other cases of explicitly detecting NaN in this function so I propose we add an extra one to cover this case. The existing XPath layout tests should suffice - with this patch Android now passes all tests in fast/xpath. Hooray! 248238 1 60892 benm 2010-07-08 08:57:24 -0700 Created attachment 60892 Proposed patch. 248598 2 60892 steveblock 2010-07-08 15:45:29 -0700 Comment on attachment 60892 Proposed patch. Does the test that's failing on Android explicitly check that a value of NaN for the position argument yields the empty string? Are we sure this is the desired behaviour? Assuming that returning the empty string is the correct behaviour ... > Here we cast a potentially NaN value to a long but do not check for NaN first. The bug is not exposed > currently on non-ARM platforms as the negative result of the cast means we early out further on in the > function. On non-ARM platforms we only early-out and return the empty string if a length argument is supplied (and if pos <= 1 - MIN_INT). If a length argument is not supplied, we call String::substring(MIN_INT - 1, -1). However, I guess this would almost certainly return the empty string too because String::substring() takes two unsigned ints, so we end up calling String::substring(MAX_UINT + MIN_INT, MAX_UINT) and the value of the position argument will exceed the length of the string. This seems wrong, so I think the proposed fix is a good idea for non-ARM platforms as well as ARM platforms. Do the existing tests test this both with and without a length argument? I think you could write layout tests that would cause the existing function to fail if you used a very large value for the length argument or a very long string. Perhaps we should add some? Also, looking at this function, there seem to be other problems with how negative values for the position argument are handled. I've filed Bug 41913 to track this. 248940 3 benm 2010-07-09 05:40:01 -0700 (In reply to comment #2) > (From update of attachment 60892 [details]) > Does the test that's failing on Android explicitly check that a value of NaN for the position argument yields the empty string? Are we sure this is the desired behaviour? > There's already a layout test that specifies empty string with a NaN position and it specifies the result is the empty string. I think it makes sense. > On non-ARM platforms we only early-out and return the empty string if a length argument is supplied (and if pos <= 1 - MIN_INT). If a length argument is not supplied, we call String::substring(MIN_INT - 1, -1). However, I guess this would almost certainly return the empty string too because String::substring() takes two unsigned ints, so we end up calling String::substring(MAX_UINT + MIN_INT, MAX_UINT) and the value of the position argument will exceed the length of the string. This seems wrong, so I think the proposed fix is a good idea for non-ARM platforms as well as ARM platforms. Right. >Do the existing tests test this both with and without a length argument? I think you could write layout tests that would cause the existing function to fail if you used a very large value for the length argument or a very long string. Perhaps we should add some? > You're right, the existing layout tests don't cover the case where we have a NaN position and a (very negative) length that when passed through the FunSubString::evaluate function overflows and wraps around positively. I can add a new test for this case, and the patch I propose for the function fixes it. > Also, looking at this function, there seem to be other problems with how negative values for the position argument are handled. I've filed Bug 41913 to track this. Indeed, good spot! This one is actually very easy to recreate and fix but let's continue discussing it in the other bug. New patch with a layout test on the way. 248945 4 61037 benm 2010-07-09 05:54:52 -0700 Created attachment 61037 Patch and layout test. 248958 5 61037 steveblock 2010-07-09 06:34:11 -0700 Comment on attachment 61037 Patch and layout test. LayoutTests/fast/xpath/substring-nan-position-expected.txt:5 + PASS document.evaluate("substring('12345', number('NaN'), -2147483645)", document, null, XPathResult.STRING_TYPE, null).stringValue is '' I think you should add a comment to the ChangeLog to make clear that only this particular test is fixed by the patch. The others just cover other edge cases that already pass. It would also be good to add a brief comment here explaining what the magic number is. WebCore/ChangeLog:13 + argument and early out if the value is indeed NaN. 'early out and return the empty string' 248964 6 61037 benm 2010-07-09 06:54:49 -0700 Comment on attachment 61037 Patch and layout test. Thanks Steve. Will update the changelogs on landing. 249014 7 benm 2010-07-09 07:54:40 -0700 Sending LayoutTests/ChangeLog Adding LayoutTests/fast/xpath/substring-nan-position-expected.txt Adding LayoutTests/fast/xpath/substring-nan-position.html Sending WebCore/ChangeLog Sending WebCore/xml/XPathFunctions.cpp Transmitting file data ..... Committed revision 62952. 60892 2010-07-08 08:57:24 -0700 2010-07-09 05:54:52 -0700 Proposed patch. 41862.txt text/plain 1496 benm SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA2Mjc4OSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTggQEAKKzIwMTAtMDctMDggIEJlbiBNdXJkb2NoICA8YmVubUBnb29nbGUuY29t PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFtBcm1d IE1pc3NpbmcgTmFOIGNoZWNrIGluIFhQYXRoIHN1YnN0cmluZyBmdW5jdGlvbgorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDE4NjIKKworICAgICAgICBF eGlzdGluZyB0ZXN0cyBpbiBmYXN0L3hwYXRoIHN1ZmZpY2UgLSB0aGlzIGZpeCBtYWtlcworICAg ICAgICBBbmRyb2lkIHBhc3MgdGhlbSBhbGwhCisKKyAgICAgICAgKiB4bWwvWFBhdGhGdW5jdGlv bnMuY3BwOgorICAgICAgICAoV2ViQ29yZTo6WFBhdGg6OkZ1blN1YnN0cmluZzo6ZXZhbHVhdGUp OiBBZGQgYW4gaXNuYW4oKSBjaGVjaworICAgICAgICAgICAgdG8gdGhlIHZhbHVlIHJldHVybmVk IGZyb20gZXZhbHVhdGluZyB0aGUgcG9zaXRpb24gYXJndW1lbnQKKyAgICAgICAgICAgIGFuZCBl YXJseSBvdXQgaWYgdGhlIHZhbHVlIGlzIGluZGVlZCBOYU4uCisKIDIwMTAtMDctMDggIFhhbiBM b3BleiAgPHhsb3BlekBpZ2FsaWEuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEd1c3Rhdm8g Tm9yb25oYS4KSW5kZXg6IFdlYkNvcmUveG1sL1hQYXRoRnVuY3Rpb25zLmNwcAo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 Ci0tLSBXZWJDb3JlL3htbC9YUGF0aEZ1bmN0aW9ucy5jcHAJKHJldmlzaW9uIDYyNzg0KQorKysg V2ViQ29yZS94bWwvWFBhdGhGdW5jdGlvbnMuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC01MDAsNyAr NTAwLDEwIEBAIFZhbHVlIEZ1blN1YnN0cmluZ0FmdGVyOjpldmFsdWF0ZSgpIGNvbnMKIFZhbHVl IEZ1blN1YnN0cmluZzo6ZXZhbHVhdGUoKSBjb25zdAogewogICAgIFN0cmluZyBzID0gYXJnKDAp LT5ldmFsdWF0ZSgpLnRvU3RyaW5nKCk7Ci0gICAgbG9uZyBwb3MgPSBzdGF0aWNfY2FzdDxsb25n PihGdW5Sb3VuZDo6cm91bmQoYXJnKDEpLT5ldmFsdWF0ZSgpLnRvTnVtYmVyKCkpKTsKKyAgICBk b3VibGUgZG91YmxlUG9zID0gYXJnKDEpLT5ldmFsdWF0ZSgpLnRvTnVtYmVyKCk7CisgICAgaWYg KGlzbmFuKGRvdWJsZVBvcykpCisgICAgICAgIHJldHVybiAiIjsKKyAgICBsb25nIHBvcyA9IHN0 YXRpY19jYXN0PGxvbmc+KEZ1blJvdW5kOjpyb3VuZChkb3VibGVQb3MpKTsKICAgICBib29sIGhh dmVMZW5ndGggPSBhcmdDb3VudCgpID09IDM7CiAgICAgbG9uZyBsZW4gPSAtMTsKICAgICBpZiAo aGF2ZUxlbmd0aCkgewo= 61037 2010-07-09 05:54:52 -0700 2010-07-09 06:54:49 -0700 Patch and layout test. 41862.txt text/plain 4477 benm SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA2Mjk0NCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTcgQEAKKzIwMTAtMDctMDkgIEJlbiBNdXJkb2NoICA8YmVubUBnb29nbGUuY29t PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFtBcm1d IE1pc3NpbmcgTmFOIGNoZWNrIGluIFhQYXRoIHN1YnN0cmluZyBmdW5jdGlvbgorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDE4NjIKKworICAgICAgICBU ZXN0OiBmYXN0L3hwYXRoL3N1YnN0cmluZy1uYW4tcG9zaXRpb24uaHRtbAorCisgICAgICAgICog eG1sL1hQYXRoRnVuY3Rpb25zLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlhQYXRoOjpGdW5TdWJz dHJpbmc6OmV2YWx1YXRlKTogQWRkIGFuIGlzbmFuKCkKKyAgICAgICAgICAgIHRvIHRoZSB2YWx1 ZSByZXR1cm5lZCBmcm9tIGV2YWx1YXRpbmcgdGhlIHBvc2l0aW9uCisgICAgICAgICAgICBhcmd1 bWVudCBhbmQgZWFybHkgb3V0IGlmIHRoZSB2YWx1ZSBpcyBpbmRlZWQgTmFOLgorCiAyMDEwLTA3 LTA5ICBTaGVyaWZmIEJvdCAgPHdlYmtpdC5yZXZpZXcuYm90QGdtYWlsLmNvbT4KIAogICAgICAg ICBVbnJldmlld2VkLCByb2xsaW5nIG91dCByNjI5MzcuCkluZGV4OiBXZWJDb3JlL3htbC9YUGF0 aEZ1bmN0aW9ucy5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS94bWwvWFBhdGhGdW5jdGlvbnMu Y3BwCShyZXZpc2lvbiA2Mjk0NCkKKysrIFdlYkNvcmUveG1sL1hQYXRoRnVuY3Rpb25zLmNwcAko d29ya2luZyBjb3B5KQpAQCAtNTAwLDcgKzUwMCwxMCBAQCBWYWx1ZSBGdW5TdWJzdHJpbmdBZnRl cjo6ZXZhbHVhdGUoKSBjb25zCiBWYWx1ZSBGdW5TdWJzdHJpbmc6OmV2YWx1YXRlKCkgY29uc3QK IHsKICAgICBTdHJpbmcgcyA9IGFyZygwKS0+ZXZhbHVhdGUoKS50b1N0cmluZygpOwotICAgIGxv bmcgcG9zID0gc3RhdGljX2Nhc3Q8bG9uZz4oRnVuUm91bmQ6OnJvdW5kKGFyZygxKS0+ZXZhbHVh dGUoKS50b051bWJlcigpKSk7CisgICAgZG91YmxlIGRvdWJsZVBvcyA9IGFyZygxKS0+ZXZhbHVh dGUoKS50b051bWJlcigpOworICAgIGlmIChpc25hbihkb3VibGVQb3MpKQorICAgICAgICByZXR1 cm4gIiI7CisgICAgbG9uZyBwb3MgPSBzdGF0aWNfY2FzdDxsb25nPihGdW5Sb3VuZDo6cm91bmQo ZG91YmxlUG9zKSk7CiAgICAgYm9vbCBoYXZlTGVuZ3RoID0gYXJnQ291bnQoKSA9PSAzOwogICAg IGxvbmcgbGVuID0gLTE7CiAgICAgaWYgKGhhdmVMZW5ndGgpIHsKSW5kZXg6IExheW91dFRlc3Rz L0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9DaGFuZ2VMb2cJKHJldmlzaW9u IDYyOTQ0KQorKysgTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMg KzEsMTMgQEAKKzIwMTAtMDctMDkgIEJlbiBNdXJkb2NoICA8YmVubUBnb29nbGUuY29tPgorCisg ICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFtBcm1dIE1pc3Np bmcgTmFOIGNoZWNrIGluIFhQYXRoIHN1YnN0cmluZyBmdW5jdGlvbgorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NDE4NjIKKworICAgICAgICAqIGZhc3Qv eHBhdGgvc3Vic3RyaW5nLW5hbi1wb3NpdGlvbi1leHBlY3RlZC50eHQ6IEFkZGVkLgorICAgICAg ICAqIGZhc3QveHBhdGgvc3Vic3RyaW5nLW5hbi1wb3NpdGlvbi5odG1sOiBBZGRlZC4KKwogMjAx MC0wNy0wOSAgTmlrb2xhcyBaaW1tZXJtYW5uICA8bnppbW1lcm1hbm5AcmltLmNvbT4KIAogICAg ICAgICBOb3QgcmV2aWV3ZWQuCkluZGV4OiBMYXlvdXRUZXN0cy9mYXN0L3hwYXRoL3N1YnN0cmlu Zy1uYW4tcG9zaXRpb24tZXhwZWN0ZWQudHh0Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL2Zh c3QveHBhdGgvc3Vic3RyaW5nLW5hbi1wb3NpdGlvbi1leHBlY3RlZC50eHQJKHJldmlzaW9uIDAp CisrKyBMYXlvdXRUZXN0cy9mYXN0L3hwYXRoL3N1YnN0cmluZy1uYW4tcG9zaXRpb24tZXhwZWN0 ZWQudHh0CShyZXZpc2lvbiAwKQpAQCAtMCwwICsxLDEwIEBACitUZXN0IGZvciBidWcgNDE4NjI6 IFhQYXRoIHN1YnN0cmluZyBmdW5jdGlvbiBpcyBicm9rZW4gd2hlbiBwYXNzaW5nIE5hTiBhcyB0 aGUgcG9zaXRpb24gcGFyYW1ldGVyLgorCitQQVNTIGRvY3VtZW50LmV2YWx1YXRlKCJzdWJzdHJp bmcoJzEyMzQ1JywgbnVtYmVyKCdOYU4nKSkiLCBkb2N1bWVudCwgbnVsbCwgWFBhdGhSZXN1bHQu U1RSSU5HX1RZUEUsIG51bGwpLnN0cmluZ1ZhbHVlIGlzICcnCitQQVNTIGRvY3VtZW50LmV2YWx1 YXRlKCJzdWJzdHJpbmcoJzEyMzQ1JywgbnVtYmVyKCdOYU4nKSwgMykiLCBkb2N1bWVudCwgbnVs bCwgWFBhdGhSZXN1bHQuU1RSSU5HX1RZUEUsIG51bGwpLnN0cmluZ1ZhbHVlIGlzICcnCitQQVNT IGRvY3VtZW50LmV2YWx1YXRlKCJzdWJzdHJpbmcoJzEyMzQ1JywgbnVtYmVyKCdOYU4nKSwgLTIx NDc0ODM2NDUpIiwgZG9jdW1lbnQsIG51bGwsIFhQYXRoUmVzdWx0LlNUUklOR19UWVBFLCBudWxs KS5zdHJpbmdWYWx1ZSBpcyAnJworUEFTUyBkb2N1bWVudC5ldmFsdWF0ZSgic3Vic3RyaW5nKCcx MjM0NScsIG51bWJlcignTmFOJyksIG51bWJlcignTmFOJykpIiwgZG9jdW1lbnQsIG51bGwsIFhQ YXRoUmVzdWx0LlNUUklOR19UWVBFLCBudWxsKS5zdHJpbmdWYWx1ZSBpcyAnJworUEFTUyBzdWNj ZXNzZnVsbHlQYXJzZWQgaXMgdHJ1ZQorCitURVNUIENPTVBMRVRFCisKSW5kZXg6IExheW91dFRl c3RzL2Zhc3QveHBhdGgvc3Vic3RyaW5nLW5hbi1wb3NpdGlvbi5odG1sCj09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0t IExheW91dFRlc3RzL2Zhc3QveHBhdGgvc3Vic3RyaW5nLW5hbi1wb3NpdGlvbi5odG1sCShyZXZp c2lvbiAwKQorKysgTGF5b3V0VGVzdHMvZmFzdC94cGF0aC9zdWJzdHJpbmctbmFuLXBvc2l0aW9u Lmh0bWwJKHJldmlzaW9uIDApCkBAIC0wLDAgKzEsMjMgQEAKKzwhRE9DVFlQRSBIVE1MIFBVQkxJ QyAiLS8vSUVURi8vRFREIEhUTUwvL0VOIj4KKzxodG1sPgorPGhlYWQ+Cis8bGluayByZWw9InN0 eWxlc2hlZXQiIGhyZWY9Ii4uL2pzL3Jlc291cmNlcy9qcy10ZXN0LXN0eWxlLmNzcyI+Cis8c2Ny aXB0IHNyYz0iLi4vanMvcmVzb3VyY2VzL2pzLXRlc3QtcHJlLmpzIj48L3NjcmlwdD4KKzwvaGVh ZD4KKzxib2R5PgorPHA+VGVzdCBmb3IgPGEgaHJlZj0iaHR0cDovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9NDE4NjIiPmJ1ZyA0MTg2MjwvYT46CitYUGF0aCBzdWJzdHJpbmcgZnVu Y3Rpb24gaXMgYnJva2VuIHdoZW4gcGFzc2luZyBOYU4gYXMgdGhlIHBvc2l0aW9uIHBhcmFtZXRl ci48L3A+Cis8ZGl2IGlkPSJjb25zb2xlIj48L2Rpdj4KKworPHNjcmlwdD4KKyAgICBzaG91bGRC ZSgiZG9jdW1lbnQuZXZhbHVhdGUoXCJzdWJzdHJpbmcoJzEyMzQ1JywgbnVtYmVyKFwnTmFOXCcp KVwiLCBkb2N1bWVudCwgbnVsbCwgWFBhdGhSZXN1bHQuU1RSSU5HX1RZUEUsIG51bGwpLnN0cmlu Z1ZhbHVlIiwgIicnIik7CisgICAgc2hvdWxkQmUoImRvY3VtZW50LmV2YWx1YXRlKFwic3Vic3Ry aW5nKCcxMjM0NScsIG51bWJlcihcJ05hTlwnKSwgMylcIiwgZG9jdW1lbnQsIG51bGwsIFhQYXRo UmVzdWx0LlNUUklOR19UWVBFLCBudWxsKS5zdHJpbmdWYWx1ZSIsICInJyIpOworICAgIHNob3Vs ZEJlKCJkb2N1bWVudC5ldmFsdWF0ZShcInN1YnN0cmluZygnMTIzNDUnLCBudW1iZXIoXCdOYU5c JyksIC0yMTQ3NDgzNjQ1KVwiLCBkb2N1bWVudCwgbnVsbCwgWFBhdGhSZXN1bHQuU1RSSU5HX1RZ UEUsIG51bGwpLnN0cmluZ1ZhbHVlIiwgIicnIik7CisgICAgc2hvdWxkQmUoImRvY3VtZW50LmV2 YWx1YXRlKFwic3Vic3RyaW5nKCcxMjM0NScsIG51bWJlcihcJ05hTlwnKSwgbnVtYmVyKFwnTmFO XCcpKVwiLCBkb2N1bWVudCwgbnVsbCwgWFBhdGhSZXN1bHQuU1RSSU5HX1RZUEUsIG51bGwpLnN0 cmluZ1ZhbHVlIiwgIicnIik7CisKKyAgICB2YXIgc3VjY2Vzc2Z1bGx5UGFyc2VkID0gdHJ1ZTsK KworPC9zY3JpcHQ+Cis8c2NyaXB0IHNyYz0iLi4vanMvcmVzb3VyY2VzL2pzLXRlc3QtcG9zdC5q cyI+PC9zY3JpcHQ+Cis8L2JvZHk+Cis8L2h0bWw+Cg==