mosh.pl: Allow shell expansion of --server with --local #947
Conversation
Fixes mobile-shell#946, matching the behavior of --server without --local. Signed-off-by: Anders Kaseorg <andersk@mit.edu>
| @@ -396,7 +396,7 @@ sub predict_check { | |||
| delete $ENV{ 'SSH_CONNECTION' }; | |||
| chdir; # $HOME | |||
| print "MOSH IP ${userhost}\n"; | |||
| exec( $server, @server ); | |||
There was a problem hiding this comment.
I'm wondering, is there any reason to use shell_quote() here at all? We could just do exec( "server " . join( " ", @server ), since we're not running ssh here.
There was a problem hiding this comment.
Joining an array with spaces and no quoting is always the wrong thing to do. It’s a fundamentally lossy operation. In this case, it would manifest by corrupting @command: mosh --local ::1 -- cmd 'arg with spaces' would result in cmd arg with spaces. Even if that bug weren’t as easy to manifest as it is, it would still be locally a bug. Don’t do it. Don’t dooo ittt.
There was a problem hiding this comment.
I plead stupid, your honor. (I couldn't sleep. I got up and posted that. Went back to bed, five minutes later realized it was the wrong thing.)
I'll wait for a response from @TomiBelan before pulling though.
There was a problem hiding this comment.
Looks good to me (unsurprisingly...), though as I mentioned I didn't test it myself.
There was a problem hiding this comment.
Actually, looking at this again, this code does break if there's a separator or metacharacter in the executable pathname (such as /Users/cgull/spacey directory/mosh/src/frontend/mosh-server). I believe it should be exec( shell_quote( $server, @server ) ), so as to quote the executable name too. I think I'm understanding Perl correctly here. @andersk, concur?
There was a problem hiding this comment.
That doesn't sound right. If the goal is to make --server='env foo=bar mosh-server' work with and without --local, then spaces in $server should not be quoted.
There was a problem hiding this comment.
Right, like @TomiBelan said, --server is intentionally passed without quoting so the shell can split it (19884d6), and the point of this PR is to fix --local to match that behavior.
There was a problem hiding this comment.
I started looking at a shell quoting problem in src/tests that this interacts with and forgot about the original problem...
Pulled now.
|
@TomiBelan: I still want to know what your use case is-- is it just devel/experimental use of Mosh, or have you got some weird situation where |
|
Oh, I misunderstood and thought you want my response to this PR. I commented in #946. |
Fixes #946, matching the behavior of --server without --local.