[PATCH] ob-maxima.el: Fix execution on MS Windows

[PATCH] ob-maxima.el: Fix execution on MS Windows

[Nikolay Kudryavtsev]

[-- Attachment #1: Type: text/plain, Size: 423 bytes --]

Hello.

I've been playing around with Maxima from time to time.

Ob-maxima currently does not work on Windows due to it using single 
quotes in the Maxima invocation and those not being supported by Windows 
CMD.

After some testing I've found an invocation that seems to work fine on 
both Windows and Linux. I don't think this patch can cause any real 
issue, since the string in those quotes is just the temp file path.

[-- Attachment #2: 0001-ob-maxima.el-Fix-execution-on-MS-Windows.patch --]
[-- Type: text/plain, Size: 1041 bytes --]

From b0206d14df8947c831e56eef21fbd38ca88c1fd5 Mon Sep 17 00:00:00 2001
From: Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com>
Date: Sun, 26 Dec 2021 22:47:19 +0300
Subject: [PATCH] ob-maxima.el: Fix execution on MS Windows

* ob-maxima.el (org-babel-execute:maxima): Change command line
invocation to a one that should work everywhere.
---
 lisp/ob-maxima.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/ob-maxima.el b/lisp/ob-maxima.el
index 7b49bb07a..512712221 100644
--- a/lisp/ob-maxima.el
+++ b/lisp/ob-maxima.el
@@ -77,7 +77,7 @@ This function is called by `org-babel-execute-src-block'."
 	(result
 	 (let* ((cmdline (or (cdr (assq :cmdline params)) ""))
 		(in-file (org-babel-temp-file "maxima-" ".max"))
-		(cmd (format "%s --very-quiet -r 'batchload(%S)$' %s"
+		(cmd (format "%s --very-quiet -r \"batchload(\\\"%S\\\")\"$ %s"
 			     org-babel-maxima-command in-file cmdline)))
 	   (with-temp-file in-file (insert (org-babel-maxima-expand body params)))
 	   (message cmd)
-- 
2.34.1.windows.1

Re: [PATCH] ob-maxima.el: Fix execution on MS Windows

[Max Nikulin]

On 27/12/2021 03:18, Nikolay Kudryavtsev wrote:
> 
> Ob-maxima currently does not work on Windows due to it using single 
> quotes in the Maxima invocation and those not being supported by Windows 
> CMD.
> 
> After some testing I've found an invocation that seems to work fine on 
> both Windows and Linux. I don't think this patch can cause any real 
> issue, since the string in those quotes is just the temp file path.

> --- a/lisp/ob-maxima.el
> +++ b/lisp/ob-maxima.el
> @@ -77,7 +77,7 @@ This function is called by `org-babel-execute-src-block'."
>  	(result
>  	 (let* ((cmdline (or (cdr (assq :cmdline params)) ""))
>  		(in-file (org-babel-temp-file "maxima-" ".max"))
> -		(cmd (format "%s --very-quiet -r 'batchload(%S)$' %s"
> +		(cmd (format "%s --very-quiet -r \"batchload(\\\"%S\\\")\"$ %s"
>  			     org-babel-maxima-command in-file cmdline)))


I do not like original variant, but suggested change makes it unsafe in 
more cases. `in-file' might contain apostrophe in the case of peculiar 
path of the directory for temporary files. More characters may be 
interpreted by BASH inside double quotes. Even docstring for 
`shell-quote-argument' mentions security issues with the function.

Ideally command arguments should be passed as a list to avoid 
intermediate interpretation by shell at all. Unfortunately gluing 
strings to make a shell command is used too widely in org code and emacs 
API encourages such unsafe way.

Re: [PATCH] ob-maxima.el: Fix execution on MS Windows

[Nikolay Kudryavtsev]

If your temporary-file-directory is something like "/tmp/apostrophe'", 
it would not work currently either. So apostrophe is a very special case 
here.

As for possible evaluation within the double quotes, while this is 
theoretically possible, user sort of has to go out of his way to trigger 
it, so the question is whether we should introduce any platform-specific 
code to mitigate such an obscure case? Then we are also limited by 
Maxima itself since it has to be able to read that path too and it's 
very picky when it comes to file paths.

Re: [PATCH] ob-maxima.el: Fix execution on MS Windows

[Max Nikulin]

On 30/12/2021 01:37, Nikolay Kudryavtsev wrote:
> If your temporary-file-directory is something like "/tmp/apostrophe'", 
> it would not work currently either. So apostrophe is a very special case 
> here.
> 
> As for possible evaluation within the double quotes, while this is 
> theoretically possible, user sort of has to go out of his way to trigger 
> it, so the question is whether we should introduce any platform-specific 
> code to mitigate such an obscure case? Then we are also limited by 
> Maxima itself since it has to be able to read that path too and it's 
> very picky when it comes to file paths.

I am not a committer, so it is up to the maintainers to decide if your 
patch is suitable. My intention is to draw attention to the issue, 
however they may tolerate it.

I have not experimented with remote execution of babel code blocks using 
tramp, so I may be unaware of some specific, e.g. execution using ssh 
almost certainly assumes shell command and interface with list of 
arguments may not be available.

When some external data is substituted into a Maxima command (batchload 
this case) there should be an extra pass of escaping that protects 
special characters like quotes (and backslashes?) accordingly to Maxima 
rules.

I expect that %S formatter does a trick by adding quotes around the 
string argument and adding backslashes before quote characters and 
backslashes inside. I suspect that quotes your added around %S must not 
be used there. Due to them file name appears outside of quotes at all. 
This error is hidden unless at least a space character presents in 
temporary directory path.

Unsure concerning Maxima but usually it is possible to pass arguments 
avoiding quoting issues for particular language. A couple of examples 
with inline code snippets

emacs -Q --batch --eval '(message "bl(%S)$" (car argv))' 'a"b\c.txt'
sh -c 'printf "bl(%s)\$\n" "$1"' 'sh' 'a"bc\d.txt'

Maybe there is a way to pass file name as a separate argument (without 
combining it with command) to Maxima as well.

In my opinion, platform-specific code should be avoided when possible. 
Even `shell-quote-argument' may be better. I would prefer e.g. 
`call-process' from info "(elisp) Synchronous Processes" 
https://www.gnu.org/software/emacs/manual/html_node/elisp/Synchronous-Processes.html 
, but I am realizing that it may require more changes in babel or even 
to cause problems with tramp.

Double quotes open issues with injection of commands in backticks `rm 
something`, $variable expansion and other constructs. I hope, 
`shell-quote-argument' is reliable enough.

P.S.
https://xkcd.com/327/ Exploits of a Mom

Re: [PATCH] ob-maxima.el: Fix execution on MS Windows

[Nikolay Kudryavtsev]

[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]

> When some external data is substituted into a Maxima command 
> (batchload this case) there should be an extra pass of escaping that 
> protects special characters like quotes (and backslashes?) accordingly 
> to Maxima rules. 
Not necessarily, Maxima is capable of understanding unescaped paths, for 
example, this works:

maxima --very-quiet -r "batchload(\"/tmp/sp 
ce/babel-gxqTkM/maxima-ua3e9j.max\")"$

> I suspect that quotes your added around %S must not be used there. Due 
> to them file name appears outside of quotes at all. 
Yes, good catch.

> Unsure concerning Maxima but usually it is possible to pass arguments 
> avoiding quoting issues for particular language.
Command line Maxima actually has a batch flag, but using it returns the 
entire input file in the output too and that seems to be the reason why 
the original authors of ob-maxima didn't use it. It's probably possible 
to filter that on our side, but such filtering would require extra work, 
which they probably deemed unnecessary, for such a rather obscure set of 
use cases.

Anyway, I've tried to get it to work using shell-quote-argument, see the 
attached patch. Seems to work well enough in practice on both platforms 
and for cases like (setq temporary-file-directory "/tmp/`echo hi`/").

[-- Attachment #2: 0001-ob-maxima.el-Fix-execution-on-MS-Windows.patch --]
[-- Type: text/plain, Size: 1288 bytes --]

From 47690d14ac4838d8e39f08bd8224f0b4af053359 Mon Sep 17 00:00:00 2001
From: Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com>
Date: Sun, 26 Dec 2021 22:47:19 +0300
Subject: [PATCH] ob-maxima.el: Fix execution on MS Windows

* ob-maxima.el (org-babel-execute:maxima): Change command line
invocation to a one that should work everywhere.
---
 lisp/ob-maxima.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/ob-maxima.el b/lisp/ob-maxima.el
index 7b49bb07a..08b586414 100644
--- a/lisp/ob-maxima.el
+++ b/lisp/ob-maxima.el
@@ -77,8 +77,11 @@ This function is called by `org-babel-execute-src-block'."
 	(result
 	 (let* ((cmdline (or (cdr (assq :cmdline params)) ""))
 		(in-file (org-babel-temp-file "maxima-" ".max"))
-		(cmd (format "%s --very-quiet -r 'batchload(%S)$' %s"
-			     org-babel-maxima-command in-file cmdline)))
+		(cmd (format "%s --very-quiet -r %s$ %s"
+			     org-babel-maxima-command
+                             (shell-quote-argument
+                              (format "batchload(%S)" in-file))
+                             cmdline)))
 	   (with-temp-file in-file (insert (org-babel-maxima-expand body params)))
 	   (message cmd)
            ;; " | grep -v batch | grep -v 'replaced' | sed '/^$/d' "
-- 
2.34.1.windows.1

Re: [PATCH] ob-maxima.el: Fix execution on MS Windows

[Max Nikulin]

Nikolay, I do not have any objections concerning the last version of 
your patch (I have not test it though). It is tracked on 
https://updates.orgmode.org/ however it may take months before it will 
be committed.

Have you signed FSF papers related to copyright? See 
https://orgmode.org/worg/org-contribute.html#copyright There is no 
"TINYCHANGE" keyword in the commit message and I have not found you in 
the list of contributors to Org mode (Bastien likely have access to more 
actual data). Since several your patches have been accepted to Emacs, 
you are close to the limit when formalities become necessary.

Do not consider the comments below as a request to change anything in 
the patch. I am just trying to express more clear what I wrote in the 
previous message.

On 31/12/2021 03:54, Nikolay Kudryavtsev wrote:
>> When some external data is substituted into a Maxima command 
>> (batchload this case) there should be an extra pass of escaping that 
>> protects special characters like quotes (and backslashes?) accordingly 
>> to Maxima rules. 
> Not necessarily, Maxima is capable of understanding unescaped paths, for 
> example, this works:
> 
> maxima --very-quiet -r "batchload(\"/tmp/sp 
> ce/babel-gxqTkM/maxima-ua3e9j.max\")"$

Consider a really peculiar path for tmp files with quotes

      /tmp/")$ something-weird$ "/maxima-ua3e9j.max

I hope, it is unrealistic and will be properly escaped by %S formatter. 
I am a bit surprised that Maxima does not allow to do it in a more 
reliable way since several interfaces exist (WxMaxima, texmacs). Unsure 
if get_application_args is suitable 
https://sourceforge.net/p/maxima/mailman/message/35180908/

> Command line Maxima actually has a batch flag, but using it returns the 
> entire input file in the output too and that seems to be the reason why 
> the original authors of ob-maxima didn't use it.

It would be great if it had another flag to suppress printing content of 
the batch script or a flag that changes a setting that controls such 
behavior.

Re: [PATCH] ob-maxima.el: Fix execution on MS Windows

[Ihor Radchenko]

Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com> writes:

> Anyway, I've tried to get it to work using shell-quote-argument, see the 
> attached patch. Seems to work well enough in practice on both platforms 
> and for cases like (setq temporary-file-directory "/tmp/`echo hi`/").
> From 47690d14ac4838d8e39f08bd8224f0b4af053359 Mon Sep 17 00:00:00 2001
> From: Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com>
> Date: Sun, 26 Dec 2021 22:47:19 +0300
> Subject: [PATCH] ob-maxima.el: Fix execution on MS Windows

Thanks for the patch, and sorry for the late reply.
I have applied your patch onto main adding TINYCHANGE cookie to the
commit message.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=6156b57bdf2b14ccfbf6646e8d217599738b694d

You are also listed as an Org contributor now.
https://git.sr.ht/~bzg/worg/commit/6a5bccb1aeca8e2b3ecef64238002bcfb46f68e0

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] ob-maxima.el: Fix execution on MS Windows

[Nikolay Kudryavtsev]

Oh, nice. Thank you.

I was starting to wonder what happened to this.