Bug 41862 – [Arm] Missing NaN check in XPath substring function

• Component: XML
• Status: RESOLVED FIXED

[Ben Murdoch]

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!

[Ben Murdoch]

Created attachment 60892 [details]
Proposed patch.

[Steve Block]

Comment on attachment 60892 [details]
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.

[Ben Murdoch]

(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.

[Ben Murdoch]

Created attachment 61037 [details]
Patch and layout test.

[Steve Block]

Comment on attachment 61037 [details]
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'

[Ben Murdoch]

Comment on attachment 61037 [details]
Patch and layout test.

Thanks Steve. Will update the changelogs on landing.

[Ben Murdoch]

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.