Address system() call portability

[paralin]

The locale command is not available on all machines.

It's totally unnecessary to run system("locale") on line 316 of mosh_server.cc. I vote to remove it.

[keithw]

Arguments for keeping it:

(1) Mosh only tries to run it if the user's configuration is already broken and we need to print a diagnostic message. The diagnostic message needs to be verbose and explanatory because this is a confusing area, Mosh is more pedantic than most programs, and nobody wants to believe that their configuration is actually broken.

(2) Having "locale" is a POSIX/Single Unix Spec requirement (since 2001). Every POSIX system has it.

(3) A missing locale is a nonfatal error. (And mosh is already quitting already when we get to that point!)

(4) If a system really doesn't have it, the distributor could remove it on that architecture.

Arguments for getting rid of it:

(1) Which platforms are we talking about that don't have it?

[jmcp]

On 24/07/15 06:50 AM, Christian Stewart wrote:

The |locale| command is not available on all machines.

It's totally unnecessary to run |system("locale")| on line 316 of
mosh_server.cc. I vote to remove it.

ugh .. execing system() is a generally bad idea. Use of
execve() is much better.

James C. McPherson

Oracle
Systems / Solaris / Core
https://www.jmcpdotcom.com/blog

[glensc]

the code bloat around execve() proper handling (fork() etc) is not worth of it. if you use system() with constant parameters (not coming from any user input), it's not that big security issue.

[paralin]

The system that doesn't have it is an ODROID XU3 running a Buildroot system which uses Busybox as the core.

I don't think executing a system() call just to output some debug information is justified. The user already knows they have a locale problem, does running locale really output valuable enough information to not just tell the user to go do it themselves?

I'm writing a buildroot cross-compilation package for Mosh at the moment which is why I'm dealing with these edge cases. I think in something SSH-like and security-oriented like Mosh, portability should probably be emphasized. Calling the locale command seems really against that.

[keithw]

Yes, in practice, we have had a lot of user confusion around mismatched locales (and Mosh's strict enforcement of a working configuration before starting up), which made us make this error message more and more verbose way back in Mosh 1.2 (released April 2012).

We often have to ask users to paste the full error message so we can diagnose the problem. The "locale" section has been helpful to see what is going on, e.g. which vars are being set explicitly and which are inferred!

If you're porting Mosh to a non-POSIX system that doesn't have the command, here are some of your options:

(1) Comment out the line. Nothing horrible will happen. (We'd appreciate if you made it print something out like "locale information omitted for Buildroot port" so just in case a user pops up with this error message and somebody is trying to debug it, we won't be confused why the locale information is missing.)

(2) Leave the line in. Nothing horrible will happen; it is literally the last thing that mosh-server does before "exit( 1 )". And failure is nonfatal anyway!

(3) Implement a locale command, either as a real command in the filesystem or by writing some code in your Mosh port to print similar output.

[paralin]

@keithw Alright, fair enough. I'll have a patch sequence in Buildroot that removes the lines and adds another debug output line:

"locale" command output removed in buildroot

Let me know if you need anything else for debugging purposes. Closing this.

[paralin]

Would you guys be open to checking if the command exists before attempting to execute it, and if it doesn't exist, printing out a debug line saying "locale command does not exist on this system"?

This is a request to me from the buildroot maintainers. They would prefer to not have a local patch for the package.

[andersk]

That is precisely how Mosh behaves today, except that the debug line is spelled sh: locale: command not found instead of locale command does not exist on this system.

[paralin]

@andersk Ah, I see. But that error is coming from the system level, you don't actually check it exists before executing it.

[andersk]

Is there something wrong about making the system check for us?

[paralin]

No, it's okay. I was requesting on behalf of the buildroot maintainers. I think we'll remove the patch from our end and just let it fail like that as you say.

Thanks.