Bug 41913

Summary:

XPath substring function does not correctly handle non-positive values for the position argument

Product:

WebKit

Reporter:

Steve Block <steveblock>

Component:

XML

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, benm, steveblock

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	ap: review+

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

See <http://www.w3.org/TR/xpath/#function-substring>.

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 4 Steve Block 2010-07-09 10:17:33 PDT

Created attachment 61054 [details]
Patch

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

Comment 7 Steve Block 2010-07-12 03:44:13 PDT

Committed r63066: <http://trac.webkit.org/changeset/63066>