builtins: make ltrimstr and rtrimstr error for non-string inputs #2969
Conversation
dc63b3c
to
9a1e508
Compare
|
|
|
I thought the previous behavior is useful for recursive trimming ( |
|
Since def ltrimstr($left): if startswith($left) then .[$left | length:] end;
def rtrimstr($right): if endswith($right) then .[:$right | -length] end;defined in The memory "optimisation" that is mentioned in the FIXME in |
|
Agree on making them consistent with other string functions and it does not look like we have documented the passthru behaviour anywhere. |
9a1e508
to
98be5a9
Compare
|
What do you think about rewriting those functions in |
If no big performance difference i think it makes sense |
98be5a9
to
786d8e9
Compare
|
If you could it's better to fix leaks in the patch release and change the behavior in 1.8. |
786d8e9
to
decfb30
Compare
|
Should I just create a new PR and leave this open for 1.8 then? |
ltrimstr/rtrimstr was ignoring and leaking the error returned by f_startswith()/f_endswith(). This also means that they just let the input pass through for non-string inputs or arguments. Only fix the leak for now; in the next release, jqlang#2969 will make them rethrow the error returned by startswith/endswith. Ref: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=64946
ltrimstr/rtrimstr was ignoring and leaking the error returned by f_startswith()/f_endswith(). This also means that they just let the input pass through for non-string inputs or arguments. Only fix the leak for now; in the next release, jqlang#2969 will make them rethrow the error returned by startswith/endswith. Ref: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=64946
|
I think the next release should be |
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. |
|
If you want to release a patch release before |
ltrimstr/rtrimstr was ignoring and leaking the error returned by f_startswith()/f_endswith(). This also means that they just let the input pass through for non-string inputs or arguments. Only fix the leak for now; in the next release, #2969 will make them rethrow the error returned by startswith/endswith. Ref: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=64946
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(). The leak was resolved by jqlang#2977 for 1.7.1 This patch rewrites ltrimstr and rtrimstr in jq, and makes them not ignore startswith and endswith errors anymore.
decfb30
to
060be3d
Compare
|
Any reason not to merge this? |
|
LGTM unless the change of pass through is a backwards compatibility issue? |
|
Yeah, I'm not concerned about the passthrough change. |
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