|Summary:||XPath substring function does not correctly handle non-positive values for the position argument|
|Product:||WebKit||Reporter:||Steve Block <steveblock>|
|Severity:||Normal||CC:||ap, benm, steveblock|
|Version:||528+ (Nightly build)|
Description Steve Block 2010-07-08 15:36:20 PDT
In the XPath substring function, if a negative value is supplied for the position argument and a length argument is present, we reset the position argument to the first character of the string and adjust the length argument such that the final character in the substring is the same as it would have been if the length were counted from the original negative position value. We then return the substring using the new position and length. However, in the case where no length argument is present, we call String::substring() with the negative position value. Since String::substring() takes two unsigned int arguments, the position value gets wrapped to a large integer and we likely return the empty string because the wrapped position value exceeds the string length. This seems incorrect and is inconsistent with the above case. See Bug 41862 for some background.
Comment 1 Alexey Proskuryakov 2010-07-09 10:02:35 PDT
Yes, substring("12345", -1) should return "12345". This is not only about negative numbers -substring("12345", 0) is also wrong for the same reason.
Comment 2 Alexey Proskuryakov 2010-07-09 10:06:16 PDT
Comment 3 Steve Block 2010-07-09 10:07:02 PDT
> Yes, substring("12345", -1) should return "12345". This is not only about > negative numbers -substring("12345", 0) is also wrong for the same reason. Yes, I meant non-positive numbers. Patch coming soon.
Comment 5 Alexey Proskuryakov 2010-07-09 10:55:51 PDT
Comment on attachment 61054 [details] Patch > + we reset the position to 1. This matches the current behaviour when a length > + argument is supplied. This fix is not really about consistency - there is a rigorous spec, and we now match it. Does the new test pass in Firefox? r=me
Comment 6 Steve Block 2010-07-12 02:51:20 PDT
> This fix is not really about consistency - there is a rigorous spec, and we > now match it. OK, have updated the bug title and will land with an updated description > Does the new test pass in Firefox? Yes