xmerl: Fix XPath predicate/function bugs and an is_incharset crash

[tomciopp]

Summary

This fixes four correctness bugs in xmerl's XPath engine (xmerl_xpath_pred) and one crash in xmerl_ucs, each with a regression test. The bugs are independent and grouped here because they were found together while reviewing the same modules; the branch is three focused commits.

What's fixed

1. XPath relational operators <, >=, <=

The four relational operators in xmerl_xpath_pred:comp_expr/4 share a comparison helper: for a OP b the code computes compare_ineq_format(A, B) OP compare_ineq_format(B, A), where compare_ineq_format/3 coerces each operand to the type XPath requires (number, or a node‑set's string value). The '>' clause was written correctly, but the '<', '>=' and '<=' clauses were copy‑pasted from it and never had their operator changed — all three used >:

comp_expr('<', E1, E2, C) ->
    N1 = expr(E1, C), N2 = expr(E2, C),
    ?boolean(compare_ineq_format(N1, N2, C) > compare_ineq_format(N2, N1, C));

The result is that in any XPath predicate, a < b, a >= b and a <= b all silently evaluated a > b. //item[position() <= 3] selected the items after position 3 rather than the first three; //item[@n >= 5] behaved like > 5; and so on. Only =, != and > behaved correctly, which is why this survived — inequality predicates other than > are comparatively rare.

The fix gives each clause its correct operator (<, >=, =<), leaving the compare_ineq_format/3 machinery — and therefore the node‑set coercion semantics — untouched.

2. substring-before() off‑by‑one

substring-before(s1, s2) should return the part of s1 that precedes the first occurrence of s2. The implementation found the 1‑based index of the separator with string:str/2 and then took that many leading characters:

Pos = string:str(S1, S2),
?string(string:substr(S1, 1, Pos)).

Because Pos is the position of the separator, substr(S1, 1, Pos) includes the separator's first character. substring-before("1999/04/01", "/") returned "1999/" instead of "1999", and substring-before("a::b", "::") returned "a:" instead of "a".

The fix rewrites the function to mirror the neighbouring substring-after/2: a case on string:str/2 that returns "" when the separator is absent (as XPath 1.0 requires) and string:substr(S1, 1, Pos - 1) otherwise. Handling the not‑found case explicitly also avoids the negative length that a naive Pos - 1 would produce when string:str/2 returns 0.

3. string-length(), sum(), round() and number() numeric results

Every value‑producing function in this module is expected to return an #xmlObj{} — the evaluator (eval/2) reads Obj#xmlObj.type, and the coercion helpers (mk_number/2, mk_string/2, …) only accept #xmlObj values or unevaluated expression tuples. number/2, floor/2 and ceiling/2 follow this and wrap their results in the ?number/1 macro (#xmlObj{type = number, value = V}).

string-length/2, sum/2 and round/2 did not — they returned a bare Erlang integer/float:

'string-length'(C, [A]) ->
    length(mk_string(C, A)).            %% bare integer

As long as such a result is the whole predicate or feeds a comparison, it reaches eval/2 or mk_number/2, neither of which has a clause for a bare number, so evaluation crashed ({badrecord, xmlObj} from eval/2, or {unknown_expr, N} from mk_number/2 → expr/2). In practice this meant predicates like //e[string-length(@id) = 5], //total[sum(../item) > 100] or //e[round(@score) = 2] failed rather than filtering. The fix wraps each result in ?number/1.

Adding tests for sum() exposed a second, deeper bug that the return‑type crash had been masking. sum/2 (and the zero‑argument number()) computed the per‑node string value with string(C, N), but string/2 only has clauses for a list argument (string(C, []) and string(C, [Arg])) — it was being called with a bare #xmlNode. That produced a function_clause on any non‑empty node set, so sum() and number() never actually worked at all. Both now call string_value/1 directly, which is the helper that already knows how to take the string value of an individual node (element, attribute, text, …). With that and the ?number/1 wrapper, sum() and number() are functional for the first time.

4. xmerl_ucs:is_incharset/2 crash on multi‑character lists

is_incharset(In, Charset) reports whether In is representable in Charset. Common charsets (US‑ASCII, Latin‑1, ISO‑646) have dedicated clauses that test each character with a predicate; everything else falls through to a generic clause that attempts a conversion with to_unicode/2 and treats success as "in charset". The is_list(In) variant of that generic clause only matched a single‑element conversion result:

is_incharset(In, Charset) when is_list(In) ->
    case to_unicode(In, Charset) of
        {error, unsupported_charset} -> {error, unsupported_charset};
        {error, _}                   -> false;
        [Int] when is_integer(Int)   -> true      %% only length‑1 results
    end.

to_unicode/2 returns the full list of code points, so a multi‑character input (e.g. is_incharset("abc", 'utf-8'), which routes here rather than to the Latin‑1 fast path) yields a multi‑element list that matches no branch and raises a case_clause. The function was therefore only usable for single‑character lists.

The fix replaces [Int] when is_integer(Int) with L when is_list(L): a successful to_unicode/2 conversion of any length means every character mapped, i.e. the whole input is in the charset. Error results (false / {error, unsupported_charset}) are unchanged.

Testing

Each fix has a regression test:

• Relational operators: a new xpath_relational case (xpath_abbrev:relational_operators/0) covering <, <=, >=, > over both position() and constant operands.
• substring-before/string-length/sum/round/number(): added to the existing xpath_functions case (xpath_abbrev:functions/0).
• is_incharset/2: a new ucs_is_incharset case.

Every new test was confirmed to fail on the unfixed code (wrong‑value badmatch for the operators and substring-before; a case_clause / function_clause crash for the numeric functions and is_incharset) and to pass with the fix. The full xmerl_SUITE is green.

Notes

All five are pure correctness fixes with no behavioural change for already‑correct inputs. No public API or record changes.

[CLAassistant]

All committers have signed the CLA.

[github-actions]

CT Test Results

Tests are running... https://github.com/erlang/otp/actions/runs/31864696684

Results for commit 6f6d116

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

• No CT logs found
• No HTML docs found
• No Windows Installer found

// Erlang/OTP Github Action Bot