Setting `IFS=_` breaks __bp_precmd_invoke_cmd

[gnachman]

I received the following issue report:

https://gitlab.com/gnachman/iterm2/issues/6398

The internals of __bp_precmd_invoke_cmd assume IFS has its default value. I think you could do this:

1. At the top of __bp_precmd_invoke_cmd, save the current value of IFS to a local variable
2. Set IFS to \t\n
3. At the end of __bp_precmd_invoke_cmd restore IFS to the saved value.

What do you think?

[rcaloras]

@gnachman Thanks for reporting! Did as you suggested storing it. I created a couple small helpers based on https://unix.stackexchange.com/a/264947. Give #65 a quick look when you get a sec. I'll cut a version as soon as it's merged.

[rcaloras]

cc @D630 @dimo414 @lguelorget bash-preexec's ⭐️ contributors.

[dimo414]

I worry the proposed fix would essentially introduce the opposite problem, since the user's function would be invoked with bash-preexec's IFS. Sure, we could do the same resetting/restoring logic inside the loops too, but that's pretty nasty.

What exactly is the behavior that's IFS-dependent currently? At a glance I don't see it.

[rcaloras]

@dimo414 That's true. IFS will be set to bash-preexec's definition for precmd and preexec.

Maybe I'm over complicating things, but if I don't have default IFS our for loops will break. For read statements it's possible to set IFS on the same line. Wonder if I can do that with our loops as well.

[rcaloras]

Updated to set original IFS across precmd and preexec functions

[dimo414]

if I don't have default IFS our for loops will break

Can you share a particular example? I believe that array-splitting (e.g. for e in "${my_array[@]}") is not impacted by IFS, though I certainly could be wrong about that. I wasn't able to break anything with a few attempts.

[d630]

Ja, I don't think setting/resetting of IFS is necessary. But you have to take care about (at least):

1. the read builtin. Set IFS as keyword parameter in front of it. Or don't use read in a pipe at all. E.g.:

# stupid

printf -v n %\*s 125 ' ';
this_command=$(HISTTIMEFORMAT=$'\r'$n$'\r' builtin history 1);

2. qouting of any function executions within the loops. If the user wants to use aliases or redefine/unset internal functions, there is also the next problem.

alias __bp_set_ret_value=ls;
__bp_set_ret_value;

So, I suggest:

        if type -t "$precmd_function" 1>/dev/null; then
            \__bp_set_ret_value "$__bp_last_ret_value" "$__bp_last_argument_prev_command"
            "$precmd_function"
        fi

One can even think about turning on the readonly attribute ...

[dimo414]

Ok sorry, I just realized the issue is specifically with IFS=_, I misread _ as a placeholder for something non-standard. Using _ as the field separator seems like a Bad Idea™ in general, but it probably still makes sense to try to support this in bash-prexec.

I think the right first step is to add some unit tests that everything works as expected with a malicious IFS. Capturing and restoring the IFS may be the best option. We might be able to avoid it (e.g. by more aggressively quoting everything in bash-preexec), but that may be more trouble than it's worth.

[rcaloras]

Thanks for the feedback @dimo414 and @D630! Just submitted #66 to address by quoting functions and setting IFS for read as advised by @D630. Tested locally and added some unit tests, I believe this fixes the problem.

[rcaloras]

@gnachman Just released 0.3.7 for the fix. Let me know if you still have any issues. Thanks for reporting and feedback!