builtins: make ltrimstr and rtrimstr error for non-string inputs

[emanuele6]

I also refactored the code in rtrimstr to make it more similar to the
code used by ltrimstr.

Previously, ltrimstr/rtrimstr would just let the input pass through for
non-string inputs or arguments.

That was happening because, they were leaking the errors returned by
startswith/endswith treating them as if they were jv_false().

This patch also fixes that memory leak discovered by oss-fuzz

Ref: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=64946

[emanuele6]

gojq copies ltrimstr/rtrimstr behaving like . if the input or the argument is not a string; do we want to preserve that behaviour, and just free the error message instead of leaking it?

[itchyny]

I thought the previous behavior is useful for recursive trimming (.. |= ltrimstr). But I also understand the inconsistency in the builtins, and using strings filter allows to trim recursively, so I'm okay with changing the behavior.

[emanuele6]

Since f_startswith / f_endswith are basically

def ltrimstr($left): if startswith($left) then .[$left | length:] end;
def rtrimstr($right): if endswith($right) then .[:$right | -length] end;

defined in C, should we just rewrite them in jq in src/builtin.jq?

The memory "optimisation" that is mentioned in the FIXME in f_ltrimstr could be handled by string slices, if we even want it...

[wader]

Agree on making them consistent with other string functions and it does not look like we have documented the passthru behaviour anywhere.

[emanuele6]

What do you think about rewriting those functions in jq instead of keeping the C implemenations?

[wader]

What do you think about rewriting those functions in jq instead of keeping the C implemenations?

If no big performance difference i think it makes sense

[wader]

Wait to see if @itchyny is happy too?

[itchyny]

LGTM

[itchyny]

If you could it's better to fix leaks in the patch release and change the behavior in 1.8.

[emanuele6]

Should I just create a new PR and leave this open for 1.8 then?

[emanuele6]

I think the next release should be 1.8 since we already merged non-bug fix changes like the one that allows using # \ everywhere

[itchyny]

we already merged non-bug fix changes like the one that allows using # \ everywhere

That's the only feature addition in 1.7? I think we should still discuss on the patch release for security issues, reverting the comment changes.

[emanuele6]

If you want to release a patch release before 1.8, reverting the comment change, only keeping the bug fix that allows CR in comments is fine I guess

[nicowilliams]

Any reason not to merge this?

[wader]

LGTM unless the change of pass through is a backwards compatibility issue?

[nicowilliams]

Yeah, I'm not concerned about the passthrough change.