use Win32::ShellQuote for kpsewhich

[xworld21]

Better fix for #2293: after adding some Windows-specific escaping, kpsewhich can be called with arbitrary file names, even with special characters, and so pathname_is_nasty can be removed altogether. This supersedes #2295, #2294.

The if (open(my $resfh, '-|', ...)) block can definitely be useful elsewhere. In Util::Pathname, there are two backticks left, and there are a few other not-quite-safe system calls in other modules. However, @brucemiller @dginev you should decide if and how to make that happen, e.g. a new Util module maybe?

[dginev]

It's great you took the effort to make sure escaping works correctly in Windows - much appreciated!

I have warmed up to the idea of dispensing with the pathname_is_nasty function entirely, and avoid the class of problems from discarding too many paths.

Using the -| open pattern in as many places as possible does sound appealing. I would also defer to @brucemiller on deciding when/where to refactor the remaining bits, since we need to juggle getting the current release out the door.

As far as I'm concerned, this PR looks good to merge!

[xworld21]

It's great you took the effort to make sure escaping works correctly in Windows - much appreciated!

Side note: it is impossible to escape arguments 100% correctly on Windows, because each binary is responsible for parsing its own command line and while most adopt the same scheme, Cygwin, MSYS2, etc sometimes do things differently, or so I hear. When it comes to kpsewhich, I understand that both MikTeX and TeX Live are built with Visual Studio, so they should be using CommandLineToArgvW under the hood. Hence Win32::ShellQuote is the correct choice for them. With Cygwin, LaTeXML is already 100% broken so let's not think about it.

The important thing is that the -| pattern with a list of arguments is guaranteed to execute the binary directly, with no risk of creating pipes, redirections, etc. so it is safe in all settings. Well, unless you know how to exploit kpsewhich (the Cygwin one!) with a weird command line argument...

[brucemiller]

Have we ensured that? I'm unclear what this warning means. Should we only call quote_system_list when scalar(@_) > 1 or ...?

[xworld21]

Should we only call quote_system_list when scalar(@_) > 1 or ...?

Yes. This is a maddening Perl quirk. To trigger the safe call you must use open FILEHANDLE,MODE,EXPR,LIST, in which case EXPR is treated as the binary file name, no parsing.

However, if LIST is empty, the call becomes open FILEHANDLE,MODE,EXPR and Perl uses heuristics to decide if EXPR is a command line (e.g. contains >/dev/null) and possibly runs a shell on it.

So safe calls are guaranteed only when (EXPR,LIST) has length at least 2. At least that's what the Win32::ShellQuote author says: https://metacpan.org/release/HAARG/Win32-ShellQuote-0.003001/view/lib/Win32/ShellQuote.pm#quote_system_list

[brucemiller]

OK, I think I understand; the comments are somewhat confusing out of context. So, basically, we must have both $kpsewhich and @candidates to get the proper call. And in fact we check that in the 2nd line of pathname_kpsewhich; Although arguably that line might be safer in hindsight as

return unless $kpsewhich && grep { $_; } @candidates;

And moreover this may be a case where it's clearer not to pull pathname_quote_system_list out as a separate sub, but embed it within pathname_kpsewhich. (so nobody's tempted to export it and call with less tested args).

[xworld21]

Right, I moved the code around and updated the comments, hopefully for the better. The 2nd line of pathname_kpsewhich is ok: for the purposes of calling open safely, return unless @candidates would already be enough (that is to say, empty or even undefined arguments are fine, it's all about the length of @candidates).

[brucemiller]

Looks good; Thanks for this!!