private/python_ipc_system: Convert to Cygwin path when needed.

[alexvong243f]

This should fix some errors when Python is running in Cygwin-like environment. But there could still be errors in other places.

See #1182.

• inst/private/cygpath.m: New function.
• inst/private/python_env_is_cygwin_like.m: New function.
• inst/private/python_ipc_system.m: Use them.

[alexvong243f]

It should work in theory. I did some tests using shim but not in a real cygwin-like environment, as wine does not support cygwin / msys2.

[cbm755]

Nice work! Maybe we can have sympref diagnose report this information too, see #1009.

[mmuetzel]

Is there a reason why you prefer opening a pipe over using system (for this one off call)? Does octsympy try to be Matlab compatible? Afaict, popen2 is an Octave-specific function. On POSIX platforms, popen2 is just an Octave-wrapper for the native function of the same name. But that function doesn't exist on Windows afaict. Octave tries to emulate that by wrapping around some Windows API functions. While that should work for most cases, it might be "safer" to just use system instead. (Unless, there is a good reason for using popen2.)

Edit: Maybe also check if system ('where /Q cygpath') returns 0 (and keep a persistent result) in a first step to quickly rule out if this approach won't work anyway...

[alexvong243f]

------- Original Message ------- On Tuesday, July 19th, 2022 at 7:54 AM, Markus Mützel @.***> wrote:

Is there a reason why you prefer opening a pipe over using system? Does octsympy try to be Matlab compatible? Afaict, popen2 is an Octave-specific function. On POSIX platforms, popen2 is just an Octave-wrapper for the native function of the same name. But that function doesn't exist on Windows afaict. Octave tries to emulate that by wrapping around some Windows API functions. While that should work for most cases, it might be "safer" to just use system instead. (Unless, there is a good reason for using popen2.)

I was worried about the use of "system" because of #1143 (escaping string is error-prone and shell-dependent). It's much better to call subprocess w/o going through shell. Anyway, we have to stick with "system" for now as "popen2" is not portable. I have in mind an idea of a portable "system2" function but we can worry about that later.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

[mmuetzel]

IIUC, it is true that a malevolent user might be able to able to execute arbitrary programs by passing specifically grafted strings. But I'd say that this risk is pretty low if the string to be executed is essentially a literal. (Or the risk is basically the same with popen2, e.g., if binaries are replaced.) On the other hand, if a user is malevolent, they might as well just use the system function directly in Octave. I don't currently see why they would try to route their commands through this package...

[alexvong243f]

------- Original Message ------- On Tuesday, July 19th, 2022 at 11:44 AM, Markus Mützel @.***> wrote:

IIUC, it is true that a malevolent user might be able to able to execute arbitrary programs by passing specifically grafted strings. But I'd say that this risk is pretty low if the string to be executed is essentially a literal. (Or the risk is basically the same with popen2, e.g., if binaries are replaced.)

The use of "system" in "python_env_is_cygwin_like" is probably fine since the input is a literal but the input to "cygpath" can be anything. I think it'd be much easier to have a secure building block so we don't have to worry about how it's used in the future (the "cygpath" function could be used elsewhere to solve similar issues in the future).

On the other hand, if a user is malevolent, they might as well just use the system function directly in Octave. I don't currently see why they would try to route their commands through this package...

The risk of "system" is probably low for local user running octave themselves, which is the most common scenario. The problem arises when someone tries to use octsympy as a backend remotely, say to evaluate symbolic expression in a website for educational purposes. This could potentially be combined with other vulnerabilities to get an exploit.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

[mmuetzel]

The input to cygpath () (at least for the changes you propose) is the output of tempname with a literal suffix. That should be pretty safe. Additionally, it is something for which fopen returned successfully. Maybe with some Unicode shenanigans it might be possible to construct a byte sequence that evaluates to something that fopen is happy with but that could still cause harm when passed as a argument to cygpath. But that is already highly theoretical. That would still leave the "task" of getting tempname to return that byte sequence. Maybe possible??? But even if it would be possible to exploit that (and I don't know if it is), Octave in that scenario would hopefully run isolated on that web service. So, the amount of damage would hopefully be limited. (And who would use Windows for that web service? 😉)

But to get this PR back on track: Your changes look good. But at least for the sake of compatibility with Matlab, it might be better to use system imho. And maybe check pretty early on what system ('where /Q cygpath') returns (0 means it could find cygpath, anything else means it couldn't).

[cbm755]

Does octsympy try to be Matlab compatible?

Yes, to some extent. But the popen2 IPC stuff is Octave-only, and is faster thansystem. Those are general answers. But here within the system-based IPC we should use system.

[mmuetzel]

I'd guess opening a pipe is only faster if there is repeated communication over the pipe. So, you don't need to spawn a new process each time. For these "one time" invocations, there shouldn't be much of a difference with respect to performance. (I haven't done any measurements though.)

[cbm755]

Thanks @alexvong1995 and @mmuetzel for detailed reviewing. Merge this whenever you are both happy.

[mmuetzel]

LGTM.